[PATCH v12 03/14] accel/rocket: wait for a running IRQ handler before resetting a core

[Jiaxing Hu]

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: drm_sched_job_timedout()
drops job_list_lock before calling ->timedout_job(), and the only live
caller, rocket_job_timedout(), runs 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. Only a POSITIVE answer says the
device is active, and that distinction is not cosmetic: the helper tests
power.disable_depth before power.runtime_status, so -EINVAL masks a
suspended device rather than excluding one. pm_runtime_force_suspend(),
which is this driver's own system sleep callback, disables runtime PM first
and turns the clocks off second; rocket_core_fini() suspends the core and
disables before cancelling the timeout worker. Both offer -EINVAL with the
domain down, which is the SError this patch exists to avoid causing.

The mask is written with a clear of the raw status, paired the way the
completion path writes them. Masking alone leaves the DPU bit latched until
rocket_core_reset(), and the hardirq decides on raw status alone, so a
fault from the IOMMU sharing this core's line would wake the thread again
and the guarantee this patch is about would stop holding partway through
the function. 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.

Igor also ran a differential on RK3588 with this patch and the previous one
removed together, so what it shows bounds the pair rather than either one
of them. On 19 August, 45 induced resets across both arms: no
manifestation, oracle 48/48 throughout. On 25 August, the same protocol on
v9 as posted, one of five runs on the arm without the two patches returned
all 48 output channels at 0x80 from an inference that reported success,
with nothing in dmesg, in lockdep or on the serial console, out of 102
resets across nine runs.

His own bound on it is the right one: one event in 53 differential resets
against zero in 49 with the patches, timing-dependent, and his protocol
cannot tell a genuinely hung block from a lost completion. It bounds; it
does not prove.

Link: https://lore.kernel.org/all/20260819073530.6087-1-royalnet026@xxxxxxxxx/
Link: https://lore.kernel.org/all/CAEWPSH5mxTbUkNouxm6yecMZYvDowquhvYvhaXQ8HoMtHD5U1g@xxxxxxxxxxxxxx/

Suggested-by: Igor Paunovic <royalnet026@xxxxxxxxx>
Signed-off-by: Jiaxing Hu <gahing@xxxxxxxxxxxxx>
Tested-by: Igor Paunovic <royalnet026@xxxxxxxxx> # RK3588, three cores, induced reset, differential base, JOB_TIMEOUT_MS=2
---
 drivers/accel/rocket/rocket_job.c | 49 +++++++++++++++++++++++++++++--
 1 file changed, 46 insertions(+), 3 deletions(-)

diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
index 575945015..0be8db391 100644
--- a/drivers/accel/rocket/rocket_job.c
+++ b/drivers/accel/rocket/rocket_job.c
@@ -377,9 +377,52 @@ 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.
+	 *
+	 * Only a positive answer will do. pm_runtime_get_if_active() tests
+	 * power.disable_depth before power.runtime_status, so -EINVAL MASKS a
+	 * suspended device rather than excluding one: pm_runtime_force_suspend(),
+	 * which is this driver's own system suspend callback, disables runtime PM
+	 * first and turns the clocks off second, and rocket_core_fini() suspends
+	 * the core and disables before it cancels the timeout worker. Both leave
+	 * the domain down with -EINVAL on offer.
+	 *
+	 * Clear the raw status along with the mask, the way the completion path
+	 * does. Masking alone leaves the DPU bit latched until
+	 * rocket_core_reset(), and the hardirq decides on raw status alone, so a
+	 * fault from the IOMMU that shares this line would wake the thread again
+	 * and what the comment below asserts would stop being true.
+	 */
+	if (pm_runtime_get_if_active(core->dev) > 0) {
+		rocket_pc_writel(core, INTERRUPT_MASK, 0x0);
+		rocket_pc_writel(core, INTERRUPT_CLEAR, 0x1ffff);
+		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