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 <[email protected]> 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/[email protected]?part=8
