Re: [PATCH v8 3/7] drm/tyr: add Memory Management Unit (MMU) support

[sashiko-bot@kernel.org]

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 <boris.brezillon@collabora.com>

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/20260721-fw-boot-b4-v8-0-b28f744581c9@collabora.com?part=3