Re: [PATCH v9 02/13] accel/rocket: wait for a running IRQ handler before resetting a core

[Igor Paunovic]

Hi Jiaxing,

As promised, the 19 August protocol re-run on v9, as-is.

Setup, identical to the run reported against v8 1-2/12: same
board (RK3588, Orange Pi 5 Plus), same personal 7.2-rc6 tree,
same config (PROVE_LOCKING=y, DEBUG_ATOMIC_SLEEP=y), all three
cores bound with scheduler-driven placement, and the same local
test-only patch lowering JOB_TIMEOUT_MS to 2 ms. The serial
console was captured on a second machine for the whole session.
Two passes per kernel, at console_loglevel 8 and 4. The only
difference from 19 August: the two rocket changes on top of the
base are now v9 1/13 and 2/13 instead of the v8 pair. Both
applied cleanly.

v9 arm: two passes, 12 and 11 induced "NPU job timed out"
resets. Every reset recovered, every inference matched the CPU
oracle within 1 (48/48), including the one issued after a
forced autosuspend and resume. No MMU faults, no raw reset
messages, no lockdep hits, nothing on the serial console.

Differential arm, fresh in the same session (same base with the
two patches removed, same config, same timeout): five runs -
the protocol pair at loglevel 8 and 4, plus three extra runs at
loglevel 8 to gauge repeatability. Reset counts 8/10/12/8/15,
all recovered. Four runs clean. One of the extra runs came back
with something I had not seen before: the inference after the
forced autosuspend "succeeded" but returned a constant buffer -
all 48 output channels uniformly 128 (0x80), which is not the
output zero point of this model, while the CPU reference varies
normally. Zero kernel messages, zero lockdep hits, nothing on
the serial console. A job that signals completion while its
output buffer is never written is exactly the silent flavour of
the race these two patches close, and in 102 induced resets
across nine runs today it appeared only on the arm that does
not carry them. Full artifacts are preserved (scorer output,
runtime and genpd state, journal, serial log) if anyone wants
them.

Before testing I also compared the tagged patches against their
v8 counterparts: 1/13 is byte-identical to v8 1/12 up to the
base-commit trailer, and 4/13 is identical to v8 3/12, so the
tags they carry describe exactly what was tested here.

For this patch:

Tested-by: Igor Paunovic <royalnet026@xxxxxxxxx> # RK3588, three
cores, induced reset, differential base, JOB_TIMEOUT_MS=2

Regards,
Igor

On Mon, Aug 24, 2026 at 1:09 PM Jiaxing Hu <gahing@xxxxxxxxxxxxx> 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.
>
> Mask the block before the sync as well. INTERRUPT_MASK is armed by
> hw_submit() on every submit and cleared only by the hardirq, so on an
> ordinary timeout it is still live and a completion can arrive after
> synchronize_irq() returns. Nothing is lost by clearing it, since the next
> submit arms it again.
>
> That write is the first register access this function has ever made, and
> it is guarded, because the function holds no runtime PM reference of its
> own. The only reference in the window belongs to in_flight_job, and the
> completion path can have put it and cleared the pointer before the
> timeout worker arrives: drm_sched_stop() sits in between and can block on
> cancel_work_sync() and on a dma_fence_wait(), and it subtracts every
> pending job's credits, so rocket_job_is_idle() is true and
> rocket_device_runtime_suspend() will not refuse. With the autosuspend
> delay elapsed the clocks are off and both NPU domains are down. A
> register access in that state takes an async SError on this hardware,
> which is the failure two later patches in this series describe from the
> power-on side.
>
> pm_runtime_get_if_active() resumes nothing and allocates nothing; if the
> core is already down there is no live interrupt to mask and the following
> synchronize_irq() is all that is needed. Igor Paunovic asked the general
> form of this on v8 -- whether rocket_reset() should hold a reference --
> and it was deferred then because nothing in the path touched a register.
> This patch is what makes it matter.
>
> The deadlock this placement avoids would not have been reported. The wait
> is on desc->wait_for_threads rather than on a lock, so lockdep does not
> model it and it would have hung silently.
>
> Suggested-by: Igor Paunovic <royalnet026@xxxxxxxxx>
> Signed-off-by: Jiaxing Hu <gahing@xxxxxxxxxxxxx>
> ---
>  drivers/accel/rocket/rocket_job.c | 34 ++++++++++++++++++++++++++++---
>  1 file changed, 31 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
> index 5f0f9682e..3c0ed4605 100644
> --- a/drivers/accel/rocket/rocket_job.c
> +++ b/drivers/accel/rocket/rocket_job.c
> @@ -377,9 +377,37 @@ 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().
> +        * Mask the block before waiting. hw_submit() arms INTERRUPT_MASK on
> +        * every submit and only the hardirq clears it, so on an ordinary
> +        * timeout it is still live and a completion can arrive after the sync
> +        * returns. The next submit re-arms it, so nothing is lost here.
> +        *
> +        * Only when the device is already awake, though. This function holds no
> +        * runtime PM reference of its own: the only one in the window belongs to
> +        * in_flight_job, and the completion path may have put it and cleared the
> +        * pointer before the timeout worker got here. drm_sched_stop() above can
> +        * block for a long time, and it drops every pending job's credits, so
> +        * rocket_job_is_idle() is true and nothing keeps the core resumed. On
> +        * this hardware a register access with the domain down takes an async
> +        * SError, so a reset must not be the thing that causes one.
> +        */
> +       if (pm_runtime_get_if_active(core->dev) > 0) {
> +               rocket_pc_writel(core, INTERRUPT_MASK, 0x0);
> +               pm_runtime_put_autosuspend(core->dev);
> +       }
> +
> +       /*
> +        * 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
>