Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] rocket: interrupt masking bypassed when PM is disabled or suspending --
commit 514c825015ef4be7a517e9ee1eca6f94eae37e6d Author: Jiaxing Hu <[email protected]> Subject: accel/rocket: wait for a running IRQ handler before resetting a core This commit introduces a mechanism to wait for a threaded IRQ handler that might already be running when rocket_reset() is called. It masks the interrupt block, clears the raw status, and calls synchronize_irq() to prevent a concurrent handler from running. It uses pm_runtime_get_if_active() to avoid writing to registers while the power domain is down, preventing an SError. Link: https://lore.kernel.org/all/[email protected]/ Link: https://lore.kernel.org/all/caewpsh5mxtbuknouxm6yecmzyvdowquhvyvhaxq8homthd5...@mail.gmail.com/ Link: https://lore.kernel.org/all/[email protected]/ > diff --git a/drivers/accel/rocket/rocket_job.c > b/drivers/accel/rocket/rocket_job.c > index 575945015ca3b..dfe9135d86af5 100644 > --- a/drivers/accel/rocket/rocket_job.c > +++ b/drivers/accel/rocket/rocket_job.c [ ... ] > @@ -377,9 +377,68 @@ rocket_reset(struct rocket_core *core, struct > drm_sched_job *bad) [ ... ] > + * pm_runtime_get_if_active() does not invoke a callback -- it only > + * takes a reference on an already-active device -- and > + * pm_runtime_put_autosuspend() is asynchronous, so neither can re-enter > + * this driver's runtime PM callbacks while the lock is held. > + */ > + scoped_guard(mutex, &core->job_lock) { > + if (pm_runtime_get_if_active(core->dev) > 0) { > + rocket_pc_writel(core, INTERRUPT_MASK, 0x0); > + rocket_pc_writel(core, INTERRUPT_CLEAR, 0x1ffff); > + pm_runtime_put_autosuspend(core->dev); > + } > + } > + > + /* > + * drm_sched_stop() returns without waiting for a threaded handler that > + * is already running, so wait for one here. This has to stay outside > + * job_lock: the handler takes that lock, so waiting for it while > + * holding it would deadlock instead of fencing anything. > + */ > + synchronize_irq(core->irq); [Severity: High] Does this logic bypass interrupt masking when PM is disabled or during system suspend? If CONFIG_PM is disabled, pm_runtime_get_if_active() unconditionally returns -EINVAL, which evaluates to <= 0. Additionally, during system suspend, the driver uses pm_runtime_force_suspend(). This increments the PM disable_depth before invoking the runtime suspend callback to turn off the clocks. Because disable_depth is non-zero, pm_runtime_get_if_active() returns -EINVAL even though the device is still fully active at that point. In both of these scenarios, the condition evaluates to false and the crucial interrupt masking is skipped. Since the hardware remains active and the interrupt is unmasked, can the handler fire concurrently with or immediately after synchronize_irq(), recreating the exact race condition this code aims to close? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
