Message-ID: <aXgpKOHAmvZuMVSt@yury>
Date: Mon, 26 Jan 2026 21:55:36 -0500
From: Yury Norov <ynorov@...dia.com>
To: Alexandre Courbot <acourbot@...dia.com>
Cc: Miguel Ojeda <ojeda@...nel.org>, Boqun Feng <boqun.feng@...il.com>,
	Gary Guo <gary@...yguo.net>,
	Björn Roy Baron <bjorn3_gh@...tonmail.com>,
	Benno Lossin <lossin@...nel.org>,
	Andreas Hindborg <a.hindborg@...nel.org>,
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
	Danilo Krummrich <dakr@...nel.org>,
	Yury Norov <yury.norov@...il.com>,
	John Hubbard <jhubbard@...dia.com>,
	Alistair Popple <apopple@...dia.com>,
	Joel Fernandes <joelagnelf@...dia.com>,
	Timur Tabi <ttabi@...dia.com>, Edwin Peer <epeer@...dia.com>,
	Eliot Courtney <ecourtney@...dia.com>,
	Daniel Almeida <daniel.almeida@...labora.com>,
	Dirk Behme <dirk.behme@...bosch.com>,
	Steven Price <steven.price@....com>, rust-for-linux@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/6] rust: add `bitfield!` macro

On Mon, Jan 26, 2026 at 10:35:49PM +0900, Alexandre Courbot wrote:
> On Wed Jan 21, 2026 at 6:16 PM JST, Yury Norov wrote:
> > On Tue, Jan 20, 2026 at 03:17:56PM +0900, Alexandre Courbot wrote:
> >> Add a macro for defining bitfield structs with bounds-checked accessors.
> >> 
> >> Each field is represented as a `Bounded` of the appropriate bit width,
> >> ensuring field values are never silently truncated.
> >> 
> >> Fields can optionally be converted to/from custom types, either fallibly
> >> or infallibly.
> >> 
> >> Signed-off-by: Alexandre Courbot <acourbot@...dia.com>
> >> ---
> >>  rust/kernel/bitfield.rs | 503 ++++++++++++++++++++++++++++++++++++++++++++++++
> >>  rust/kernel/lib.rs      |   1 +
> >>  2 files changed, 504 insertions(+)
> >> 
> >> diff --git a/rust/kernel/bitfield.rs b/rust/kernel/bitfield.rs
> >> new file mode 100644
> >> index 000000000000..2926ab802227
> >> --- /dev/null
> >> +++ b/rust/kernel/bitfield.rs
> >> @@ -0,0 +1,503 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +
> >> +//! Support for defining bitfields as Rust structures.
> >> +
> >> +/// Defines a bitfield struct with bounds-checked accessors for individual bit ranges.
> >> +///
> >> +/// # Example
> >> +///
> >> +/// ```rust
> >> +/// use kernel::bitfield;
> >> +/// use kernel::num::Bounded;
> >> +///
> >> +/// bitfield! {
> >> +///     pub struct Rgb(u16) {
> >> +///         15:11 blue;
> >> +///         10:5 green;
> >> +///         4:0 red;
> >> +///     }
> >> +/// }
> >> +///
> >> +/// // Setters can be chained. Bounded::new::<N>() does compile-time bounds checking.
> >> +/// let color = Rgb::default()
> >> +///     .set_red(Bounded::<u16, _>::new::<0x10>())
> >> +///     .set_green(Bounded::<u16, _>::new::<0x1f>())
> >> +///     .set_blue(Bounded::<u16, _>::new::<0x18>());
> >
> > Is there a way to just say:
> >
> >     let color = Rgb::default().
> >             .set_red(0x10)
> >             .set_green(0x1f)
> >             .set_blue(0x18)
> >
> > I think it should be the default style. Later in the patch you say: 
> >
> >     Each field is internally represented as a [`Bounded`]
> >
> > So, let's keep implementation decoupled from an interface?
> 
> That is unfortunately not feasible, but the syntax above should seldomly
> be used outside of examples.

The above short syntax is definitely more desired over that wordy and
non-trivial version that exposes implementation internals.

A regular user doesn't care of the exact mechanism that protects the
bitfields. He wants to just assign numbers to the fields, and let
your machinery to take care of the integrity. 

Can you please explain in details why that's not feasible, please
do it in commit message. If it's an implementation constraint,
please consider to re-implement. 

> >> +///
> >> +/// assert_eq!(color.red(), 0x10);
> >> +/// assert_eq!(color.green(), 0x1f);
> >> +/// assert_eq!(color.blue(), 0x18);
> >> +/// assert_eq!(
> >> +///     color.as_raw(),
> >> +///     (0x18 << Rgb::BLUE_SHIFT) + (0x1f << Rgb::GREEN_SHIFT) + 0x10,
> >> +/// );
> >
> > What about: 
> >
> >         bitfield! {
> >             pub struct Rgb(u16) {
> >                 15:11 blue;
> >                 10:5  Blue;
> >                 4:0   BLUE;
> >             }
> >         }
> >
> 
> Oh, all of these will name-clash very badly. :) At the end of the day,
> it is still a macro.
> 
> > What Rgb::BLUE_SHIFT would mean in this case? Maybe Rgb::SHIFT(blue)?
> 
> You wouldn't even have the luxury to yse `BLUE_SHIFT` here because where
> would be conflicting definitions and thus a build error.
> 
> >
> >> +///
> >> +/// // Convert to/from the backing storage type.
> >> +/// let raw: u16 = color.into();
> >
> > What about: 
> >
> >         bitfield! {
> >             pub struct Rgb(u16) {
> >                 15:11 blue;
> >                 10:5  set_blue;
> >                 4:0   into;
> >             }
> >         }
> >
> > What color.set_blue() and color.into() would mean? Even if they work,
> > I think, to stay on safe side there should be a more conventional set
> > of accessors: color.get(into), color.set(set_blue, 0xff) and son on.
> 
> This would just not build.

I know it wouldn't. I am just trying to understand corner cases that may
(and would!) frustrate people for decades. 

I understand that this implementation works just great for the registers,
but my role is to make sure that it would work equally well for everyone. 
Now, for example, Rust, contrary to C in Linux, actively uses camel case. 
So, the blue vs Blue vs BLUE restriction is a very valid concern. The
same for reserved words like 'into'. As long as the API matures, the
number of such special words would only grow. The .shr() and .shl() that
you add in this series are the quite good examples.

Let's make a step back and just list all limitations that come with this
implementation.

Again, this all is relevant for a basic generic data structure. If we
consider it a supporting layer for the registers, everything is totally
fine. In that case, we should just give it a more specific name, and
probably place in an different directory, closer to IO APIs.  

Thanks,
Yury

Powered by blists - more mailing lists