Re: [RFC PATCH v5 v5 6/8] accel/rocket: add RK3576 NPU (RKNN) support

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:

New issues:
- [High] Hardware reset triggered while polling timer and workqueue are still active.
- [High] Unclocked MMIO writes in `rocket_poll_work_fn` due to stale sequence numbers.
- [High] Bypassing job completion in `rocket_poll_work_fn` during teardown leads to deadlocks and resource leaks.
- [High] Concurrent execution of `rocket_poll_work_fn` and `rocket_job_handle_irq` corrupts hardware state and skips tasks.
- [High] Late hardware interrupts re-arm the polling timer after teardown, causing a Use-After-Free.

Pre-existing issues:
- [High] Unclocked MMIO access in shared IRQ handler (`rocket_job_irq_handler`).
- [High] Reset workqueue can be used after destruction due to late IRQ firing.
- [High] IOMMU group reference leak on every job completion.
- [High] DRM device is registered and exposed to userspace before core hardware and schedulers are initialized.
- [High] Unbinding a non-terminal core corrupts `rdev->num_cores` and leads to state overwrite.
--

--- Patch [6]: [RFC PATCH v5 v5 6/8] accel/rocket: add RK3576 NPU (RKNN) support ---
commit 49430ea8bb109098d159ff322aab30ccbfc4c20a
Author: Jiaxing Hu <[email protected]>

accel/rocket: add RK3576 NPU (RKNN) support

The RK3576 carries the same RKNN block as the RK3588, with two cores
instead of three and a few platform differences: The patch adds RK3576
support, managing its separate clock domains, power domains, and polling
the completion interrupt via an hrtimer.

> diff --git a/drivers/accel/rocket/rocket_device.c b/drivers/accel/rocket/rocket_device.c
> --- a/drivers/accel/rocket/rocket_device.c
> +++ b/drivers/accel/rocket/rocket_device.c
> @@ -31,6 +31,10 @@ struct rocket_device *rocket_device_init(struct platform_device *pdev,

[ ... ]

>  	err = drm_dev_register(ddev, 0);
>  	if (err)
>  		return ERR_PTR(err);
>  
>  	return rdev;
>  }

[Severity: High]
This is a pre-existing issue, but does rocket_device_init() expose the
DRM device to userspace before the hardware is actually ready?

It looks like drm_dev_register() is called here, exposing the device,
but rocket_probe() only initializes the core hardware and schedulers via
rocket_core_init() afterward. Could userspace race to open the device and
submit jobs before the schedulers and memory maps are initialized?

> diff --git a/drivers/accel/rocket/rocket_drv.c b/drivers/accel/rocket/rocket_drv.c
> --- a/drivers/accel/rocket/rocket_drv.c
> +++ b/drivers/accel/rocket/rocket_drv.c
> @@ -176,6 +176,7 @@ static int rocket_probe(struct platform_device *pdev)
>  
>  	rdev->cores[core].rdev = rdev;
>  	rdev->cores[core].dev = &pdev->dev;
> +	rdev->cores[core].soc = of_device_get_match_data(&pdev->dev);
>  	rdev->cores[core].index = core;
>  
>  	rdev->num_cores++;

[Severity: High]
This is a pre-existing issue, but does the core removal logic in
rocket_remove() corrupt the device state if a non-terminal core is unbound?

If rocket_remove() is called for core 0 on a 2-core setup, it blindly
decrements rdev->num_cores to 1. If another core is then bound,
rocket_probe() will use rdev->num_cores (1) as the index, overwriting the
active rdev->cores[1] structure currently in use.

> diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
> --- a/drivers/accel/rocket/rocket_job.c
> +++ b/drivers/accel/rocket/rocket_job.c

[ ... ]

> +static enum hrtimer_restart rocket_poll_timer_fn(struct hrtimer *timer)
> +{
> +	struct rocket_core *core = container_of(timer, struct rocket_core, poll_timer);
> +	u32 raw;
> +
> +	if (!atomic_read(&core->poll_active))
> +		return HRTIMER_NORESTART;
> +
> +	core->poll_work_seq = core->poll_seq;
> +
> +	raw = rocket_pc_readl(core, INTERRUPT_RAW_STATUS);

[Severity: High]
Can the hardware reset logic race with this polling timer and workqueue?

When a job times out, the DRM scheduler calls rocket_reset(), which
triggers rocket_core_reset() without first stopping the active poll timer
or work. If the timer fires or the work executes concurrently, will they
attempt to read or write MMIO registers on a device undergoing reset or
without active clocks?

> +	if ((raw & (PC_INTERRUPT_RAW_STATUS_DPU_0 | PC_INTERRUPT_RAW_STATUS_DPU_1)) ||
> +	    ++core->poll_ticks >= RK3576_POLL_MAX_TICKS) {
> +		atomic_set(&core->poll_active, 0);
> +		schedule_work(&core->poll_work);
> +		return HRTIMER_NORESTART;
> +	}
> +
> +	hrtimer_forward_now(timer, ns_to_ktime(RK3576_POLL_INTERVAL_NS));
> +	return HRTIMER_RESTART;
> +}
> +
> +/* Start the job's next task, or retire it. Caller holds job_lock. */
> +static void rocket_job_next_locked(struct rocket_core *core)
> +{
> +	lockdep_assert_held(&core->job_lock);
> +
> +	if (!core->in_flight_job)
> +		return;
> +
> +	if (core->in_flight_job->next_task_idx < core->in_flight_job->task_count) {
> +		rocket_job_hw_submit(core, core->in_flight_job);
> +		return;
> +	}
> +
> +	iommu_detach_group(NULL, iommu_group_get(core->dev));

[Severity: High]
This is a pre-existing issue, but does calling iommu_group_get() here leak 
the IOMMU group reference?

It appears iommu_group_get() acquires a reference that is passed to
iommu_detach_group(), but iommu_detach_group() does not consume the
reference, and there is no matching iommu_group_put(). Will this cause the
reference count to overflow or permanently pin IOMMU resources?

> +	dma_fence_signal(core->in_flight_job->done_fence);
> +	pm_runtime_put_autosuspend(core->dev);
> +	core->in_flight_job = NULL;
> +}
> +
> +static void rocket_poll_work_fn(struct work_struct *work)
> +{
> +	struct rocket_core *core = container_of(work, struct rocket_core, poll_work);
> +
> +	pm_runtime_mark_last_busy(core->dev);
> +
> +	scoped_guard(mutex, &core->job_lock) {
> +		/*
> +		 * The interrupt can land while this work is queued, retire the job
> +		 * and start the next task. poll_seq only moves under job_lock, in
> +		 * hw_submit, so comparing it here says whether that happened. Doing
> +		 * it outside the lock would leave the window open rather than close
> +		 * it, and this work would then submit a task on top of a live one.
> +		 */
> +		if (READ_ONCE(core->poll_dying) || core->poll_work_seq != core->poll_seq)
> +			return;

[Severity: High]
Does returning early here on poll_dying cause a resource leak and deadlock?

If poll_work_fn() returns early without calling rocket_job_next_locked(), it
looks like the done_fence of the currently executing job is never signaled,
the PM runtime reference is never released, and the IOMMU group is never
detached.

[Severity: High]
Can this sequence check incorrectly pass if the last task of a job completes
via a shared IRQ?

If rocket_job_handle_irq() finishes the last task, it drops the PM runtime
reference and clears in_flight_job. However, because rocket_job_hw_submit()
isn't called, poll_seq is not updated.

When this work function subsequently runs, it passes the sequence check
(poll_work_seq == poll_seq) and proceeds to write to hardware. Since the
PM reference was already dropped, could this cause a fatal unclocked MMIO
access?

> +
> +		rocket_pc_writel(core, OPERATION_ENABLE, 0x0);
> +		rocket_pc_writel(core, INTERRUPT_CLEAR, 0x1ffff);
> +
> +		rocket_job_next_locked(core);
> +	}
> +}
> +
>  static void rocket_job_handle_irq(struct rocket_core *core)
>  {
> +	if (core->soc->poll_completion) {
> +		atomic_set(&core->poll_active, 0);
> +		hrtimer_cancel(&core->poll_timer);
> +	}
> +
>  	pm_runtime_mark_last_busy(core->dev);
>  
>  	rocket_pc_writel(core, OPERATION_ENABLE, 0x0);

[Severity: High]
Can this hardware write outside the job_lock race with task submission?

The rocket_job_handle_irq() function writes 0 to OPERATION_ENABLE before
acquiring the job_lock. If poll_work_fn() is concurrently inside the lock
submitting the next task (which sets OPERATION_ENABLE to 1), the IRQ handler
can immediately overwrite it to 0, disabling the new task.

The IRQ handler would then acquire job_lock and call rocket_job_next_locked(),
which retires the newly started task without it ever executing.

>  	rocket_pc_writel(core, INTERRUPT_CLEAR, 0x1ffff);
>  
>  	scoped_guard(mutex, &core->job_lock)
> -		if (core->in_flight_job) {
> -			if (core->in_flight_job->next_task_idx < core->in_flight_job->task_count) {
> -				rocket_job_hw_submit(core, core->in_flight_job);
> -				return;
> -			}
> -
> -			iommu_detach_group(NULL, iommu_group_get(core->dev));
> -			dma_fence_signal(core->in_flight_job->done_fence);
> -			pm_runtime_put_autosuspend(core->dev);
> -			core->in_flight_job = NULL;
> -		}
> +		rocket_job_next_locked(core);
>  }

[ ... ]

> @@ -460,6 +540,10 @@ int rocket_job_init(struct rocket_core *core)
>  	int ret;
>  
>  	INIT_WORK(&core->reset.work, rocket_reset_work);
> +	INIT_WORK(&core->poll_work, rocket_poll_work_fn);
> +	hrtimer_setup(&core->poll_timer, rocket_poll_timer_fn, CLOCK_MONOTONIC,
> +		      HRTIMER_MODE_REL);
> +	atomic_set(&core->poll_active, 0);
>  	spin_lock_init(&core->fence_lock);
>  	mutex_init(&core->job_lock);
>  
> @@ -501,8 +585,23 @@ int rocket_job_init(struct rocket_core *core)

[Severity: High]
This is a pre-existing issue, but does the shared IRQ handler perform
unclocked MMIO access?

The rocket_job_irq_handler (which calls rocket_job_handle_irq) is registered
with IRQF_SHARED. If it reads INTERRUPT_RAW_STATUS immediately without
checking if the device's power domain and clocks are active, a shared
interrupt firing while the NPU is runtime suspended could cause a
synchronous external abort.

>  void rocket_job_fini(struct rocket_core *core)
>  {
> +	/*
> +	 * Stop the poll from starting hardware work before tearing anything
> +	 * down: it submits the next task, and drm_sched_fini() does not wait
> +	 * for work already queued. Cancel after the scheduler is gone, so a
> +	 * job running now cannot re-arm the timer behind the cancel.
> +	 */
> +	if (core->soc->poll_completion)
> +		WRITE_ONCE(core->poll_dying, true);
> +
>  	drm_sched_fini(&core->sched);
>  
> +	if (core->soc->poll_completion) {
> +		atomic_set(&core->poll_active, 0);
> +		hrtimer_cancel(&core->poll_timer);
> +		cancel_work_sync(&core->poll_work);
> +	}

[Severity: High]
Can a late hardware interrupt re-arm the polling timer after teardown?

While hrtimer_cancel() is called here, the hardware IRQ is not disabled.
If an interrupt fires after this point, rocket_job_handle_irq() will execute,
progress the job, and call rocket_job_hw_submit(), which re-arms the timer
via hrtimer_start().

Once the rocket_core structure is freed, could the armed timer fire and
access freed memory?

> +
>  	cancel_work_sync(&core->reset.work);
>  	destroy_workqueue(core->reset.wq);
>  }

[Severity: High]
This is a pre-existing issue, but can the reset workqueue be used after it
is destroyed?

Because the shared IRQ remains active until devres cleans it up after
rocket_remove(), a late interrupt hitting the MAC interrupt condition could
call queue_work() on this destroyed workqueue, leading to a kernel panic.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260805063826.95682-1-gahing-SYz5u2A9JnigSpxsJD1C4w@public.gmane.org?part=6