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
