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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] accel/rocket: asynchronous autosuspend in rocket_reset() skips
  domain reset
- [Medium] accel/rocket: rocket_core_reset() runs without PM ref if job
  IRQ races
--

commit 8873affc45494653d715f08563b9727e826503f5
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() and moves the call after
rocket_core_reset() so the NPU core stays powered during the reset and can
suspend afterward.

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 8b4ad028bc9b2..41c723238b2b7 100644
> --- a/drivers/accel/rocket/rocket_job.c
> +++ b/drivers/accel/rocket/rocket_job.c
> @@ -397,6 +397,8 @@ static void rocket_job_handle_irq(struct rocket_core *core)
>  static void
>  rocket_reset(struct rocket_core *core, struct drm_sched_job *bad)
>  {
> +	bool put_job_ref;
> +
>  	if (!atomic_read(&core->reset.pending))
>  		return;
>  
> @@ -415,11 +417,10 @@ rocket_reset(struct rocket_core *core, struct drm_sched_job *bad)
>  	/*
>  	 * Remaining interrupts have been handled, but we might still have
>  	 * stuck jobs. Let's make sure the PM counters stay balanced by
> -	 * manually calling pm_runtime_put_noidle().
> +	 * dropping the job's reference once the core has been reset.
>  	 */
>  	scoped_guard(mutex, &core->job_lock) {
> -		if (core->in_flight_job)
> -			pm_runtime_put_noidle(core->dev);
> +		put_job_ref = core->in_flight_job;
>  
>  		iommu_detach_group(NULL, core->iommu_group);
>  
> @@ -429,6 +430,9 @@ rocket_reset(struct rocket_core *core, struct drm_sched_job *bad)
>  	/* Proceed with reset now. */
>  	rocket_core_reset(core);

[Severity: Medium]
As reported by Igor Paunovic on v8 [1], can rocket_core_reset() still run
without holding a runtime PM reference if the in-flight job completes
concurrently with the timeout?

CPU0 (reset work)                 CPU1 (threaded IRQ)
rocket_reset()
  drm_sched_stop()
  pm_runtime_get_if_active()
  ...
  pm_runtime_put_autosuspend()
                                  rocket_job_handle_irq()
                                    pm_runtime_put_autosuspend(core->dev)
                                    core->in_flight_job = NULL
  synchronize_irq(core->irq)
  put_job_ref = core->in_flight_job (NULL)
  rocket_core_reset(core)         <-- no runtime PM ref held

rocket_reset() relies on the in-flight job's runtime PM reference to keep
core->dev powered across rocket_core_reset(), because the temporary
reference from pm_runtime_get_if_active() is dropped before
synchronize_irq().  If rocket_job_handle_irq() finishes the last task and
clears core->in_flight_job before synchronize_irq() returns,
rocket_core_reset() executes with zero runtime PM references held, which
allows core->dev to suspend before or during the reset and hang the board.

>  
> +	if (put_job_ref)
> +		pm_runtime_put_autosuspend(core->dev);

[Severity: High]
Does pm_runtime_put_autosuspend() guarantee that the power domain is
switched off and on again before the next job runs?

rocket_core_init() configures a 50 ms autosuspend delay on core->dev:

drivers/accel/rocket/rocket_core.c:rocket_core_init() {
	...
	pm_runtime_use_autosuspend(dev);
	...
	pm_runtime_set_autosuspend_delay(dev, 50);
}

In rocket_reset(), pm_runtime_put_autosuspend(core->dev) only arms the
50 ms timer and leaves core->dev active, and drm_sched_start(&core->sched,
0) is called immediately afterward.  When another job is already queued in
the scheduler (or submitted within 50 ms), rocket_job_run() calls
pm_runtime_resume_and_get(core->dev) while core->dev is still active,
canceling the autosuspend timer without ever suspending the device or
cycling the power domain.  On RK3576, the NPU bus interface is then not
reset and the job after the timeout still fails.

[1] https://lore.kernel.org/r/CAEWPSH7tM9WQUJ0H13ghkiHGzDcdycC1syTuO5B8DpC6cUSnfg@mail.gmail.com

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