Re: [PATCH v16 04/16] 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 aed4735942793150a3547c69cb7317b3ba01d633
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 release after
rocket_core_reset() so the NPU stays powered during 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 abef12b86d481..4a577b642849d 100644
> --- a/drivers/accel/rocket/rocket_job.c
> +++ b/drivers/accel/rocket/rocket_job.c
> @@ -395,6 +395,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;
>  
> @@ -413,11 +415,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);
>  
> @@ -427,6 +428,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 a runtime PM reference if the in-flight job completes concurrently
with the timeout?

kworker                           <irq/thread>
rocket_reset()
  drm_sched_stop()
  lock(&core->job_lock)
  pm_runtime_get_if_active()
  ...
  pm_runtime_put_autosuspend()
  unlock(&core->job_lock)
  synchronize_irq(core->irq)
                                  rocket_job_handle_irq()
                                    lock(&core->job_lock)
                                    pm_runtime_put_autosuspend(core->dev)
                                    core->in_flight_job = NULL
                                    unlock(&core->job_lock)
  lock(&core->job_lock)
  put_job_ref = core->in_flight_job (NULL)
  unlock(&core->job_lock)
  rocket_core_reset(core)  <-- no runtime PM reference held

Because rocket_job_handle_irq() already dropped the job's runtime PM
reference and rocket_reset() dropped its temporary reference before
synchronize_irq(), core->dev has usage_count == 0 during
rocket_core_reset().  If the autosuspend timer expires before
rocket_core_reset() finishes, the NPU core is reset while unpowered, which
hangs the board.

>  
> +	if (put_job_ref)
> +		pm_runtime_put_autosuspend(core->dev);
> +
>  	/* NPU has been reset, we can clear the reset pending bit. */
>  	atomic_set(&core->reset.pending, 0);

[Severity: High]
Does pm_runtime_put_autosuspend() actually power-cycle the domain when
another job is already queued or submitted right after the reset?

rocket_core_init() enables autosuspend with a 50 ms delay:

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

pm_runtime_put_autosuspend() only arms the 50 ms timer and leaves
core->dev in RPM_ACTIVE, after which rocket_reset() immediately calls
drm_sched_start(&core->sched, 0).  Any job already queued in the scheduler
or submitted within the next 50 ms calls pm_runtime_resume_and_get() in
rocket_job_run(), sees the device still active, and cancels the timer
without ever calling rocket_device_runtime_suspend().  On RK3576, the
power domain is never switched off and back on, leaving the NPU bus
interface and IOMMU wedged for subsequent jobs.

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

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