From 3af228c8d5f7bd2cefc53572828849e6163a4aa1 Mon Sep 17 00:00:00 2001 From: Michael Fischer Date: Thu, 27 Aug 2026 20:25:27 -0500 Subject: [PATCH 1/5] rknpu: do not access NPU registers in the IRQ handler after power-off 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. --- drivers/rknpu/rknpu_job.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/drivers/rknpu/rknpu_job.c b/drivers/rknpu/rknpu_job.c index 57d67d81f7d5e..7a4aee9f12f5f 100644 --- a/drivers/rknpu/rknpu_job.c +++ b/drivers/rknpu/rknpu_job.c @@ -646,6 +646,27 @@ static inline irqreturn_t rknpu_irq_handler(int irq, void *data, int core_index) uint32_t status = 0; unsigned long flags; + /* + * Never touch NPU registers while the block is powered down. + * + * Both paths below access registers unconditionally: the no-job path writes + * RKNPU_OFFSET_INT_CLEAR and the normal path reads RKNPU_OFFSET_INT_STATUS. + * rknpu_power_off() is driven by a deferred work item, so a late or spurious + * interrupt can arrive after power has gone. The register access then takes an + * external abort and the machine dies immediately, with no console output and + * no way back but a power cycle: + * + * pc : readl+0x4/0x20 + * lr : rknpu_irq_handler.isra.0+0x94/0x2f0 + * Call trace: readl / rknpu_core0_irq_handler / __handle_irq_event_percpu + * + * power_refcount is an atomic, so unlike power_lock (a mutex) it is safe to read + * from hard IRQ context. If the device is unpowered it cannot be asserting an + * interrupt, so IRQ_NONE is correct and cannot cause a level-IRQ storm. + */ + if (atomic_read(&rknpu_dev->power_refcount) <= 0) + return IRQ_NONE; + subcore_data = &rknpu_dev->subcore_datas[core_index]; spin_lock_irqsave(&rknpu_dev->irq_lock, flags); From aedcff95c87bc5af8dd1948ad1513342646d1701 Mon Sep 17 00:00:00 2001 From: Michael Fischer Date: Thu, 27 Aug 2026 20:25:42 -0500 Subject: [PATCH 2/5] rknpu: signal a job's fence when it is torn down without completing 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. --- drivers/rknpu/rknpu_job.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/drivers/rknpu/rknpu_job.c b/drivers/rknpu/rknpu_job.c index 7a4aee9f12f5f..edc383d1532e0 100644 --- a/drivers/rknpu/rknpu_job.c +++ b/drivers/rknpu/rknpu_job.c @@ -101,8 +101,23 @@ static void rknpu_job_free(struct rknpu_job *job) rknpu_gem_object_put(&task_obj->base); #endif - if (job->fence) + if (job->fence) { + /* + * A job torn down without completing (rknpu_job_timeout_clean() or + * rknpu_job_abort()) never reaches the RKNPU_JOB_DONE path, so its fence + * is never signalled and every waiter blocks until its own timeout + * expires -- which defeats the point of waiting on a fence to notice + * that a job has failed. Signal it with an error instead, so waiters + * wake immediately and can distinguish failure from completion via + * dma_fence_get_status(). No-op on the success path, where the + * completion interrupt has already signalled it. + */ + if (!dma_fence_is_signaled(job->fence)) { + dma_fence_set_error(job->fence, -ETIMEDOUT); + dma_fence_signal(job->fence); + } dma_fence_put(job->fence); + } if (job->args_owner) kfree(job->args); From f9d560fe4919062e98a46d1d5bd7fec0c043755c Mon Sep 17 00:00:00 2001 From: Michael Fischer Date: Thu, 27 Aug 2026 20:26:00 -0500 Subject: [PATCH 3/5] rknpu: clamp the IOMMU domain reference count at zero 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. --- drivers/rknpu/rknpu_iommu.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/drivers/rknpu/rknpu_iommu.c b/drivers/rknpu/rknpu_iommu.c index a3efb1ef02734..e9e9f3652d302 100644 --- a/drivers/rknpu/rknpu_iommu.c +++ b/drivers/rknpu/rknpu_iommu.c @@ -560,7 +560,25 @@ int rknpu_iommu_domain_get_and_switch(struct rknpu_device *rknpu_dev, int rknpu_iommu_domain_put(struct rknpu_device *rknpu_dev) { - atomic_dec(&rknpu_dev->iommu_domain_refcount); + /* + * Never let the reference count go negative. + * + * rknpu_iommu_domain_get_and_switch() proceeds only when this reads exactly + * zero, so a bare atomic_dec() turns a single unbalanced put into a permanent + * wedge: the count never reads zero again, every domain switch burns its full + * RKNPU_SWITCH_DOMAIN_WAIT_TIME_MS and fails, and because + * rknpu_gem_object_create() switches domains, every subsequent allocation then + * fails with -EINVAL until the machine is rebooted. + * + * Clamping turns an over-put into a bounded anomaly instead of a dead device. + * It treats the symptom, not the cause, so warn once per occurrence to keep any + * remaining imbalance visible. + */ + if (atomic_dec_return(&rknpu_dev->iommu_domain_refcount) < 0) { + atomic_set(&rknpu_dev->iommu_domain_refcount, 0); + LOG_DEV_ERROR(rknpu_dev->dev, + "iommu domain refcount underflow, clamped to 0\n"); + } return 0; } From b3c37c1ab82aee1f0af9ccc910b0fb528012fbff Mon Sep 17 00:00:00 2001 From: Michael Fischer Date: Thu, 27 Aug 2026 20:26:37 -0500 Subject: [PATCH 4/5] rknpu: release the IOMMU domain reference exactly once per job 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: https://github.com/rockchip-linux/kernel/issues/387 --- drivers/rknpu/include/rknpu_job.h | 11 +++++++++++ drivers/rknpu/rknpu_job.c | 7 +++++-- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/drivers/rknpu/include/rknpu_job.h b/drivers/rknpu/include/rknpu_job.h index b4d40d96ffce4..86acb0540bf7e 100644 --- a/drivers/rknpu/include/rknpu_job.h +++ b/drivers/rknpu/include/rknpu_job.h @@ -32,6 +32,17 @@ struct rknpu_job { struct work_struct cleanup_work; bool irq_entry[RKNPU_MAX_CORES]; unsigned int flags; + /* + * Does this job currently hold the IOMMU domain reference? + * + * 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() -- with nothing recording whether this particular + * job still holds one. Its own word rather than a bit in ->flags because the + * releases run from both interrupt and process context, so the test-and-clear + * has to be atomic. + */ + unsigned long dom_held; int ret; struct rknpu_submit *args; bool args_owner; diff --git a/drivers/rknpu/rknpu_job.c b/drivers/rknpu/rknpu_job.c index edc383d1532e0..7a981a839413b 100644 --- a/drivers/rknpu/rknpu_job.c +++ b/drivers/rknpu/rknpu_job.c @@ -500,7 +500,8 @@ static void rknpu_job_done(struct rknpu_job *job, int ret, int core_index) if (atomic_dec_and_test(&job->interrupt_count)) { int use_core_num = job->use_core_num; - rknpu_iommu_domain_put(rknpu_dev); + if (test_and_clear_bit(0, &job->dom_held)) + rknpu_iommu_domain_put(rknpu_dev); job->flags |= RKNPU_JOB_DONE; job->ret = ret; @@ -556,6 +557,7 @@ static void rknpu_job_schedule(struct rknpu_job *job) job->ret = -EINVAL; return; } + set_bit(0, &job->dom_held); spin_lock_irqsave(&rknpu_dev->irq_lock, flags); for (i = 0; i < rknpu_dev->config->num_irqs; i++) { @@ -580,7 +582,8 @@ static void rknpu_job_abort(struct rknpu_job *job) unsigned long flags; int i = 0; - rknpu_iommu_domain_put(rknpu_dev); + if (test_and_clear_bit(0, &job->dom_held)) + rknpu_iommu_domain_put(rknpu_dev); msleep(100); From ccee542566b1ef98181d0e83bc557fdaf41fd2fc Mon Sep 17 00:00:00 2001 From: Michael Fischer Date: Thu, 27 Aug 2026 20:27:13 -0500 Subject: [PATCH 5/5] rknpu: release the domain reference held by a job reaped on timeout 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. --- drivers/rknpu/rknpu_job.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/drivers/rknpu/rknpu_job.c b/drivers/rknpu/rknpu_job.c index 7a981a839413b..fc0d245802bc2 100644 --- a/drivers/rknpu/rknpu_job.c +++ b/drivers/rknpu/rknpu_job.c @@ -757,6 +757,17 @@ static void rknpu_job_timeout_clean(struct rknpu_device *rknpu_dev, spin_unlock_irqrestore(&rknpu_dev->irq_lock, flags); + /* + * Release the domain reference this job still + * holds. The completion and abort paths both do + * this, but reaping a timed-out job here did + * not, so the reference was leaked and the + * device-wide count never returned to zero -- + * after which no domain switch can ever succeed. + */ + if (test_and_clear_bit(0, &job->dom_held)) + rknpu_iommu_domain_put(rknpu_dev); + do { schedule_work(&job->cleanup_work);