Re: [PATCH v6 2/2] drm/tyr: add Job IRQ handling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] JobIrqEvents::wait_ready discards the timeout result from wait_event_timeout, improperly returning success even if the wait times out.
- [High] Race condition during IRQ teardown between JobIrqMaskGuard and the threaded IRQ handler leaves the hardware interrupt incorrectly unmasked.
--

commit 56688d4ee1c985c61ffa39c42b540903cfd3dca8
Author: Laura Nao <laura.nao@collabora.com>

drm/tyr: add Job IRQ handling

This commit adds Job IRQ handling to report requests from the CSF firmware,
including global interface requests to check firmware readiness. It adds
JobIrqEvents to signal readiness and JobIrqMaskGuard to ensure the Job IRQ
is masked before registration is freed.

> diff --git a/drivers/gpu/drm/tyr/fw/irq.rs b/drivers/gpu/drm/tyr/fw/irq.rs
> new file mode 100644
> index 0000000000000..7dd894de18cb2
> --- /dev/null
> +++ b/drivers/gpu/drm/tyr/fw/irq.rs

[ ... ]

> +    /// Waits until the firmware signals readiness via the GLB IRQ bit, or the timeout expires.
> +    pub(crate) fn wait_ready(&self, timeout_ms: Msecs) -> Result {
> +        self.wait.wait_event_timeout(
> +            || self.ready.load(ordering::Acquire),
> +            msecs_to_jiffies(timeout_ms),
> +        )?;
> +
> +        Ok(())
> +    }

[Severity: High]
Does this code accidentally discard the timeout result from
wait_event_timeout()? 

The ? operator will only unwrap wait queue errors, but the actual return
value indicating whether the wait timed out or completed is discarded by
the semicolon, resulting in an unconditional Ok(()) return. 

Could this cause the driver to proceed as if the firmware successfully
signaled readiness even when the initialization timed out, leading to 
invalid hardware interactions or driver hangs?

[ ... ]

> +pub(crate) struct JobIrqMaskGuard<'a>(Arc<IoMem<'a>>);
> +
> +impl Drop for JobIrqMaskGuard<'_> {
> +    fn drop(&mut self) {
> +        self.0.write_reg(JOB_IRQ_MASK::zeroed());
> +    }
> +}

[Severity: High]
Can this mask guard race with the threaded IRQ handler during teardown?

During driver teardown, this Drop implementation zeroes the hardware IRQ
mask before the ThreadedRegistration is dropped. However, if the
threaded handler handle_threaded() is running concurrently on another CPU,
it finishes its execution by unconditionally calling
self.irq.reenable_mask().

If the handler's reenable_mask() executes after this Drop but before
free_irq() completes, it would leave the hardware interrupt incorrectly
unmasked. Since the handler is being unregistered, could this lead to an
unhandled interrupt storm on the shared IRQ line?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728-tyr-irq-v2-v6-0-15c90baed949@collabora.com?part=2