Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] accel/rocket: asynchronous autosuspend in rocket_reset() skips domain reset - [Medium] accel/rocket: pm_runtime_put_autosuspend() precedes detach and reset --
commit a40b84e5c2017a835f035c1e3114023e0444d2a9 Author: Jiaxing Hu <[email protected]> accel/rocket: let the core suspend after a reset This commit replaces pm_runtime_put_noidle() with pm_runtime_put_autosuspend() in rocket_reset() when dropping the in-flight job's runtime PM reference so that the NPU core can runtime suspend and power-cycle its domain after a timeout. 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 5b2abf40b618c..152a9098634b9 100644 > --- a/drivers/accel/rocket/rocket_job.c > +++ b/drivers/accel/rocket/rocket_job.c > @@ -467,12 +467,12 @@ rocket_reset(struct rocket_core *core, struct > drm_sched_job *bad) > > /* > * 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(). > + * make sure the PM counters stay balanced by putting the reference the > + * job took, and request idle while doing it so the core can suspend. > */ > scoped_guard(mutex, &core->job_lock) { > if (core->in_flight_job) > - pm_runtime_put_noidle(core->dev); > + pm_runtime_put_autosuspend(core->dev); [Severity: High] As reported by Sashiko on v14 [1], can pm_runtime_put_autosuspend() skip the required power domain cycle when another job is already queued or submitted shortly after the reset? In rocket_core_init(), runtime PM autosuspend is configured with a 50 ms delay: pm_runtime_use_autosuspend(dev); ... pm_runtime_set_autosuspend_delay(dev, 50); When rocket_reset() calls pm_runtime_put_autosuspend(core->dev), it marks the device busy and schedules the autosuspend timer 50 ms in the future, returning while core->dev is still in RPM_ACTIVE. Immediately after that, rocket_reset() runs rocket_core_reset(core), clears core->reset.pending, and restarts the scheduler via drm_sched_start(&core->sched, 0). If another job is already queued in the scheduler (for example from a multi-job submission in rocket_ioctl_submit()) or submitted within the 50 ms window, rocket_job_run() will call: ret = pm_runtime_resume_and_get(core->dev); while core->dev is still RPM_ACTIVE, which increments usage_count and cancels the pending autosuspend without ever suspending the device or cycling its power domain. On RK3576, won't this leave the NPU bus interface and IOMMU wedged for subsequent jobs? [Severity: Medium] Also in rocket_reset(), is it safe to call pm_runtime_put_autosuspend(core->dev) before iommu_detach_group(NULL, core->iommu_group) and rocket_core_reset(core) have completed? Unlike the normal completion path in rocket_job_handle_irq(), which detaches the IOMMU group before dropping the runtime PM reference: iommu_detach_group(NULL, iommu_group_get(core->dev)); dma_fence_signal(core->in_flight_job->done_fence); pm_runtime_put_autosuspend(core->dev); core->in_flight_job = NULL; rocket_reset() drops the last runtime PM reference on core->dev before calling iommu_detach_group() and rocket_core_reset(): scoped_guard(mutex, &core->job_lock) { if (core->in_flight_job) pm_runtime_put_autosuspend(core->dev); iommu_detach_group(NULL, core->iommu_group); core->in_flight_job = NULL; } /* Proceed with reset now. */ rocket_core_reset(core); If autosuspend_delay_ms is set to 0 or negative via sysfs, or if the reset worker is preempted past the autosuspend delay, runtime suspend on pm_wq can run rocket_device_runtime_suspend() and drop supplier references via __rpm_put_suppliers(core->dev) concurrently with iommu_detach_group() and rocket_core_reset(core). Could this turn off core clocks and the power domain before rocket_core_reset() pulses the synchronous reset lines, and race with rk_iommu_identity_attach() in drivers/iommu/rockchip-iommu.c: iommu->domain = identity_domain; ... ret = pm_runtime_get_if_in_use(iommu->dev); WARN_ON_ONCE(ret < 0); if (ret > 0) { rk_iommu_disable(iommu); pm_runtime_put(iommu->dev); } such that both rk_iommu_identity_attach() and rk_iommu_suspend() skip rk_iommu_disable(iommu)? [1] https://lore.kernel.org/r/[email protected] -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
