Re: [PATCH v7.1 2/4] gpu: nova-core: bitfield: Move bitfield-specific code from register! into new macro

[Joel Fernandes <joelagnelf@nvidia.com>]

> On Oct 16, 2025, at 1:48 PM, Yury Norov <yury.norov@gmail.com> wrote:
> 
> On Thu, Oct 16, 2025 at 11:13:21AM -0400, Joel Fernandes wrote:
>> Move the bitfield-specific code from the register macro into a new macro
>> called bitfield. This will be used to define structs with bitfields,
>> similar to C language.
> 
> Can you please fix line length issues before v8?
> 
> $ awk '{print length}' drivers/gpu/nova-core/bitfield.rs | sort -rn | uniq -c
>      1 118
>      1 116
>      1 113
>      1 109
>      1 105
>      1 103

That is intentional. I will look again but long lines can be a matter of style
and if wrapping effects readability then we do not want to do that. That is
why it is a checkpatch warning not an error. We have to look it case by case.

>      ...
> 
>> Reviewed-by: Elle Rhumsaa <elle@weathered-steel.dev>
>> Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
>> Reviewed-by: Edwin Peer <epeer@nvidia.com>
>> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
>> ---
>> drivers/gpu/nova-core/bitfield.rs    | 319 +++++++++++++++++++++++++++
>> drivers/gpu/nova-core/nova_core.rs   |   3 +
>> drivers/gpu/nova-core/regs/macros.rs | 259 +---------------------
>> 3 files changed, 332 insertions(+), 249 deletions(-)
>> create mode 100644 drivers/gpu/nova-core/bitfield.rs
> 
> ...
> 
>> +///
>> +/// bitfield! {
>> +///     struct ControlReg {
>> +///         3:0 mode as u8 ?=> Mode;
>> +///         7:7 state as bool => State;
>> +///     }
>> +/// }
> 
> This notation is really unwelcome this days. It may be OK for a random
> macro in some local driver, but doesn't really work for a global basic
> data type:
> 
> https://lore.kernel.org/all/CAHk-=whoOUsqPKb7OQwhQf9H_3=5sXGPJrDbfQfwLB3Bi13tcQ@mail.gmail.com/
> 
> I've already shared this link with you, and shared my concern.
> 
> I realize that rust/bitfield derives the GENMASK(hi, lo) notation here,
> and GENMASK() derives verilog or hardware specs popular notations. But
> software people prefer lo:hi. I'm probably OK if you choose C-style
> start:nbits, if you prefer. But let's stop this hi:lo early, please.
> 
> Let me quote Linus from the link above:
> 
>  It does "high, low", which is often very unintuitive, and in fact the
>  very commit that introduced this thing from hell had to convert the
>  sane "low,high" cases to the other way around.

I agree with Linus but I disagree with comparing it with these macros.
I agree with Linus it is oddly unreadable when used as function parameters.
But that is a different syntax. Over here we are using colons with sufficient whitespace around hi:lo.

> 
>  See commit 10ef6b0dffe4 ("bitops: Introduce a more generic BITMASK
>  macro"), and notice how ALMOST ALL use cases were switched around to
>  the illogical "high,low" format by that introductory phase.

Again this is different syntax so assuming that Linus will not be ok with it is a stretch IMO.

The rust macros here have their own syntax and are not function parameters.

> 
>  And yes, I understand why that person did it: many datasheets show
>  bits in a register graphically, and then you see that "high .. low"
>  thing in a rectangle that describes the register, and that ordering
>  them makes 100% sense IN THAT CONTEXT.
> 
>  But it damn well does not make sense in most other contexts.
> 
>  In fact, even in the context of generating mask #defines, it actually
>  reads oddly, because you end up having things like
> 
>    /* Status register (SR) */
>    #define I2C_SR_OP               GENMASK(1, 0)   /* Operation */
>    #define I2C_SR_STATUS           GENMASK(3, 2)   /* controller status */
>    #define I2C_SR_CAUSE            GENMASK(6, 4)   /* Abort cause */
>    #define I2C_SR_TYPE             GENMASK(8, 7)   /* Receive type */
>    #define I2C_SR_LENGTH           GENMASK(19, 9)  /* Transfer length */

Sure this is super odd indeed. But again not apples to apples here.

thanks,

 - Joel


> 
>  ...
> 
> Now compare it to what we've got in nova right now:
> 
>  register!(NV_PMC_BOOT_0 @ 0x00000000, "Basic revision information about the GPU" {
>      3:0     minor_revision as u8, "Minor revision of the chip";
>      7:4     major_revision as u8, "Major revision of the chip";
>      8:8     architecture_1 as u8, "MSB of the architecture";
>      23:20   implementation as u8, "Implementation version of the architecture";
>      28:24   architecture_0 as u8, "Lower bits of the architecture";
>  });
> 
> There's so far 36 thousands of GENMASK()s in the kernel, and only 67
> register!()s. It's a separate topic what to do with the GENMASK()
> codebase. But now that you do this massive refactoring for the
> register!() macro, let's convert those 67 users to the lo:hi notation.
> 
> As a side note, for GENMASKs, I tried this trick:
> 
>        #define GENMASK(a, b) UNSAFE_GENMASK(MIN(a, b), MAX(a, b))
> 
> It works, but bloats defconfig kernel for another 1K. I don't think it
> would add to readability on both C and rust sides.
> 
> Thanks,
> Yury