Re: [PATCH v3 16/16] rust: io: register: unify handling of register with/without bitfields

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The `#[unique]` attribute allows implementing `FixedIoLoc` for standard primitive types (like `u32`), which silently binds all implicit type-based writes in a Base to a single specific register.
- [Low] The `register!` macro incorrectly uses the `unique` flag instead of checking for an inline bitfield to validate array registers, allowing `#[unique]` on non-bitfield arrays to bypass validation and generate invalid Rust code.
--

--- Patch [16]: [PATCH v3 16/16] rust: io: register: unify handling of register with/without bitfields ---
commit 154a352ba1f637b66e6182a851b9260fee49b4d6
Author: Gary Guo <[email protected]>

rust: io: register: unify handling of register with/without bitfields

This commit moves the FixedRegister to become a property of the type by
introducing a new FixedIoLoc trait. It exposes this to registers without
inline bitfield definitions via the #[unique] attribute.

> diff --git a/rust/macros/io/register.rs b/rust/macros/io/register.rs
> index e0bba6b71ebc1..84be1a5939596 100644
> --- a/rust/macros/io/register.rs
> +++ b/rust/macros/io/register.rs
[ ... ]
> @@ -217,27 +230,30 @@ pub(crate) fn register(def: RegDef) -> Result<TokenStream> {
>          }
>  
>          match array {
> -            None if bitfield.is_none() => outputs.extend(quote!(
> -                #(#attrs)* #vis const #name: ::kernel::io::register::OffsetLoc<#base, #ty> =
> -                    ::kernel::io::register::OffsetLoc::new(#offset);
> -            )),
> -
> -            _ if bitfield.is_none() => Err(Error::new_spanned(
> -                ty,
> -                "defining without bitfield is not yet supported for this type of register",
> -            ))?,
> -
> -            None => outputs.extend(quote_spanned!(span =>
> -                impl ::kernel::io::register::FixedRegister for #name {
> -                    type Base = #base;
> -                    const OFFSET: usize = #offset;
> +            None => {
> +                if unique {
> +                    outputs.extend(quote!(
> +                        impl ::kernel::io::register::FixedIoLoc<#base> for #ty {
> +                            type Location = ::kernel::io::register::OffsetLoc<#base, #ty>;
> +                            const LOCATION: Self::Location = #name;
> +                        }
> +                    ))
>                  }

[Severity: Medium]
Does this allow implementing FixedIoLoc for standard primitive types like u32?

If a register is defined with a standard primitive type and #[unique] is
applied (e.g., pub REG: #[unique] u32 @ 0x100;), this generates
impl FixedIoLoc<Base> for u32. This would silently bind all implicit
type-based writes in that Base to a single specific register.

If someone accidentally types io.write_reg(val) intending to write to another
register, and val is a u32, the compiler might silently accept it and write to
REG, bypassing the intended type-safety.

Is it possible to restrict this behavior to ensure it doesn't apply to shared
primitive types?

>  
> -                #(#attrs)* #vis const #name: ::kernel::io::register::FixedRegisterLoc<#name> =
> -                    ::kernel::io::register::FixedRegisterLoc::<#name>::new();
> -            )),
> +                outputs.extend(quote_spanned!(span =>
> +                    #(#attrs)* #vis const #name: ::kernel::io::register::OffsetLoc<#base, #ty> =
> +                        ::kernel::io::register::OffsetLoc::new(#offset);
> +                ));
> +            }
>  
>              Some(def) => {
> +                if !unique {
> +                    Err(Error::new_spanned(
> +                        &ty,
> +                        "defining without bitfield is not yet supported for this type of register",
> +                    ))?
> +                }

[Severity: Low]
Does this incorrectly check for the unique flag instead of an inline bitfield
to validate array registers?

For array registers, the macro must ensure a type named #name exists so it can
implement the Array trait on it. If a user defines an array register without a
bitfield but explicitly adds the #[unique] attribute, unique becomes true,
which bypasses the macro's validation check.

This would then generate impl ::kernel::io::register::Array for #name {}
without actually defining #name as a type or constant, resulting in a confusing
"cannot find type" compiler error rather than the intended descriptive macro
error.

Should this check be based on whether a bitfield is present?

> +
>                  let size = &def.size;
>                  let stride = if let Some(stride) = &def.stride {
>                      outputs.extend(quote_spanned!(stride.span().resolved_at(span) =>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=16