Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] Teardown race condition: device interrupts are disabled after the IRQ handler is unregistered, which can cause an interrupt storm on a shared IRQ line. - [High] The threaded IRQ handler will enter an infinite loop if the device becomes inaccessible, causing a CPU stall. - [Medium] Missing dummy read after clearing the interrupt mask causes spurious hardirq invocations. --
--- Patch [1]: [PATCH v5] drm/tyr: add Job IRQ handling --- commit 3c87c90f73f8609cae24e8591456f2e88af085b5 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. Link: https://lore.kernel.org/all/[email protected]/ Link: https://lore.kernel.org/rust-for-linux/[email protected]/ Link: https://lore.kernel.org/rust-for-linux/[email protected]/ Link: https://lore.kernel.org/rust-for-linux/[email protected]/ Link: https://lore.kernel.org/all/[email protected]/ > diff --git a/drivers/gpu/drm/tyr/driver.rs b/drivers/gpu/drm/tyr/driver.rs > index d78ad9d292ff7..495cc3baa98c3 100644 > --- a/drivers/gpu/drm/tyr/driver.rs > +++ b/drivers/gpu/drm/tyr/driver.rs [ ... ] > @@ -236,3 +245,102 @@ struct Regulators { > +impl<T: TyrIrqTrait> ThreadedHandler for TyrIrq<T> { > + fn handle(&self) -> ThreadedIrqReturn { > + let masked_status = self.irq.read_status(); > + > + if masked_status == 0 { > + return ThreadedIrqReturn::None; > + } > + self.irq.clear_mask(); > + ThreadedIrqReturn::WakeThread > + } [Severity: Medium] Is a dummy read required after clearing the mask to flush the posted write? Looking at JobIrq::clear_mask() in fw/irq.rs, it performs a raw MMIO write without a subsequent dummy read: fn clear_mask(&self) { self.iomem.write_reg(JOB_IRQ_MASK::zeroed()); } If the write hasn't reached the device when handle() returns WakeThread, could the interrupt controller immediately re-invoke the hardirq handler, causing spurious invocations? > + > + 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; > + } [Severity: High] Does this loop need a safeguard against MMIO reads returning 0xFFFFFFFF? If the device becomes inaccessible (e.g. from surprise removal, loss of power, or a bus error) while handle_threaded() is running, read_raw_status() might return all 1s. Since that value bitwise-ANDed with mask() is non-zero, could this cause the kernel thread to spin indefinitely and result in CPU stalls? > + self.irq.clear_status(raw_status); > + self.irq.handle(raw_status); > + ret = IrqReturn::Handled; > + } > + > + self.irq.reenable_mask(); > + ret > + } > +} > + > +#[pinned_drop] > +impl<T: TyrIrqTrait> PinnedDrop for TyrIrq<T> { > + fn drop(self: Pin<&mut Self>) { > + self.irq.clear_mask(); > + } > +} [Severity: High] Will clear_mask() execute too late during teardown, leading to an interrupt storm? Since TyrIrq is embedded inside the ThreadedRegistration returned by request_threaded_irq_by_name(), the Rust drop order will drop the ThreadedRegistration first (which calls free_irq()), followed by TyrIrq (which calls clear_mask() here). If the device asserts an interrupt after free_irq() completes but before clear_mask() runs, could the unhandled interrupt on a shared line result in an IRQ storm that permanently disables the IRQ line? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
