[PATCH v8 02/12] accel/rocket: wait for a running IRQ handler before resetting a core

[Igor Paunovic]

Hi Jiaxing,

I have not run v8 on hardware yet. Everything below is source review.
My Tested-by on 1/12 is from the v7 run and I am not extending it to
this patch yet; the last section says why, and what I will run instead.
Line numbers are next-20260814.

The patch is right and the reasoning in the message is right.
synchronize_irq() is the correct one of the pair: __synchronize_irq()
does the hardirq spin and then waits on desc->wait_for_threads
(kernel/irq/manage.c), so it fences the threaded handler, which is the
one that takes job_lock. synchronize_hardirq() would not have. Worth
adding to your placement argument: the deadlock you avoided would have
been a silent hang rather than a lockdep splat, because that wait is on
a waitqueue and not on a lock, so lockdep does not model it at all.
Nothing would have told you.

Two questions, both about what the reset path does not do.

1. Would you also mask the block before the sync?

rocket_reset() is descended from panfrost_reset(), and the comment this
patch replaces is inherited from it word for word. In panfrost that
comment is earned, because three things happen before it:

    job_write(pfdev, JOB_INT_MASK, 0);
    synchronize_irq(pfdev->js->irq);
    ... soft-stop ...
    panfrost_jm_handle_irqs(pfdev);

(panfrost_job.c:692-693 and :710, with the same sentence rocket carries
at :712-715). Rocket inherited the sentence and none of the three steps,
which is your point, and 2/12 restores one of them.

Without the device level mask a fresh completion interrupt can still
arrive after synchronize_irq() returns. INTERRUPT_MASK is armed on every
submit (rocket_job.c:142) and the only place it is cleared is the
hardirq (:446); rocket_reset() never touches it. In the ordinary
timeout, where no interrupt fired before the job hung, the mask is still
live. So: would you write

    rocket_pc_writel(core, INTERRUPT_MASK, 0x0);

immediately before the synchronize_irq()? It is safe to do, since
hw_submit() re-arms the mask on every submit, so nothing is lost.

I do not think this is a hole. The reset.pending check in hw_submit()
(:116, returning before next_task_idx++ at :123) already stops the
resubmit branch from firing here. It is about making the new comment
literally true. I have no TRM, so I am not claiming that a write landing
on a block under reset is harmful.

2. Should rocket_reset() hold a runtime PM reference?

This one surprised me. drm_sched_stop() zeroes the credit counter and it
is only restored in drm_sched_start(), the last line of rocket_reset().
That counter is the only thing gating suspend: rocket_job_is_idle() is
exactly atomic_read(&core->sched.credit_count) (rocket_job.c:540-543),
and rocket_device_runtime_suspend() returns -EBUSY iff that says busy,
then disables the clocks (rocket_drv.c). So for the whole body of
rocket_reset() the driver is telling runtime PM the core is idle while
holding no reference, and rocket_core_reset() is an assert, udelay(10),
deassert on top of that.

The part that touches this patch: the two puts are not equivalent.
pm_runtime_put_noidle() is a bare decrement with no idle request, so
nothing is armed. put_autosuspend() arms the 50 ms timer. Before 2/12 it
depended on who won the lock. After 2/12, if a completion interrupt was
in flight, the handler is guaranteed to finish its retire first, so
put_autosuspend() runs, the reset finds in_flight_job NULL and skips its
put_noidle, and rocket_core_reset() runs with a live timer. In the plain
hung-job case there is no handler and put_noidle still runs, so this is
only the racing case.

I have not seen it fire, and panfrost has the same
is_idle-on-credit_count shape, so this may be inherited and intended.
But would you bracket the reset with pm_runtime_get_sync() and
pm_runtime_put_autosuspend()? For contrast, panfrost_device_reset()
re-powers the block and re-initialises the MMU; rocket_core_reset()
toggles the reset lines and returns.

On the RK3576 non-recovery in your cover

You wrote that you have not worked out what sits between
rocket_core_reset() and rk_iommu re-enabling the MMU. Three things from
the source that may narrow it. I have no RK3576, so this is a direction,
not a diagnosis.

- That message is rk_iommu_force_reset(), reached only from
  rk_iommu_enable(), via attach or runtime_resume. Never from
  rocket_core_reset(). Since rocket attaches per job, the report is at
  the next job, so the damage precedes it. That matches your "a few
  seconds later".

- The check is a write-then-readback of RK_MMU_DTE_ADDR, and
  rk_iommu_enable_stall() returns 0 silently when paging is not enabled,
  which it decides by reading RK_MMU_STATUS. So a block whose registers
  read back zero produces exactly that message with nothing printed
  before it. It means the MMU is not responding at all, rather than
  being in a wrong state. Worth grepping the same run for "stall request
  timed out" and "paging request timed out"; if they are absent, that
  reading holds.

- Between the reset and the next job the core does not power cycle.
  put_noidle leaves usage_count at 0 with the device still active and no
  idle request pending. So the domain stays up, and on RK3576 the BIU
  reset your 9/12 cycles on power-on never gets cycled. If the MMU needs
  that to come back, it never gets it.

One build would settle it: put_autosuspend() instead of put_noidle() at
the end of rocket_reset(), or force a suspend and resume before the next
job, and see whether the block returns.

One question only you can answer: did MMU_DTE_ADDR appear on v7 too,
before the rail moved to domain-supply? If it did not, "existing
behaviour rather than something the series introduces" may want another
look.

What I will run, and why no tag yet

I am not putting Tested-by on this patch from an ordinary run, because
an ordinary run cannot reach the code it changes. rocket_reset()
early-returns unless reset.pending (:368), and
atomic_set(&core->reset.pending, 1) appears at exactly one site in the
driver, inside rocket_job_timedout() (:405). No debugfs knob, no sysfs
trigger, no error path. (Separately, rocket_reset_work() is never queued
anywhere, which does not affect your message but is a cleanup
candidate.) At 182 inf/s an RK3588 job is about 5.5 ms against a 500 ms
timeout, so I have never been near it.

So: a test build with JOB_TIMEOUT_MS lowered to 2, which makes healthy
jobs time out with a completion interrupt genuinely in flight, which is
the window 1/12 and 2/12 exist for. Twenty induced resets per core
across all three, with and without 1/12+2/12 for a differential, at
least one inference after each reset and one after an autosuspend and
resume, and PROVE_LOCKING plus DEBUG_ATOMIC_SLEEP on. On RK3588 core 0
sits in PD_NPUTOP, which is the parent of PD_NPU1 and PD_NPU2, so I will
do core 0 with the other two busy and core 1 on its own. Capturing
dmesg, the per-core lines in /proc/interrupts either side of each
timeout, runtime status, the genpd summary, and the output tensor
against my existing bit-exact baseline.

If all three cores come back clean, that says your non-recovery is
RK3576-specific, which is the useful answer. If RK3588 logs the same
MMU_DTE_ADDR, it is rocket-wide and pre-existing, wants its own patch,
and should not hold up v8. Either way I will report it with the lowered
timeout stated in the trailer.

Reviewed-by on 3/12 sent separately.

Regards,
Igor


On Mon, Aug 17, 2026 at 1:36 PM Jiaxing Hu <gahing at gahingwoo.com> wrote:
>
> rocket_reset() calls drm_sched_stop(), which stops the scheduler and
> returns. It does not wait for a threaded handler that is already
> running, so the comment that follows, "Remaining interrupts have been
> handled", states an assumption rather than something the code arranges.
>
> Call synchronize_irq(core->irq) after drm_sched_stop() and reword the
> comment to say what holds afterwards.
>
> It has to go before the scoped_guard(mutex, &core->job_lock) rather than
> inside it. rocket_job_handle_irq() takes job_lock, so waiting for the
> handler while holding that lock would be waiting for a handler that is
> waiting for us. Nothing is held at that point, and both callers,
> rocket_job_timedout() and rocket_reset_work(), run in process context,
> so sleeping there is allowed.
>
> This does not stop a handler that has already read in_flight_job from
> finishing its work on the job the reset is about to drop. That window
> needs the check and the register writes to be one step under the lock,
> which is what the previous patch does; the two are complementary.
>
> Suggested-by: Igor Paunovic <royalnet026 at gmail.com>
> Signed-off-by: Jiaxing Hu <gahing at gahingwoo.com>
> ---
>  drivers/accel/rocket/rocket_job.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
> index 5f0f9682e..d484a3a6f 100644
> --- a/drivers/accel/rocket/rocket_job.c
> +++ b/drivers/accel/rocket/rocket_job.c
> @@ -377,9 +377,17 @@ rocket_reset(struct rocket_core *core, struct drm_sched_job *bad)
>         drm_sched_stop(&core->sched, 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().
> +        * 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) {
>                 if (core->in_flight_job)
> --
> 2.43.0
>