Hi Miguel, On 10/20/2025 5:30 PM, Miguel Ojeda wrote: > Hi Joel, > > A few nits below (I do sometimes this kind of docs review to try to > keep a consistent style across all Rust code).
Thanks a lot for these, I studied all of the suggestions and agree with them. May I also suggest to add some of these suggestions to the kernel rust coding guidelines document, that way others new to sending rust kernel patches don't miss it (example not adding a period at the end of a markdown doc header.). But I will make it a point to check all my patches to make sure it confirms to the suggestions. Also a lot of your suggestions are related to how it looks it rustdoc, so I will try to build rustdoc and see what it looks like as well, to get an idea of when things in my patches could be improved. thanks, - Joel > > On Mon, Oct 20, 2025 at 8:56 PM Joel Fernandes <[email protected]> wrote: >> >> +//! .set_table_frame_number(new_table.frame_number()); >> +//! // Call a function to write PDE to VRAM address > > Newline between these. Period ad the end. > >> +//! ## Given a PTE, Get or allocate a PFN (page frame number). > > In headers, no period at the end. Also, is "Get" intended to be capitalized? > >> +//! // Call a function to read 64-bit PTE value from VRAM address > > Period at the end too (more of these elsewhere). > >> +//! if pte.valid() { >> +//! // Return physical frame number from existing mapping >> +//! Ok(Pfn::new(pte.frame_number())) > > Early returns where possible, like in C, i.e. to avoid indentation on > big `else` branches. > >> +/// Memory size constants >> +pub(crate) const KB: usize = 1024; >> +pub(crate) const MB: usize = KB * 1024; > > The docs will only apply to the first item, so this probably was meant > to be a `//` instead of a `///`. > > Or you could use a module to contain these (and then possibly `use` > them outside), and then you can have docs in the module itself, but > that is heavier. > >> +/// Page size: 4 KiB >> +pub(crate) const PAGE_SIZE: usize = 4 * KB; > > `rustdoc` would eventually render the value and the non-evaluated > expression, and in the source code it already says `4 * KB`, so I > think repeating the value isn't needed, unless you mean to show it is > really a multiple of 2. > >> +pub(crate) enum PageTableLevel { >> + Pdb, // Level 0 - Page Directory Base >> + L1, // Level 1 >> + L2, // Level 2 >> + L3, // Level 3 - Dual PDE (128-bit entries) >> + L4, // Level 4 - PTEs > > In this case, I think you meant the other way around, i.e. actual > docs: `///` instead of `//`. > > (Also, unless there is a particular reason (e.g. it is a big table), > please generally put comments on top of things, not at the side, which > matches closer to what is needed for docs.) > >> + /// Convert an Address to a frame number. > > These should eventually be intra-doc links, but at least please use > for the moment backticks when referring to Rust items like types etc. > >> + /// # Example > > We always use the plural for these section headers, even if there is a > single item (e.g. single example). > >> + /// ```no_run >> + /// let va = VirtualAddress::default(); >> + /// let pte_idx = va.level_index(PageTableLevel::L4); >> + /// ``` > > This will need some `use` lines -- not needed now, but just as a > reminder that these will get actually built eventually. > >> + /// Get raw u64 value. > > Intra-doc link or at least backticks. > >> + /// The valid bit is inverted so add an accessor to flip it. >> + pub(crate) fn set_valid(&self, value: bool) -> Pde { > > This docs string sounds like a commit message. > >> +/// Dual PDE at Level 3 - 128-bit entry containing both LPT and SPT >> pointers. >> +/// Lower 64 bits = big/large page, upper 64 bits = small page. > > It sounds like a few of these details should be on its own paragraph > to avoid having them in the short description (title). > >> +/// // Call a function to read dual PDE from VRAM address >> +/// let mut dual_pde: DualPde = read_dual_pde(dpde_addr)?; >> +/// // Call a function to allocate a page table and return its VRAM address >> +/// let spt_addr = allocate_page_table()?; >> +/// dual_pde.set_spt(Pfn::from(spt_addr), AperturePde::VideoMemory); >> +/// // Call a function to write dual PDE to VRAM address >> +/// write_dual_pde(dpde_addr, dual_pde)?; > > Newlines before the comments, i.e. between "conceptual blocks". > >> + pub lpt: Pde, // Large/Big Page Table pointer (2MB pages) - bits 63:0 >> (lower) >> + pub spt: Pde, // Small Page Table pointer (4KB pages) - bits 127:64 >> (upper) > > Docs instead of comments. > >> + /// Check if has valid Small Page Table. > > Missing word? > > Thanks! > > Cheers, > Miguel
