Re: [PATCH v4 08/16] drm/tyr: specify base type for registers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Out-of-bounds Register Definition: The `DOORBELL` register array exceeds the maximum size of its specified base type `TyrRegisters`.
- [Low] API Misuse: Applying `base: TyrRegisters;` to purely in-memory bitfields allows them to silently alias hardware MMIO registers.
--

commit 1d550d1a42720086769c2c8dc9fd2f019102d497
Author: Gary Guo <gary@garyguo.net>

drm/tyr: specify base type for registers

This commit updates register definitions to specify TyrRegisters as the base
type for all register! invocations.

> diff --git a/drivers/gpu/drm/tyr/driver.rs b/drivers/gpu/drm/tyr/driver.rs
> index bfb0ba19caff9..730b84e37a544 100644
> --- a/drivers/gpu/drm/tyr/driver.rs
> +++ b/drivers/gpu/drm/tyr/driver.rs
> @@ -46,6 +46,7 @@
>  };
>  
>  pub(crate) type IoMem<'a> = kernel::io::mem::IoMem<'a, SZ_2M>;
> +pub(crate) type TyrRegisters = kernel::io::Region<SZ_2M>;

[ ... ]

> diff --git a/drivers/gpu/drm/tyr/fw.rs b/drivers/gpu/drm/tyr/fw.rs
> index 47d25c901bd01..7edb5eff17077 100644
> --- a/drivers/gpu/drm/tyr/fw.rs
> +++ b/drivers/gpu/drm/tyr/fw.rs
> @@ -101,6 +102,8 @@ fn from(value: CacheMode) -> Self {
>  }
>  
>  register! {
> +    base: TyrRegisters;
> +
>       #[allow(non_upper_case_globals)]
>      pub(super) SectionFlags(u32) @ 0x0 {
>          0:0 read => bool;

[Severity: Low]
Does applying an MMIO base to this firmware struct unintentionally allow it
to be used in hardware I/O operations?

By specifying base: TyrRegisters; and @ 0x0, SectionFlags gains I/O traits
tied to the hardware MMIO base. Since this appears to be an in-memory bitfield
used for parsing firmware, should the bitfield! macro be used instead to
prevent it from silently aliasing hardware registers?

[ ... ]

> diff --git a/drivers/gpu/drm/tyr/regs.rs b/drivers/gpu/drm/tyr/regs.rs
> index a62724378ced2..0c419c4e1186c 100644
> --- a/drivers/gpu/drm/tyr/regs.rs
> +++ b/drivers/gpu/drm/tyr/regs.rs
[ ... ]
> @@ -1104,6 +1131,8 @@ fn from(val: MemoryType) -> Self {
>          }
>  
>          register! {
> +            base: TyrRegisters;
> +
>              /// Stage 1 memory attributes (8-bit bitfield).
>              ///
>              /// This is not an actual register, but a bitfield definition used by the MEMATTR

[Severity: Low]
Similar to SectionFlags above, the comment explicitly notes this is not an
actual register. Does adding base: TyrRegisters; here mistakenly grant it
MMIO traits and allow accidental I/O operations?

[ ... ]

> @@ -1770,6 +1811,8 @@ pub(crate) mod doorbell_block {
>      const STRIDE: usize = 0x10000;
>  
>      register! {
> +        base: TyrRegisters;
> +
>          /// Doorbell request register. Write-only.
>          pub(crate) DOORBELL(u32)[NUM_DOORBELLS, stride = STRIDE] @ 0x80000 {
>              /// Doorbell set. Writing 1 triggers the doorbell.

[Severity: Medium]
Does this base type provide enough address space for the entire array?

TyrRegisters is defined in driver.rs as kernel::io::Region<SZ_2M>
(a 2MB limit). However, the DOORBELL array has 64 elements (NUM_DOORBELLS)
with a stride of 0x10000 and starts at offset 0x80000. 

Accessing higher indices in this array will exceed the 2MB limit (e.g., the
maximum offset is 0x80000 + 63 * 0x10000 = 0x470000, ~4.5MB). Will this
result in a build assertion failure or runtime out-of-bounds error when
upper doorbell indices are accessed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901-typed_register-v4-0-5552b1d59525@garyguo.net?part=8