Greg KH
2025-Sep-24 10:40 UTC
[PATCH v4 1/6] nova-core: bitfield: Move bitfield-specific code from register! into new macro
On Tue, Sep 23, 2025 at 06:24:34PM -0400, Joel Fernandes wrote:> Hi Greg, > > On Sun, Sep 21, 2025 at 02:45:27PM +0200, Greg KH wrote: > > On Sun, Sep 21, 2025 at 02:33:56PM +0200, Benno Lossin wrote: > > > On Sun Sep 21, 2025 at 11:36 AM CEST, Greg KH wrote: > > > > On Sat, Sep 20, 2025 at 02:22:27PM -0400, Joel Fernandes wrote: > > > >> The bitfield-specific into new macro. This will be used to define > > > >> structs with bitfields, similar to C language. > > > >> > > > >> Reviewed-by: Elle Rhumsaa <elle at weathered-steel.dev> > > > >> Signed-off-by: Joel Fernandes <joelagnelf at nvidia.com> > > > >> --- > > > >> drivers/gpu/nova-core/bitfield.rs | 314 +++++++++++++++++++++++++++ > > > >> drivers/gpu/nova-core/nova_core.rs | 3 + > > > >> drivers/gpu/nova-core/regs/macros.rs | 259 +--------------------- > > > >> 3 files changed, 327 insertions(+), 249 deletions(-) > > > >> create mode 100644 drivers/gpu/nova-core/bitfield.rs > > > >> > > > >> diff --git a/drivers/gpu/nova-core/bitfield.rs b/drivers/gpu/nova-core/bitfield.rs > > > >> new file mode 100644 > > > >> index 000000000000..ba6b7caa05d9 > > > >> --- /dev/null > > > >> +++ b/drivers/gpu/nova-core/bitfield.rs > > > >> @@ -0,0 +1,314 @@ > > > >> +// SPDX-License-Identifier: GPL-2.0 > > > >> + > > > >> +//! Bitfield library for Rust structures > > > >> +//! > > > >> +//! Support for defining bitfields in Rust structures. Also used by the [`register!`] macro. > > > >> +//! > > > >> +//! # Syntax > > > >> +//! > > > >> +//! ```rust > > > >> +//! #[derive(Debug, Clone, Copy)] > > > >> +//! enum Mode { > > > >> +//! Low = 0, > > > >> +//! High = 1, > > > >> +//! Auto = 2, > > > >> +//! } > > > >> +//! > > > >> +//! impl TryFrom<u8> for Mode { > > > >> +//! type Error = u8; > > > >> +//! fn try_from(value: u8) -> Result<Self, Self::Error> { > > > >> +//! match value { > > > >> +//! 0 => Ok(Mode::Low), > > > >> +//! 1 => Ok(Mode::High), > > > >> +//! 2 => Ok(Mode::Auto), > > > >> +//! _ => Err(value), > > > >> +//! } > > > >> +//! } > > > >> +//! } > > > >> +//! > > > >> +//! impl From<Mode> for u32 { > > > >> +//! fn from(mode: Mode) -> u32 { > > > >> +//! mode as u32 > > > >> +//! } > > > >> +//! } > > > >> +//! > > > >> +//! #[derive(Debug, Clone, Copy)] > > > >> +//! enum State { > > > >> +//! Inactive = 0, > > > >> +//! Active = 1, > > > >> +//! } > > > >> +//! > > > >> +//! impl From<bool> for State { > > > >> +//! fn from(value: bool) -> Self { > > > >> +//! if value { State::Active } else { State::Inactive } > > > >> +//! } > > > >> +//! } > > > >> +//! > > > >> +//! impl From<State> for u32 { > > > >> +//! fn from(state: State) -> u32 { > > > >> +//! state as u32 > > > >> +//! } > > > >> +//! } > > > >> +//! > > > >> +//! bitfield! { > > > >> +//! struct ControlReg { > > > >> +//! 3:0 mode as u8 ?=> Mode; > > > >> +//! 7 state as bool => State; > > > >> +//! } > > > >> +//! } > > > > > > > > As discussed at the conference this week, I do object to this as it > > > > will allow the same mistakes to happen that we used to do in the kernel > > > > for a long time before the regmap() api happened, along with GENMASK(). > > > > > > Have you read the following macro arm of the implementation? > > > > > > // Generates the accessor methods for a single field. > > > ( > > > @leaf_accessor $name:ident $hi:tt:$lo:tt $field:ident > > > { $process:expr } $to_type:ty => $res_type:ty $(, $comment:literal)?; > > > ) => { > > > ::kernel::macros::paste!( > > > const [<$field:upper _RANGE>]: ::core::ops::RangeInclusive<u8> = $lo..=$hi; > > > const [<$field:upper _MASK>]: u32 = ((((1 << $hi) - 1) << 1) + 1) - ((1 << $lo) - 1); > > > const [<$field:upper _SHIFT>]: u32 = Self::[<$field:upper _MASK>].trailing_zeros(); > > > ); > > > > > > $( > > > #[doc="Returns the value of this field:"] > > > #[doc=$comment] > > > )? > > > #[inline(always)] > > > pub(crate) fn $field(self) -> $res_type { > > > ::kernel::macros::paste!( > > > const MASK: u32 = $name::[<$field:upper _MASK>]; > > > const SHIFT: u32 = $name::[<$field:upper _SHIFT>]; > > > ); > > > let field = ((self.0 & MASK) >> SHIFT); > > > > > > Here you can see that it's just a mask + shift operation internally to > > > access the field. > > > > > > $process(field) > > > } > > > > > > ::kernel::macros::paste!( > > > $( > > > #[doc="Sets the value of this field:"] > > > #[doc=$comment] > > > )? > > > #[inline(always)] > > > pub(crate) fn [<set_ $field>](mut self, value: $to_type) -> Self { > > > const MASK: u32 = $name::[<$field:upper _MASK>]; > > > const SHIFT: u32 = $name::[<$field:upper _SHIFT>]; > > > let value = (u32::from(value) << SHIFT) & MASK; > > > self.0 = (self.0 & !MASK) | value; > > > > > > self > > > } > > > ); > > > }; > > > > Yes, that's great, but that is all done in "native cpu" endian, and > > might not actually represent what the hardware does at all, which is > > what I was trying to get at here, sorry for not being more specific. > > > > > Now I too would like to see how exactly this will be used to read data > > > from hardware. But at least in theory if the conversion from hardware > > > endianness to native endianness is done correctly, this will do the > > > right thing :) > > > > That's great, so we are close, but it's not quite correct. How about > > something like: > > > > 0:7 reg_X as __le32 > > 8:15 reg_y as __le32 > > I don't think we should force endianness requirements within specific fields in > the bitfield rust library itself, it is upto the user. bitfields are not only > for registers even in C. If you see on the C side, we have rcu_special union > which uses 'u32' and does not enforce endianness within the fields or bytes > of the struct with respect to the fields. Its all native CPU endian and works > fine. You're basically saying in terms of C that, the designers of the C > bitfield in C standard force the C language to use endianness in the types, no > they can't / shouldn't be forced to.For "cpu native" structures, just use the bit and genmask macros we have today, on top of a normal variable type and you should be fine. The only place you need/want to do stuff like what is being proposed here is when you are trying to match up a data structure that is in hardware to be able to split the values out of it safely. And when dealing with data that goes outside of the kernel (i.e. to/from hardware), you HAVE to specify the endianness of that data as the hardware is the one that defines this, NOT the cpu that the kernel is running on. So you should NEVER see a bitfield structure that is defined just as a "u32" and expect that to work properly when read/written to hardware because while little-endian is what seems to have "won" the recent battles on this topic it's not always the case for many places that Linux runs on today.> For the separate issue of enforcing endianness with respect to (across) > multiple fields, I agree with you that if the user's backend (the consumer of > the data) is not doing such conversion, say via regmap, then that becomes a > problem. But that problem is orthogonal/different and cannot be solved here.But that is exactly what these macros are being defined here for, so to ignore that is going to cause problems :) thanks, greg k-h
Joel Fernandes
2025-Sep-29 19:26 UTC
[PATCH v4 1/6] nova-core: bitfield: Move bitfield-specific code from register! into new macro
On 9/24/2025 12:40 PM, Greg KH wrote:> On Tue, Sep 23, 2025 at 06:24:34PM -0400, Joel Fernandes wrote:[..]> >> For the separate issue of enforcing endianness with respect to (across) >> multiple fields, I agree with you that if the user's backend (the consumer of >> the data) is not doing such conversion, say via regmap, then that becomes a >> problem. But that problem is orthogonal/different and cannot be solved here. > > But that is exactly what these macros are being defined here for, so to > ignore that is going to cause problems :) >If needed, happy to add endianness support as needed by providing additional options to the macro. Based on this thread, it sounds like we want see if that is really needed here or can be solved elsewhere (?). The mental model I kind of have is this macro should only be dealing with CPU native endianness, much like bitfields in C deal with CPU endianness. Hmm. Thanks.
Greg KH
2025-Sep-29 19:37 UTC
[PATCH v4 1/6] nova-core: bitfield: Move bitfield-specific code from register! into new macro
On Mon, Sep 29, 2025 at 03:26:57PM -0400, Joel Fernandes wrote:> On 9/24/2025 12:40 PM, Greg KH wrote: > > On Tue, Sep 23, 2025 at 06:24:34PM -0400, Joel Fernandes wrote: > [..] > > > >> For the separate issue of enforcing endianness with respect to (across) > >> multiple fields, I agree with you that if the user's backend (the consumer of > >> the data) is not doing such conversion, say via regmap, then that becomes a > >> problem. But that problem is orthogonal/different and cannot be solved here. > > > > But that is exactly what these macros are being defined here for, so to > > ignore that is going to cause problems :) > > > > If needed, happy to add endianness support as needed by providing additional > options to the macro. Based on this thread, it sounds like we want see if that > is really needed here or can be solved elsewhere (?). The mental model I kind of > have is this macro should only be dealing with CPU native endianness, much like > bitfields in C deal with CPU endianness. Hmm.Just don't go down the old path like drivers/net/fddi/skfp/h/supern_2.h does with it's definition of: union tx_descr { struct { #ifdef LITTLE_ENDIAN unsigned int tx_length:16 ; /* frame length lower/upper byte */ unsigned int tx_res :8 ; /* reserved (bit 16..23) */ unsigned int tx_xmtabt:1 ; /* transmit abort */ unsigned int tx_nfcs :1 ; /* no frame check sequence */ unsigned int tx_xdone :1 ; /* give up token */ unsigned int tx_rpxm :2 ; /* byte offset */ unsigned int tx_pat1 :2 ; /* must be TXP1 */ unsigned int tx_more :1 ; /* more frame in chain */ #else unsigned int tx_more :1 ; /* more frame in chain */ unsigned int tx_pat1 :2 ; /* must be TXP1 */ unsigned int tx_rpxm :2 ; /* byte offset */ unsigned int tx_xdone :1 ; /* give up token */ unsigned int tx_nfcs :1 ; /* no frame check sequence */ unsigned int tx_xmtabt:1 ; /* transmit abort */ unsigned int tx_res :8 ; /* reserved (bit 16..23) */ unsigned int tx_length:16 ; /* frame length lower/upper byte */ #endif } t ; long i ; } ; CPU endinness, hah, as if...
Danilo Krummrich
2025-Sep-29 20:25 UTC
[PATCH v4 1/6] nova-core: bitfield: Move bitfield-specific code from register! into new macro
On Mon Sep 29, 2025 at 9:26 PM CEST, Joel Fernandes wrote:> On 9/24/2025 12:40 PM, Greg KH wrote: >> On Tue, Sep 23, 2025 at 06:24:34PM -0400, Joel Fernandes wrote: > [..] >> >>> For the separate issue of enforcing endianness with respect to (across) >>> multiple fields, I agree with you that if the user's backend (the consumer of >>> the data) is not doing such conversion, say via regmap, then that becomes a >>> problem. But that problem is orthogonal/different and cannot be solved here. >> >> But that is exactly what these macros are being defined here for, so to >> ignore that is going to cause problems :) >> > > If needed, happy to add endianness support as needed by providing additional > options to the macro. Based on this thread, it sounds like we want see if that > is really needed here or can be solved elsewhere (?). The mental model I kind of > have is this macro should only be dealing with CPU native endianness, much like > bitfields in C deal with CPU endianness. Hmm.At least for register!() we don't need anything else than CPU endianness. In fact, as described in [1], any representation is fine as long as it is consistent. [1] https://lore.kernel.org/all/DD0ZTZM8S84H.1YDWSY7DF14LM at kernel.org/