rknpu: fix IOMMU domain reference handling, job teardown and IRQ-after-power-off - #388
rknpu: fix IOMMU domain reference handling, job teardown and IRQ-after-power-off#388mafischer wants to merge 5 commits into
Conversation
rknpu_irq_handler() touches the core's registers unconditionally: the no-job path writes RKNPU_OFFSET_INT_CLEAR and the normal path reads RKNPU_OFFSET_INT_STATUS. rknpu_power_off() runs from a deferred work item, so an interrupt that is late or spurious can be delivered after the block has already been powered down. The register access then takes an external abort and the machine dies instantly, with no console output and no recovery short of a power cycle: Unable to handle kernel paging request at virtual address ... pc : readl+0x4/0x20 lr : rknpu_irq_handler.isra.0+0x94/0x2f0 Call trace: readl rknpu_core0_irq_handler __handle_irq_event_percpu ... el1_interrupt / cpuidle_enter / do_idle The trace was captured over netconsole with CPU 0 idle, i.e. the NPU had finished and powered down and the interrupt landed afterwards. Bail out early when the device is not powered. power_refcount is an atomic, so unlike power_lock (a mutex) it is safe to read from hard IRQ context. The check is racy in principle but closes the window that occurs in practice, and an unpowered device cannot be asserting an interrupt, so returning IRQ_NONE cannot cause a level-triggered interrupt storm.
rknpu_job_free() drops the fence reference but never signals the fence. A job that completes normally is signalled from the completion path, but a job destroyed by rknpu_job_timeout_clean() or rknpu_job_abort() never gets there. Every waiter on such a fence therefore blocks until its own timeout expires rather than being woken when the job actually died, which defeats the purpose of waiting on the fence to detect a failed job. With CONFIG_ROCKCHIP_RKNPU_FENCE=y and a userspace consumer that waits on the fence fd, a single aborted job costs the full wait timeout instead of returning promptly. Signal the fence with -ETIMEDOUT before dropping the reference so waiters wake at once and can tell failure from completion with dma_fence_get_status(). The added dma_fence_is_signaled() test makes this a no-op on the normal completion path.
rknpu_iommu_domain_put() is a bare atomic_dec(), so an unbalanced put drives iommu_domain_refcount negative without bound. rknpu_iommu_domain_get_and_switch() decides a switch is safe using only if (atomic_read(&rknpu_dev->iommu_domain_refcount) == 0) so once the count has gone negative it never reads zero again. Every subsequent domain switch then waits the full RKNPU_SWITCH_DOMAIN_WAIT_TIME_MS and fails, and since rknpu_gem_object_create() switches domains, every allocation after that returns -EINVAL: RKNPU: switch iommu domain time out, failed to switch iommu domain, id: 1 RKNPU: rknpu_gem_object_create error Note this presents as MEM_CREATE failing with -EINVAL at essentially zero IOVA in use, which is easy to misread as memory exhaustion rather than a stuck reference count. Clamp at zero and log, so an unbalanced put is a bounded, visible anomaly instead of a device that needs a reboot. This is deliberately a safety net: the underlying imbalance is fixed separately.
rknpu_job_abort() releases the IOMMU domain reference unconditionally. The reference is acquired exactly once, in rknpu_job_commit(), but three teardown paths release it -- the completion path, rknpu_job_abort() and rknpu_job_timeout_clean() -- and nothing records whether a given job still holds one. A job that completes and is then aborted, or is aborted and then reaped, therefore releases twice. Because the count is device-wide rather than per job, a double release drives iommu_domain_refcount to zero while other cores are still executing. rknpu_iommu_domain_get_and_switch() decides a switch is safe using only if (atomic_read(&rknpu_dev->iommu_domain_refcount) == 0) so once the count reaches zero prematurely the IOMMU is reprogrammed underneath live work, and the result is a cascade that only a reboot clears: RKNPU: mismatch domain get from iommu_get_domain_for_dev RKNPU: failed to switch iommu domain, id: 1, ret: -22 RKNPU: rknpu_gem_get_pages: dma map 2097152 fail This only affects users of more than one IOMMU domain (rknpu_mem_create()'s iommu_domain_id); a single-domain workload cannot hit it. The premature zeros are silent: the count never goes negative, so an underflow check does not catch them. Instrumenting rknpu_iommu_domain_put() with __builtin_return_address(0) and reporting "reached zero while some core still has a job" separately from "went below zero" attributed 17 premature zeros in a single run to rknpu_job_abort(), against one underflow from rknpu_job_timeout_clean(). Track ownership per job and make every release a test-and-clear, so a job releases the reference at most once no matter which teardown path runs, and a second teardown of the same job is a no-op. With this applied the premature-zero count measured over the same workload drops to zero. Closes: rockchip-linux#387
rknpu_job_timeout_clean() detaches a timed-out job from its core and queues its cleanup work, but never releases the IOMMU domain reference the job acquired in rknpu_job_commit(). The completion path and rknpu_job_abort() both release it; this path does not. The reference is therefore leaked for every job reaped here, and since iommu_domain_refcount is device-wide and rknpu_iommu_domain_get_and_switch() proceeds only when it reads exactly zero, one leaked reference is enough to make every subsequent domain switch fail for the lifetime of the boot -- and with it every allocation that has to switch domains. Release it, guarded by the same per-job ownership bit the other two paths use, so a job reaped after it has already been aborted does not double release.
|
Follow-up datapoint that may be useful for review, and which sets the boundary of what this series We A/B'd this branch against our larger local rework, same board and workload, to check whether
So these five are necessary but not sufficient: they remove the reference-count wedge (no more
We work around it locally by reprogramming Happy to open that separately if it would be useful. |
|
Superseded by #390, which contains these five commits unchanged plus the change that takes Closing this one because these five fixes on their own are not sufficient for multi-domain use, The fixes here are still correct and still wanted — they are simply carried in #390 rather than |
1 participant
Five independent fixes to the
rknpudriver's IOMMU domain reference handling, job teardown andinterrupt path, found while running multi-domain workloads on RK3588 (
develop-6.1, 6.1.115).Each commit stands alone and is bisectable. The series is deliberately limited to defects in the
driver as shipped — it contains no feature work and no refactoring.
The series
do not access NPU registers in the IRQ handler after power-off —
rknpu_power_off()runs froma deferred work item, so a late or spurious interrupt takes an external abort and kills the
machine outright (trace captured over netconsole, CPU idle). Bail out when
power_refcount <= 0.signal a job's fence when it is torn down without completing — a job destroyed by
rknpu_job_timeout_clean()orrknpu_job_abort()never reaches theRKNPU_JOB_DONEpath, so itsfence is never signalled and every waiter blocks for its full timeout instead of being woken when
the job died.
clamp the IOMMU domain reference count at zero —
rknpu_iommu_domain_put()is a bareatomic_dec(). Sincerknpu_iommu_domain_get_and_switch()proceeds only when the count readsexactly zero, one unbalanced put makes it never read zero again: every later domain switch burns
its full timeout and fails, and because
rknpu_gem_object_create()switches domains, everyallocation then returns
-EINVALuntil reboot. Worth noting this presents asMEM_CREATEfailing at essentially zero IOVA in use, which is easy to misread as memory exhaustion.
release the IOMMU domain reference exactly once per job — closes rknpu: rknpu_job_abort() can zero iommu_domain_refcount while other cores have jobs in flight, allowing a domain switch under live work #387. The reference is taken
once in
rknpu_job_commit()but released from three teardown paths with nothing recording whethera given job still holds one, so a job that is completed-then-aborted (or aborted-then-reaped)
releases twice. The count is device-wide, so that drives it to zero while other cores are still
executing, and the IOMMU is then reprogrammed underneath live work. Tracked per job with a
test-and-clear so each job releases at most once.
release the domain reference held by a job reaped on timeout —
rknpu_job_timeout_clean()detaches the job and queues its cleanup but never releases its domain reference, leaking one per
reaped job. A single leak is enough to stop every subsequent domain switch for the lifetime of
the boot.
Fixes 3, 4 and 5 are related but distinct: 4 is the imbalance, 5 is a second independent leak, and 3
is the safety net that keeps either from turning into an unrecoverable device.
Scope
These only bite users of more than one IOMMU domain (
rknpu_mem_create()'siommu_domain_id); asingle-domain workload cannot hit 3/4/5. Fixes 1 and 2 are domain-independent.
How this was found
The premature zeros in #387 are silent — the count never goes negative, so an underflow check does
not catch them. Instrumenting
rknpu_iommu_domain_put()with__builtin_return_address(0)andreporting "reached zero while some core still has a job" separately from "went below zero" attributed
17 premature zeros in one run to
rknpu_job_abort(), against a single underflow fromrknpu_job_timeout_clean(). With the ownership bit applied that count goes to zero.Testing
Developed and run on RK3588 (Rock 5B) against 6.1.115, through a userspace NPU runtime that drives
multi-domain matmul workloads. In these five files the armbian 6.1 tree used for that testing is
byte-identical to
develop-6.1(rknpu_job.c,rknpu_gem.c,rknpu_reset.cdiffer by 0 lines;rknpu_iommu.cby 2), so the commits here are a direct port of code that has been built andexercised, not a re-derivation. To be precise about the claim: this exact branch has not been
separately compiled, only the equivalent code in the testing tree.
Not addressed here
rknpu_gem_object_destroy()gives up after three failed domain switches and returns withoutfreeing, leaking the object and its mapping. That is a genuine leak, but in practice it is a
consequence of the stuck reference count above, and deferring such objects for later reclaim was
measured not to help (reclaim needs a switch into the very domain that is unreachable). It is
mentioned for completeness rather than fixed speculatively.