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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing interrupt mask teardown leaves the device asserting the physical IRQ line when the driver is unloaded, risking an IRQ storm on a shared line.
--

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

drm/tyr: add Job IRQ handling

Add a threaded IRQ wrapper for Tyr interrupt sources and use it to handle
the firmware Job IRQ. A wait queue and a bool flag are added so the handler
can signal firmware readiness when the GLB bit is set.

> diff --git a/drivers/gpu/drm/tyr/driver.rs b/drivers/gpu/drm/tyr/driver.rs
> index bfb0ba19caff9..020df7c257355 100644
> --- a/drivers/gpu/drm/tyr/driver.rs
> +++ b/drivers/gpu/drm/tyr/driver.rs
[ ... ]
> +/// A threaded IRQ wrapper shared by all Tyr interrupt sources.
> +#[pin_data]
> +pub(crate) struct TyrIrq<T: TyrIrqTrait> {
> +    /// The interrupt source.
> +    irq: T,
> +    #[pin]
> +    _pin: PhantomPinned,
> +}

[Severity: High]
Does this wrapper need a Drop (or PinnedDrop) implementation to clean up the
interrupt mask?

If the registration is dropped (for example, during driver unbind or probe
failure), there doesn't seem to be a call to clear_mask(). Leaving the
interrupt unmasked while the handler is unregistered could cause an unhandled
IRQ storm if the device asserts the line. This might potentially cause the
kernel's IRQ core to permanently disable a shared IRQ line.

[ ... ]
> diff --git a/drivers/gpu/drm/tyr/fw/irq.rs b/drivers/gpu/drm/tyr/fw/irq.rs
> index 0000000000000..fcdc0097f2dde
> --- /dev/null
> +++ b/drivers/gpu/drm/tyr/fw/irq.rs
[ ... ]
> +    // SAFETY: The caller guarantees the resulting registration will not be leaked.
> +    let registration = unsafe { TyrIrq::request(pdev, c"job", job_irq) };
> +
> +    // Unmask once the handler has been successfully registered.
> +    Ok(registration?.pin_chain(|reg| {
> +        reg.handler().irq().reenable_mask();
> +        Ok(())
> +    }))
> +}

[Severity: High]
Since the interrupt is explicitly unmasked here via reenable_mask() during
initialization, shouldn't there be a corresponding teardown step to mask it
again when the registration chain is dropped?

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