Message-Id: <DG1Q6I5CTD87.RYS7QBJCHZ7T@nvidia.com>
Date: Fri, 30 Jan 2026 15:55:03 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Gary Guo" <gary@...yguo.net>
Cc: "Danilo Krummrich" <dakr@...nel.org>, "Alice Ryhl"
 <aliceryhl@...gle.com>, "Daniel Almeida" <daniel.almeida@...labora.com>,
 "Miguel Ojeda" <ojeda@...nel.org>, "Boqun Feng" <boqun.feng@...il.com>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>, "Benno Lossin"
 <lossin@...nel.org>, "Andreas Hindborg" <a.hindborg@...nel.org>, "Trevor
 Gross" <tmgross@...ch.edu>, "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>,
 "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 v4 5/7] rust: io: add `register!` macro

On Thu Jan 29, 2026 at 11:10 PM JST, Gary Guo wrote:
> On Thu Jan 29, 2026 at 8:00 AM GMT, Alexandre Courbot wrote:
>> On Thu Jan 29, 2026 at 1:16 AM JST, Gary Guo wrote:
>>> On Wed Jan 28, 2026 at 2:37 AM GMT, Alexandre Courbot wrote:
>>>> Add a macro for defining hardware register types with I/O accessors.
>>>>
>>>> Each register 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.
>>>>
>>>> The address of registers can be direct, relative, or indexed, supporting
>>>> most of the patterns in which registers are arranged.
>>>>
>>>> Tested-by: Dirk Behme <dirk.behme@...bosch.com>
>>>> Signed-off-by: Alexandre Courbot <acourbot@...dia.com>
>>>> ---
>>>>  rust/kernel/io.rs          |    1 +
>>>>  rust/kernel/io/register.rs | 1287 ++++++++++++++++++++++++++++++++++++++++++++
>>>>  2 files changed, 1288 insertions(+)
>>>>
>>>> diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
>>>> index 056a3ec71647..112f43ecbf88 100644
>>>> --- a/rust/kernel/io.rs
>>>> +++ b/rust/kernel/io.rs
>>>> @@ -11,6 +11,7 @@
>>>>  
>>>>  pub mod mem;
>>>>  pub mod poll;
>>>> +pub mod register;
>>>>  pub mod resource;
>>>>  
>>>>  pub use resource::Resource;
>>>> diff --git a/rust/kernel/io/register.rs b/rust/kernel/io/register.rs
>>>> new file mode 100644
>>>> index 000000000000..fc85dcd1f09a
>>>> --- /dev/null
>>>> +++ b/rust/kernel/io/register.rs
>>>> @@ -0,0 +1,1287 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +
>>>> +//! A macro to define register layout and accessors.
>>>> +//!
>>>> +//! A single register typically includes several fields, which are accessed through a combination
>>>> +//! of bit-shift and mask operations that introduce a class of potential mistakes, notably because
>>>> +//! not all possible field values are necessarily valid.
>>>> +//!
>>>> +//! The [`register!`] macro in this module provides an intuitive and readable syntax for defining a
>>>> +//! dedicated type for each register. Each such type comes with its own field accessors that can
>>>> +//! return an error if a field's value is invalid.
>>>> +//!
>>>> +//! [`register!`]: kernel::register!
>>>> +
>>>> +use core::ops::Deref;
>>>> +
>>>> +use crate::io::{
>>>> +    IoCapable,
>>>> +    IoKnownSize, //
>>>> +};
>>>> +
>>>> +/// Trait providing a base address to be added to the offset of a relative register to obtain
>>>> +/// its actual offset.
>>>> +///
>>>> +/// The `T` generic argument is used to distinguish which base to use, in case a type provides
>>>> +/// several bases. It is given to the `register!` macro to restrict the use of the register to
>>>> +/// implementors of this particular variant.
>>>> +pub trait RegisterBase<T> {
>>>> +    /// Base address to which register offsets are added.
>>>> +    const BASE: usize;
>>>> +}
>>>> +
>>>> +/// Trait providing I/O read/write operations for register storage types.
>>>> +///
>>>> +/// This trait is implemented for all integer types on which I/O can be performed, allowing the
>>>> +/// `register!` macro to generate appropriate I/O accessor methods based on the register's storage
>>>> +/// type.
>>>> +pub trait RegisterIo: Sized {
>>>
>>> Is this trait intended for public usage or just internal detail of `register!()`
>>> macro?
>>>
>>> If it's the former, then we should probably just put the method into `IoCapable`
>>> and allow generic-read in Io. If it's the latter, let's `#[doc(hidden)]` this so
>>> it won't get abused.
>>
>> It is an internal detail of `register!()` and not supposed to be used by anyone
>> else.
>>
>>>
>>>> +    /// Read a value from the given offset in the I/O region.
>>>> +    fn read<T, I>(io: &T, offset: usize) -> Self
>>>> +    where
>>>> +        T: Deref<Target = I>,
>>>> +        I: IoKnownSize + IoCapable<Self>;
>>>
>>> I think generally `Deref` bound shouldn't be exposed to user. What smart
>>> pointers are involved here, and can we implement forwarding impls of `Io`?
>>
>> Removing the requirement for `Deref` in the `RegisterIo` trait is simple - we
>> can just call `Deref` in the register IO accessors.
>
> The issue with `Deref` bound here is that now you *require* a level of
> indirection. If something implements `Io` directly, you cannot use it with the
> method. While `&Bar` is accepted, `&Mmio` is not because `Mmio` is a direct
> implementor of `Io` and not deref to it.
>
> A `Deref` bound also does not help if, say, a type is `Arc<Bar>` which needs two
> level of dereffing before it is Io. For consistency I think it's best to avoid
> `Deref` call all together

Do you mean that `Bar` should implement `Io`, `IoKnownSize`, and all the
required `IoCapable`s? That's a lot of boilerplate I'm afraid, and that
would need to be repeated for all other I/O proxy types.

No, I'm starting to believe that the fundamental issue is that the
register interface does its I/O backwards, and that design issue is only
exacerbated by the recent I/O redesign. I.e. instead of doing

  regs::NV_PMC_BOOT_0::read(bar);

We should really do

  bar.read_reg::<regs::NV_PMC_BOOT_0>();

Because that way we can use deref coercion.

That's quite a big redesign though, which means I don't believe
`register!` can make it this cycle... I'll give it a try though.

<snip>
>>>> +        $vis struct $name($storage);
>>>> +
>>>> +        #[allow(dead_code)]
>>>> +        impl $name {
>>>> +            /// Creates a bitfield from a raw value.
>>>> +            $vis const fn from_raw(value: $storage) -> Self {
>>>> +                Self(value)
>>>> +            }
>>>> +
>>>> +            /// Creates a zeroed bitfield value.
>>>> +            ///
>>>> +            /// This is a const alternative to the `Zeroable::zeroed()` trait method.
>>>> +            $vis const fn zeroed() -> Self {
>>>> +                Self(0)
>>>> +            }
>>>
>>> All types that impl `Zeroable` automatically have the `::zeroed()` fn provided
>>> via the trait.
>>
>> Yes, but that method from the trait cannot be used in const context, and
>> `zeroed` is the starting point for building register values from scratch (and
>> thus constant values).
>
> `pin_init:::zeroed()` is a const function.

Ah, I thought about using `Zeroable::zeroed()` at first, but indeed we
can use the function instead.

Strangely though, I expected to be able to do

  const V: BOOT_0 = pin_init::zeroed()

and have the compiler infer the generic type, but for some reason I have
to specify it explicitly:

  const V: BOOT_0 = pin_init::zeroed::<BOOT_0>()

Not a big deal, but a const method would reduce this to 

  const V: BOOT_0 = BOOT_0::zeroed()

(for variables `let v: BOOT_0 = pin_init::zeroed()` works fine.)

Powered by blists - more mailing lists