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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Hardware interrupt is eagerly unmasked before the OS-level IRQ handler is registered, creating a race condition that can lead to an unhandled interrupt storm.
- [High] The threaded IRQ handler clears the interrupt status after handling it, causing a race condition where newly arrived interrupts for the same status bits are lost.
--

--- Patch [1]: [PATCH v3] drm/tyr: add Job IRQ handling ---
commit b6b7c4ae779f404042159506914510a46f953463
Author: Laura Nao <[email protected]>

drm/tyr: add Job IRQ handling

This commit adds a threaded IRQ wrapper for Tyr interrupt sources and
implements handling for the firmware Job IRQ. It sets up a wait queue to
signal firmware readiness when the global interface request bit is set.

> diff --git a/drivers/gpu/drm/tyr/driver.rs b/drivers/gpu/drm/tyr/driver.rs
> index 8348c6cd39295..224ee7f2eb3fd 100644
> --- a/drivers/gpu/drm/tyr/driver.rs
> +++ b/drivers/gpu/drm/tyr/driver.rs

[ ... ]

> +    fn handle_threaded(&self) -> IrqReturn {
> +        let mut ret = IrqReturn::None;
> +
> +        loop {
> +            let raw_status = self.irq.read_raw_status() & self.irq.mask();
> +            if raw_status == 0 {
> +                break;
> +            }
> +            self.irq.handle(raw_status);
> +            self.irq.clear_status(raw_status);

[Severity: High]
Is it possible to lose hardware events here?

By calling handle() before clear_status(), if the hardware re-asserts
the same interrupt bit while handle() is executing, the subsequent
clear_status() will write the original raw_status bits to the
Write-1-to-Clear register.

This could inadvertently wipe out the newly arrived event, causing the
next iteration of the loop to read 0 and the new event to never be
handled. Should clear_status() be called before handle() to avoid this race?

> +            ret = IrqReturn::Handled;
> +        }
> +
> +        self.irq.reenable_mask();
> +        ret
> +    }
> +}

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

[ ... ]

> +pub(crate) unsafe fn job_irq_init<'a>(
> +    pdev: &'a platform::Device<Bound>,
> +    iomem: Arc<IoMem<'a>>,
> +    fw_ready: Arc<AtomicBool>,
> +    job_irq_wait: Arc<WaitQueue>,
> +) -> Result<impl PinInit<ThreadedRegistration<'a, TyrIrq<JobIrq<'a>>>, Error> + 'a> {
> +    iomem.write_reg(JOB_IRQ_MASK::zeroed().with_glb(true));

[Severity: High]
Could this cause an unhandled interrupt storm?

This writes to JOB_IRQ_MASK to enable the interrupt on the hardware
immediately, but returns a deferred PinInit descriptor for the OS-level
handler registration.

Because the actual OS-level handler registration is deferred until the
caller pins and executes this descriptor, there is a window where the
hardware can assert an interrupt that the OS has no handler for.

If the interrupt line is shared, could this cause the kernel to permanently
disable the IRQ line? Should the hardware mask be enabled only after
the handler is fully registered?

> +    let job_irq = JobIrq {
> +        iomem: iomem.clone(),
> +        fw_ready,
> +        job_irq_wait,
> +    };
> +    // SAFETY: The caller guarantees the resulting registration will not be leaked.
> +    unsafe { TyrIrq::request(pdev, c_str!("job"), job_irq) }
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1