Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] Delayed validation of the hardware AS_PRESENT mask causes probe to succeed but runtime operations to fail. - [Medium] Busy-waiting for up to 10ms in a sleepable context. --
--- Patch [3]: [PATCH v8 3/7] drm/tyr: add Memory Management Unit (MMU) support --- commit d32ee18da5af7d1944026f776d63573705b0da02 Author: Boris Brezillon <[email protected]> drm/tyr: add Memory Management Unit (MMU) support This commit adds Memory Management Unit (MMU) support in Tyr, wrapping a SlotManager instance to allocate MMU address-space slots for virtual memory. It also introduces the AddressSpaceManager for hardware-specific MMU register commands and updates Kconfig dependencies. > diff --git a/drivers/gpu/drm/tyr/mmu/address_space.rs > b/drivers/gpu/drm/tyr/mmu/address_space.rs > index 0000000000000..add5100b26f2e > --- /dev/null > +++ b/drivers/gpu/drm/tyr/mmu/address_space.rs [ ... ] > +impl<'bound> AddressSpaceManager<'bound> { > + /// Creates a new address space manager. > + /// > + /// Initializes the manager with references to the platform device and > + /// I/O memory region, along with the bitmask of available AS slots. > + pub(super) fn new( > + dev: &'bound Device<Bound>, > + iomem: Arc<IoMem<'bound>>, > + as_present: u32, > + ) -> Result<AddressSpaceManager<'bound>> { > + Ok(Self { > + dev, > + iomem, > + as_present, > + }) > + } > + > + /// Validates that an AS slot number is within range and present in > hardware. > + /// > + /// Checks that the slot index is less than [`MAX_AS`] and that > + /// the corresponding bit is set in the `as_present` mask read from the > GPU. > + /// > + /// Returns [`EINVAL`] if the slot is out of range or not present in > hardware. > + fn validate_as_slot(&self, as_nr: usize) -> Result { > + if as_nr >= MAX_AS { > + dev_err!( > + self.dev, > + "AS slot {} out of valid range (max {})\n", > + as_nr, > + MAX_AS > + ); > + return Err(EINVAL); > + } > + > + if (self.as_present & (1 << as_nr)) == 0 { > + dev_err!( > + self.dev, > + "AS slot {} not present in hardware (AS_PRESENT={:#x})\n", > + as_nr, > + self.as_present > + ); > + return Err(EINVAL); > + } > + > + if self.as_present.trailing_ones() != self.as_present.count_ones() { [Severity: Medium] Should the as_present mask validation occur during probe in AddressSpaceManager::new() instead of on every runtime slot operation? If the hardware has a sparse AS_PRESENT mask, wouldn't the driver successfully probe but unconditionally fail all subsequent MMU operations at runtime? Moving this static mask check to probe time would catch unsupported hardware earlier and avoid re-evaluating it on every slot operation. > + dev_err!( > + self.dev, > + "Sparse AS_PRESENT mask is unsupported: {:#x}\n", > + self.as_present > + ); > + return Err(EINVAL); > + } > + > + Ok(()) > + } > + > + /// Waits for an AS slot to become ready (not active). > + /// > + /// Returns an error if polling times out after 10ms or if register > access fails. > + fn as_wait_ready(&self, as_nr: usize) -> Result { > + let io = &*self.iomem; > + let op = || { > + let status_reg = STATUS::try_at(as_nr).ok_or(EINVAL)?; > + Ok(io.read(status_reg)) > + }; > + let cond = |status: &STATUS| -> bool { !status.active_ext() }; > + poll::read_poll_timeout(op, cond, Delta::from_millis(0), > Delta::from_millis(10))?; [Severity: Medium] Can this busy-wait stall the CPU for up to 10ms without yielding? Since the MMU is protected by a sleepable kernel Mutex (specifically as_manager: Mutex<AsSlotManager>), should the sleep delta in poll::read_poll_timeout be a small non-zero value instead of 0ms to allow yielding and prevent degrading system scheduling latency? > + > + Ok(()) > + } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
