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
