Re: [PATCH v2 11/16] rust: io: register: support fixed offset register without bitfield

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The `register!` macro documentation provides an example defining a FIFO register (`TX_FIFO`) with a `u32` type. This violates the subsystem guidelines which mandate that FIFOs must use stream accessors (no byteswapping). Using a `u32` register type implies standard register access (e.g., `writel`), which will byteswap on big-endian architectures and corrupt stream data. Additionally, the macro's syntax rule `$ty: ident $(:: $path_frag:ident)*` restricts the type to paths, preventing developers from directly specifying a byte array (e.g., `[u8; 4]`) as the type to properly model a byteswap-free stream.
--

--- Patch [11]: [PATCH v2 11/16] rust: io: register: support fixed offset register without bitfield ---
commit a13a899311e93c249f5925eff4ce304f82a4cb69
Author: Gary Guo <[email protected]>

rust: io: register: support fixed offset register without bitfield

Add a rule to allow creating `IoLoc` in `regiser!()` using an existing type
and not create a bitfield. Add an example to demonstrate this for FIFO
registers.

> diff --git a/rust/kernel/io/register.rs b/rust/kernel/io/register.rs
> index dc800fc711722..49a61da106c7d 100644
> --- a/rust/kernel/io/register.rs
> +++ b/rust/kernel/io/register.rs

[ ... ]

> @@ -499,6 +516,19 @@ fn into_io_op(self) -> (FixedRegisterLoc<T>, T) {
>  /// In this example, `SCRATCH_BOOT_STATUS` uses the same I/O address as `SCRATCH`, while providing
>  /// its own `completed` field.
>  ///
> +/// If you do not wish to have a bitfield defined, you can also create a register using an existing
> +/// type.
> +///
> +/// ```no_run
> +/// # use kernel::io::*;
> +/// register! {
> +///     base: Region<0x1000>;
> +///
> +///     /// TX FIFO register.
> +///     pub TX_FIFO: u32 @ 0x00001000;

[Severity: Low]
Does this example promote an anti-pattern for FIFO access?

Using u32 for a FIFO implies standard register I/O accessors (like writel or
readl), which will perform byte-swapping on big-endian architectures.
According to the I/O accessor subsystem guidelines, FIFOs should use stream
accessors without byteswapping to prevent data corruption.

Would it be better to avoid using a FIFO as the example here, or use a type
that enforces stream semantics?

> +/// }
> +/// ```
> +///
>  /// ## Relative registers

[ ... ]

> @@ -826,6 +856,23 @@ fn into_io_op(self) -> (FixedRegisterLoc<T>, T) {
>  macro_rules! register {
>      (base: $reg_base:ty;) => {};
>  
> +    // Creates a register at a fixed offset of the MMIO space with provided type.
> +    (
> +        base: $reg_base:ty;
> +        // `$ty` cannot be `:ty` due to follow-set restrictions.
> +        $(#[$attr:meta])* $vis:vis $name:ident: $ty: ident $(:: $path_frag:ident)*

[Severity: Low]
Does this macro rule prevent developers from specifying inline byte arrays
(like [u8; 4]) as the register type?

Since the type matcher is restricted to identifiers and paths, it seems
developers wouldn't be able to directly define a byte array to properly model
a byteswap-free stream without defining a separate type alias first.

Could this be adjusted to support array syntax, or is the limitation strict
enough to require type aliases for stream buffers?

> +            $(@ $offset:literal)?
> +            $(=> $alias:path $([$alias_idx:expr])? )?;
> +        $($rest:tt)*
> +    ) => {

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