Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] accel/rocket: raw IRQ status can re-latch before reset in rocket_reset() - [High] accel/rocket: race with pm_runtime_force_suspend() in rocket_reset() --
commit 678645d3ffb5af090f4c6f7400dc95c3975b295a Author: Jiaxing Hu <[email protected]> accel/rocket: wait for a running IRQ handler before resetting a core This commit masks and clears the NPU core's interrupts under job_lock when pm_runtime_get_if_active() succeeds, then calls synchronize_irq() outside job_lock in rocket_reset() after drm_sched_stop(). 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]/ Link: https://lore.kernel.org/all/[email protected]/ 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 03e5f094f60bf..5b2abf40b618c 100644 > --- a/drivers/accel/rocket/rocket_job.c > +++ b/drivers/accel/rocket/rocket_job.c > @@ -401,9 +401,74 @@ rocket_reset(struct rocket_core *core, struct > drm_sched_job *bad) > drm_sched_stop(&core->sched, bad); [ ... ] > + 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); > + } > + } [Severity: High] As reported by Igor Paunovic on v8 [1], can rocket_reset() still race with pm_runtime_force_suspend() or rocket_core_fini() here? When rocket_job_timedout() runs rocket_reset() on core->reset.wq (which is allocated via alloc_ordered_workqueue() without WQ_FREEZABLE), drm_sched_stop() subtracts the pending jobs' credits from core->sched.credit_count. Because rocket_job_is_idle() in drivers/accel/rocket/rocket_job.c only checks credit_count: int rocket_job_is_idle(struct rocket_core *core) { /* If there are any jobs in this HW queue, we're not idle */ if (atomic_read(&core->sched.credit_count)) return false; return true; } and rocket_device_runtime_suspend() in drivers/accel/rocket/rocket_drv.c only checks rocket_job_is_idle() without taking core->job_lock: if (!rocket_job_is_idle(&rdev->cores[core])) return -EBUSY; clk_bulk_disable_unprepare(ARRAY_SIZE(rdev->cores[core].clks), rdev->cores[core].clks); what prevents a concurrent pm_runtime_force_suspend() from disabling the core clocks after pm_runtime_get_if_active(core->dev) returns 1? Unlike normal runtime suspend, pm_runtime_force_suspend() does not check dev->power.usage_count, and __pm_runtime_disable() only waits in __pm_runtime_barrier() for RPM_SUSPENDING or RPM_RESUMING transitions rather than usage_count reaching zero, so clk_bulk_disable_unprepare() and pm_clk_suspend() can turn off the clocks while rocket_reset() is executing rocket_pc_writel() on INTERRUPT_MASK and INTERRUPT_CLEAR. Conversely, in rocket_core_fini() in drivers/accel/rocket/rocket_core.c: void rocket_core_fini(struct rocket_core *core) { pm_runtime_dont_use_autosuspend(core->dev); pm_runtime_disable(core->dev); iommu_group_put(core->iommu_group); core->iommu_group = NULL; rocket_job_fini(core); } if core->in_flight_job is still non-NULL when rocket_core_fini() runs, its runtime PM reference keeps usage_count >= 1 until after synchronize_irq() in rocket_reset(). Doesn't pm_runtime_dont_use_autosuspend() then leave the core powered on while pm_runtime_disable() increments disable_depth, causing pm_runtime_get_if_active(core->dev) in rocket_reset() to return -EINVAL and skip masking INTERRUPT_MASK and clearing INTERRUPT_CLEAR on an active core? > + > + /* > + * 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); > + > + /* > + * No handler is running now, but we might still have stuck jobs. Let's > + * make sure the PM counters stay balanced by manually calling > + * pm_runtime_put_noidle(). > + */ > scoped_guard(mutex, &core->job_lock) { [Severity: High] Can an active NPU task complete after INTERRUPT_CLEAR is written in rocket_reset(), re-latching INTERRUPT_RAW_STATUS before rocket_core_reset() stops the hardware? In rocket_job_handle_irq() in drivers/accel/rocket/rocket_job.c, the completion path clears OPERATION_ENABLE before writing INTERRUPT_CLEAR: rocket_pc_writel(core, OPERATION_ENABLE, 0x0); rocket_pc_writel(core, INTERRUPT_CLEAR, 0x1ffff); However, rocket_reset() only writes INTERRUPT_MASK and INTERRUPT_CLEAR while the NPU core is still running, and does not call rocket_core_reset(core) until after synchronize_irq(core->irq) and iommu_detach_group(NULL, core->iommu_group) have already run: synchronize_irq(core->irq); scoped_guard(mutex, &core->job_lock) { if (core->in_flight_job) pm_runtime_put_noidle(core->dev); iommu_detach_group(NULL, core->iommu_group); core->in_flight_job = NULL; } /* Proceed with reset now. */ rocket_core_reset(core); If an in-flight DPU task finishes between the INTERRUPT_CLEAR write and rocket_core_reset(), won't the hardware set PC_INTERRUPT_RAW_STATUS_DPU_0 or PC_INTERRUPT_RAW_STATUS_DPU_1 in the unmasked INTERRUPT_RAW_STATUS register? Because rocket_job_irq_handler() checks INTERRUPT_RAW_STATUS rather than the masked INTERRUPT_STATUS register on the shared IRQ line: u32 raw_status = rocket_pc_readl(core, INTERRUPT_RAW_STATUS); ... if (!(raw_status & PC_INTERRUPT_RAW_STATUS_DPU_0 || raw_status & PC_INTERRUPT_RAW_STATUS_DPU_1)) return IRQ_NONE; rocket_pc_writel(core, INTERRUPT_MASK, 0x0); return IRQ_WAKE_THREAD; couldn't a shared IOMMU interrupt (such as a fault triggered when iommu_detach_group() detaches the IOMMU while the NPU core is still performing DMA prior to rocket_core_reset()) wake rocket_job_irq_handler_thread() after synchronize_irq() has already returned? [1] https://lore.kernel.org/r/caewpsh7tm9wquj0h13ghkihgzdcdycc1sytuo5b8dpc6cus...@mail.gmail.com -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
