Re: [PATCH 01/10] rust: io: register: allow explicit base type specification

[Gary Guo]

On Fri Jul 24, 2026 at 7:38 AM BST, Alexandre Courbot wrote:
> nit: the base-commit trailer points to a commit that only exists in
> linux-next (next-20260720) and is not easily discoverable - it would
> help if you could mention the base in human-resolvable form in the cover
> letter.
>
> On Wed Jul 22, 2026 at 1:54 AM JST, Gary Guo wrote:
>> Currently registers work for all untyped I/O regions, which is not ideal.
>> It allows registers defined for device A to work for another device B and
>> there is no safeguarding at all.
>
> This doesn't sound like a real concern. Registers are typically local to
> a driver, so one cannot use registers for device A on device B even now.
> And within the same crate registers can be isolated by module if needed.

Even for a single driver you don't want to mix register definitions. E.g. if
you have more bars, e.g. if you have subregions of registers, like PFALCON and
PFALCON2.

Your relative register design uses an artifical marker type to distinguish
between two different subregions, which is a very good motiviation to say that
registers should be typed.

>
> Even if we restrict the base type, the examples given in this patch only
> partially address the issue: registers from different drivers using
> regions of identical sizes could be used interchangeably without
> complaint.

This patch set gives you the way to do newtype on regions. Ideally all drivers
would eventually use that and not use the `Region` type.

>
> So I don't think this patchset addresses this particular problem, for
> which namespace isolation is the correct solution anyway.
>
>>
>> All users of the `register!` macro know what type it will be operating on,
>> and that type is consistent across the driver. Therefore, add a `base`
>> parameter to `register!`.
>>
>> Currently this parameter is unused in the generated code; it will be used
>> when all users of `register!` is converted to gain the parameter.
>>
>> Signed-off-by: Gary Guo <gary@xxxxxxxxxxx>
>> ---
>>  rust/kernel/io.rs          |  4 +++
>>  rust/kernel/io/register.rs | 79 ++++++++++++++++++++++++++++++++++++++++++----
>>  2 files changed, 76 insertions(+), 7 deletions(-)
>>
>> diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
>> index 95f46bb75f9e..c5f07c38e59e 100644
>> --- a/rust/kernel/io.rs
>> +++ b/rust/kernel/io.rs
>> @@ -883,6 +883,8 @@ fn try_write<T, L>(self, location: L, value: T) -> Result
>>      /// };
>>      ///
>>      /// register! {
>> +    ///     base: Region;
>> +    ///
>>      ///     VERSION(u32) @ 0x100 {
>>      ///         15:8 major;
>>      ///         7:0  minor;
>> @@ -1028,6 +1030,8 @@ fn write<T, L>(self, location: L, value: T)
>>      /// };
>>      ///
>>      /// register! {
>> +    ///     base: Region<0x1000>;
>
> I'd prefer if we could limit the `base:` parameter to registers defined
> against a specific block (like the current relative registers). Having
> it on all register definitions adds an artificial limitation (that e.g.
> regions must be of a given size, which may not suit drivers that support
> several generations of hardware with varying BAR sizes).

This is more or less intended. Your driver code would have a fixed known minimum
size, even if your code works with multiple generation of hardware.

It's just that code touches registers outside the known minimum size would need
to use fallible accessors because the bar is not guaranteed to be large enough.

If the code is written so you have dedicated HAL paths that the register will
always exist, your should just create a new type for the larger region of
specific hardware generation and define the register on that instead.

E.g.
    
    #[repr(align(4))]
    #[derive(FromBytes, IntoBytes)]
    struct MyNewGenHardware([u8; SZ_1G]); // This new hardware gen has huge register space!

    register! {
        base: MyNewGenHardware;
    }

>
> Top-level registers are currently working fine without this, so it
> doesn't seem justified except by the safeguarding argument, which I
> don't think is relevant here.

This *could* be done, but this means that we need to change register traits to
not have `Base` assoc type but have it as generics. It is however problematic
from trait coherence POV, because Rust complains that downstream crate can
implement

    impl FixedRegister<MyRegion> for () {}

and conflict with blanket impl.

Also I think this is not really needed with strongly typed regions (see above),
hence the design.

Best,
Gary

>
> The syntax (specifying `base:` only once at the top of a register block)
> is a great improvement over the current one; if I were to pursue my own
> fixup series I would definitely have adopted it.
>
> Still going through the series in detail but wanted to raise this point
> first. Overall I think I like the direction, it makes the falcon
> register accesses read much more naturally.