marfrit/rkopnu
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

rkopnu: harden IOMMU domain switch against shared-NIU PM race; fix false-success on submit timeout

Browse Source 
NOT YET LIVE-VALIDATED. Written and built after the 30B/multi-domain
prefill hang reported against b831a2d ("honor vendor iommu_domain_id"),
from source analysis only (no live reboot access at patch time) --
needs a coordinated reboot-test before it earns "fixes the hang" in
the commit log. Recording the reasoning here so the next person (or
session) doesn't have to re-derive it.

What's confirmed by source reading, not speculation:
 - rkopnu_ioctl_submit's own dma_fence_wait_timeout(..., 5000ms) is
   correctly bounded (dma-fence.c: dma_fence_default_wait() uses a
   real schedule_timeout(), always returns). It is NOT the permanent
   hang -- but on timeout it fell through to unconditionally report
   *success* (args->task_counter = args->task_number; ret = 0)
   regardless of whether the job ever actually finished. Fixed: now
   returns -ETIMEDOUT (or the fence's own error) and leaves the job's
   own scheduler-held kref alone (drm_sched_job_cleanup() is only safe
   for jobs that never got pushed; this one did).
 - drm_sched_stop()'s unbounded dma_fence_wait() (sched_main.c, for
   jobs whose HW-fence callback removal races) is unreachable here:
   credit_limit=1 per core means at most one job is ever pending, so
   that loop never has a second job to wait on.
 - rockchip-iommu.c's attach/detach path (rk_iommu_enable/disable,
   force_reset, stall) is bounded throughout via readx_poll_timeout()
   with real timeout constants -- no raw unbounded register poll found
   there. iommu_attach_group() failure is already handled correctly by
   rocket_job_run() (signals the fence with the error immediately), so
   a bounded HW-level attach timeout was already not a hang source.
 - rocket_device_pm_get()/put() (rocket_device.c) serialize ALL 3
   cores' power-domain resume/release through one device-global
   rdev->pm_lock, specifically because (per that file's own comments)
   a sibling core's independent clock/power-domain gating can race
   another core's in-flight DMA on the shared NIU/PMU. That lock is
   NOT held around the IOMMU domain attach/detach in rocket_job_run()
   or rocket_reset() -- and rk_iommu_attach_device()/rk_iommu_disable()
   do their own clk_bulk_enable()/disable() on the IOMMU submodule's
   clocks, sharing the same upstream PMU/clock-tree infrastructure.
   This is the same hazard class already fixed here for power-domain
   gating, just not yet extended to IOMMU domain switching -- and the
   30B case is exactly the scenario that exercises it hard (frequent
   cross-domain MoE expert routing across 3 concurrently-segmenting
   cores, each independently deciding to attach/detach at any moment).

Fix: take rdev->pm_lock around the iommu_detach_group()/
iommu_attach_group() calls in both rocket_job_run() and rocket_reset(),
serializing domain switches against power-unit transitions (and against
each other, device-wide) the same way power-domain gating already is.
Safe to acquire: rocket_device_pm_get()/put_noidle() immediately above
each site already acquire-and-release pm_lock without holding it across
their own return, so this is a fresh, non-recursive acquisition. Lock
order is always job_lock -> pm_lock in both call sites, never the
reverse, so no new AB-BA risk.

This does not explain (and does not fix, if true) a genuine hardware-
level unbounded hang -- I found no evidence of one in the code I could
read, which is what makes the PM/clock-race theory the leading
candidate. If the 30B prefill still hangs after this, the next step is
live /proc/PID/stack (needs lockdown adjusted or a debug kernel) or
ftrace on iommu_attach_group/rk_iommu_enable/pm_runtime_resume_and_get
to catch the actual stuck frame.

Builds clean (make -j8, vermagic 7.0.0-rc3-npuclk+ matches the running
kernel), checkpatch --strict clean (only pre-existing false-positive
blank-context-line whitespace hits, and the pre-existing missing-
prototype warning for rkopnu_ioctl_submit -- both present on
unmodified rocket_job.c too, verified by diffing a clean checkout).
Not yet inserted into the running kernel (never rmmod rkopnu -- reboot
required to swap modules).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EWpfhDgYNA21tETDP9ueBE
This commit is contained in:
Markus Fritsche
2026-07-20 05:18:55 +02:00
parent b831a2d931
commit a7ace5da8e
1 changed files with 65 additions and 6 deletions
Download Patch File Download Diff File Expand all files Collapse all files
rocket_job.c
+65 -6
View File
@@ -356,14 +356,41 @@ static struct dma_fence *rocket_job_run(struct drm_sched_job *sched_job)
* Re-attach only on a domain change. The attach/detach of attached_domain
* is done under job_lock so it is serialized against the detach in
* rocket_job_close() and rocket_reset() (Fable review).
*
* rkopnu multi-domain deadlock hardening: also serialize the actual
* iommu_detach_group()/iommu_attach_group() calls against rdev->pm_lock
* (the SAME lock rocket_device_pm_get()/put() above already use to
* serialize power-unit resume/release across all 3 cores as a unit).
* rk_iommu_attach_device()/rk_iommu_disable() (rockchip-iommu.c) do their
* own clk_bulk_enable()/clk_bulk_disable() on the IOMMU submodule's
* clocks, which share upstream PMU/clock-tree infrastructure with the
* NPU cores' own power-domain resume -- the exact hazard class this file
* already documents and handles for cross-core clock/power-domain gating
* ("a sibling core's independent gating can race this core's in-flight
* DMA traffic", rocket_device_pm_get() above). Without this, a domain
* switch on one core's IOMMU submodule could race a power-unit
* transition happening concurrently for another core, both touching the
* shared NIU/PMU. Suspected root cause of the 30B/multi-domain prefill
* hang seen in testing (D-state on dma_fence_default_wait, no TDR
* recovery) -- not confirmed by live tracing (lockdown blocked
* /proc/PID/stack), so this is a principled hardening measure pending
* validation on the next reboot-test, not a proven fix.
*
* pm_lock is safe to take here: rocket_device_pm_get() above already
* acquired and released it (it never holds pm_lock across its own
* return), so this is a fresh, non-recursive acquisition. Lock order is
* always job_lock -> pm_lock in this file (see rocket_reset() below,
* updated the same way) -- never the reverse -- so no new AB-BA risk.
*/
scoped_guard(mutex, &core->job_lock) {
if (core->attached_domain != job->domain) {
if (core->attached_domain)
iommu_detach_group(NULL, core->iommu_group);
attach_err = iommu_attach_group(job->domain->domain,
core->iommu_group);
scoped_guard(mutex, &rdev->pm_lock) {
if (core->attached_domain)
iommu_detach_group(NULL, core->iommu_group);
attach_err = iommu_attach_group(job->domain->domain,
core->iommu_group);
}
core->attached_domain = attach_err ? NULL : job->domain;
}
if (!attach_err) {
@@ -432,8 +459,15 @@ rocket_reset(struct rocket_core *core, struct drm_sched_job *bad)
if (core->in_flight_job)
rocket_device_pm_put_noidle(core->rdev);
/* rkopnu multi-domain deadlock hardening: same rdev->pm_lock
* serialization as rocket_job_run() above, and for the same
* reason -- this detach must not race a power-unit transition
* on the shared NIU/PMU either. rocket_device_pm_put_noidle()
* just above already released pm_lock before returning, so
* this is a fresh acquisition, not held across the call above. */
if (core->attached_domain) {
iommu_detach_group(NULL, core->iommu_group);
scoped_guard(mutex, &core->rdev->pm_lock)
iommu_detach_group(NULL, core->iommu_group);
core->attached_domain = NULL;
}
@@ -889,8 +923,33 @@ int rkopnu_ioctl_submit(struct drm_device *dev, void *data, struct drm_file *fil
if (rjob->inference_done_fence) {
long tout = dma_fence_wait_timeout(rjob->inference_done_fence,
false, msecs_to_jiffies(5000));
if (tout <= 0)
if (tout <= 0) {
/*
* Pre-existing bug (not introduced by multi-domain): this
* branch used to log a warning and then fall through to
* "args->task_counter = args->task_number; ret = 0;"
* regardless -- i.e. reported SUCCESS to userspace even
* though the job never actually finished (or we couldn't
* prove it did). That silently hands back stale/garbage
* output instead of an error librknnrt/ggml-rknpu2 could
* detect and propagate. Report the real outcome instead:
* -ETIMEDOUT if our bound simply expired, or dma_fence's
* own negative error code if it has one (e.g. -ECANCELED
* from a TDR-driven reset abandoning the job).
*
* Do NOT drm_sched_job_cleanup() here (that's what
* out_cleanup below is for): the job was successfully
* pushed onto the scheduler above, so it is still live
* there (or being unwound by TDR) -- the scheduler holds
* its own kref (see rocket_job_push()) and will properly
* finish/free it via the normal completion or
* timedout_job() path. Just drop our own reference.
*/
drm_warn(dev, "rkopnu: submit wait unfinished (%ld)\n", tout);
ret = tout == 0 ? -ETIMEDOUT : (int)tout;
args->task_counter = 0;
goto out_put_job;
}
}
args->task_counter = args->task_number;