Re: [PATCH v3 1/2] rust: sizes: add DeviceSize trait for device address space constants

[John Hubbard <jhubbard@nvidia.com>]

On 4/1/26 3:16 AM, Miguel Ojeda wrote:
> On Wed, Apr 1, 2026 at 12:43 AM John Hubbard <jhubbard@nvidia.com> wrote:
>>
>> +//! // usize constant (CPU-side)
> 
> Some nits below -- possibly for apply time.

I've fixed all of them in a pending v4, which I think I'll need
to post in order to rename the trait, as per the other thread with
Alice.

Sorry that these slipped in, there are three separate systems
that should have caught them, each of which requires a bit of
maintenance:

* My brain: still hasn't emotionally absorbed these rules yet,
especially the backtick thing. Work in progress... :)

* checkpatch.pl: it's still delighted to bless all of these
patches with a "has no obvious style problems and is ready for
submission, hooray!".

I could update checkpatch, but...I wonder if it's really
the right tool. I forget what we decided lately, but I vaguely
recall wanting the Rust toolchain to help.

* Local AI reviewer tool(s): this at least I can quickly fix: I've
added a few lines to the rules.

thanks,
-- 
John Hubbard
> 
> Markdown and period:
> 
>     //! // `usize` constant (CPU-side).
> 
>> +//! let num_pages_in_1m: usize = SZ_1M / kernel::page::PAGE_SIZE;
> 
> Perhaps import `PAGE_SIZE` since we are doing imports above anyway?
> 
>> +//! // Device-side constant via the trait
> 
> Period at the end.
> 
>> +//! let heap_size: u64 = 14 * u64::SZ_1M;
>> +//! let small: u32 = u32::SZ_4K;
> 
> I guess the resulting types here are to make it clearer; or do we
> expect people to write the sizes for this? i.e. since part of the
> advantage of using the constants is that you get them already typed, I
> wonder if we should write the code as we would normally expect it to
> be written.
> 
> Also, I always consider in examples whether to add `assert_eq!`s, but
> I am ambivalent for this case.
> 
>> +            /// 0x0000_0400
>> +            SZ_1K,
> 
> This is consistent with the other constants, but I wonder if there was
> a reason to not do something like:
> 
>     /// `0x0000_0400`.
> 
> Anyway, all these is minor or can be fixed on apply.
> 
> These constants are meant to be useful soon, so we could do the same
> as with the other patch, i.e. I could pick it already it depending on
> the rest of the discussion (especially if we confirm that the user
> patch is what we want).
> 
> Thanks!
> 
> Cheers,
> Miguel