Re: [PATCH v15 04/15] accel/rocket: let the core suspend after a reset

[sashiko-bot@kernel.org]

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 <gahing@gahingwoo.com>

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/20260819073530.6087-1-royalnet026@gmail.com/

> 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/20260924102135.92217-5-gahing@gahingwoo.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261101082307.151957-1-gahing@gahingwoo.com?part=4