Joel Fernandes
2025-Oct-16 19:28 UTC
[PATCH v7.1 2/4] gpu: nova-core: bitfield: Move bitfield-specific code from register! into new macro
> On Oct 16, 2025, at 1:48?PM, Yury Norov <yury.norov at 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 103That 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 at weathered-steel.dev> >> Reviewed-by: Alexandre Courbot <acourbot at nvidia.com> >> Reviewed-by: Edwin Peer <epeer at nvidia.com> >> Signed-off-by: Joel Fernandes <joelagnelf at 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 at 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
Danilo Krummrich
2025-Oct-16 19:34 UTC
[PATCH v7.1 2/4] gpu: nova-core: bitfield: Move bitfield-specific code from register! into new macro
On Thu Oct 16, 2025 at 9:28 PM CEST, Joel Fernandes wrote:>> On Oct 16, 2025, at 1:48?PM, Yury Norov <yury.norov at gmail.com> wrote: >> ?On Thu, Oct 16, 2025 at 11:13:21AM -0400, Joel Fernandes wrote: >>> +/// >>> +/// 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 at 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.I agree with Joel here. While I'm not super opinionated for general bitfields, for the register!() infrastructure I very much prefer the hi:lo notation, as this is the common notation in datasheets and TRMs. However, if we use hi:lo, we should use it decending, i.e.: bitfield! { struct ControlReg { 7:5 state as u8 => State; 3:0 mode as u8 ?=> Mode; } } - Danilo
John Hubbard
2025-Oct-16 19:42 UTC
[PATCH v7.1 2/4] gpu: nova-core: bitfield: Move bitfield-specific code from register! into new macro
On 10/16/25 12:28 PM, Joel Fernandes wrote:>> On Oct 16, 2025, at 1:48?PM, Yury Norov <yury.norov at gmail.com> wrote: >> ?On Thu, Oct 16, 2025 at 11:13:21AM -0400, Joel Fernandes wrote:...>> 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 isThis is true, but let's be very mindful about pushing that *subjective* readability thing--and strongly prefer Rust's 100 line length convention. The larger Rust for Linux has been quite diligent about trying to stay within that length. In this case, most or even all of these can stay under 100 lines, I suspect.> why it is a checkpatch warning not an error. We have to look it case by case. >thanks, -- John Hubbard