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

[sashiko-bot@kernel.org]

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

New issues:
- [High] Resource leak on `devm_pm_domain_attach_list` failure.
- [High] Teardown order race in `rocket_job_fini` allows the hrtimer to be re-armed after cancellation.
- [High] Race condition and hardware state corruption on RK3576 DMA errors due to double-processing and unsynchronized MMIO writes.

Pre-existing issues:
- [High] Unconditional MMIO access in shared IRQ handler causes bus faults while device is suspended.
- [High] Leaked IOMMU group reference on job completion.
- [High] The runtime suspend callback powers down the device without synchronizing the shared IRQ.
- [High] Calling `drm_sched_fini` on a partially initialized scheduler causes a kernel panic.
--

--- Patch [6]: [RFC PATCH v2 6/8] accel/rocket: add RK3576 NPU (RKNN) support ---
commit 3624dfd3291e5171660b2a505645f90fa60606b8
Author: Jiaxing Hu <gahing@gahingwoo.com>

accel/rocket: add RK3576 NPU (RKNN) support

This commit adds RK3576 support to the rocket DRM accelerator driver. It
introduces new clocks, power domains, and a polled completion mechanism
since the RK3576 has no maskable completion interrupt.

> diff --git a/drivers/accel/rocket/rocket_core.c b/drivers/accel/rocket/rocket_core.c
> index b3b2fa9ba645a..140e37969a7b1 100644
> --- a/drivers/accel/rocket/rocket_core.c
> +++ b/drivers/accel/rocket/rocket_core.c
> @@ -65,6 +83,23 @@ int rocket_core_init(struct rocket_core *core)
>  		return err;
>  	}
>  
> +	/*
> +	 * RK3576: the NPU spans TWO power domains (PD_NPU0 + PD_NPU1).
> [ ... ]
> +	if (core->soc->multi_power_domain) {
> +		struct dev_pm_domain_list *pd_list;
> +
> +		err = devm_pm_domain_attach_list(dev, NULL, &pd_list);
> +		if (err < 0)
> +			return dev_err_probe(dev, err,
> +					     "failed to attach NPU power domains\n");
> +	}
> +

[Severity: High]
Does this early return leak resources?

In rocket_core_init(), rocket_job_init() is called earlier, which allocates
non-devm resources such as an ordered workqueue and a DRM scheduler.

If devm_pm_domain_attach_list() fails, the function returns directly with
dev_err_probe() and skips the necessary rocket_job_fini() and
iommu_group_put() cleanup.

> 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
> @@ -260,7 +276,7 @@ static int rocket_device_runtime_suspend(struct device *dev)
>  	if (!rocket_job_is_idle(&rdev->cores[core]))
>  		return -EBUSY;
>  
> -	clk_bulk_disable_unprepare(ARRAY_SIZE(rdev->cores[core].clks), rdev->cores[core].clks);
> +	clk_bulk_disable_unprepare(rdev->cores[core].soc->num_clks, rdev->cores[core].clks);
>  

[Severity: High]
This is a pre-existing issue, but does this power down the device without
synchronizing the shared IRQ?

Since the driver uses IRQF_SHARED, another device sharing the IRQ line could
trigger the interrupt handler just as the clocks are being disabled.

Can this race condition cause the IRQ handler to execute on powered-down
hardware, potentially leading to a kernel panic? Should we call
synchronize_irq() before turning off the clocks?

> 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
> @@ -326,12 +360,55 @@ static struct dma_fence *rocket_job_run(struct drm_sched_job *sched_job)
> [ ... ]
> +static void rocket_job_handle_irq(struct rocket_core *core)
> +{
> +	u32 clear = 0x1ffff;
> +
> +	if (core->soc->poll_completion) {
> +		/* Stop the completion poll -- we're handling it now. */
> +		atomic_set(&core->poll_active, 0);
> +		hrtimer_cancel(&core->poll_timer);
> +		clear |= PC_INTERRUPT_MASK_RK3576_PC_DONE_0 |
> +			 PC_INTERRUPT_MASK_RK3576_PC_DONE_1;
> +	}
> +
>  	pm_runtime_mark_last_busy(core->dev);
>  
>  	rocket_pc_writel(core, OPERATION_ENABLE, 0x0);
> -	rocket_pc_writel(core, INTERRUPT_CLEAR, 0x1ffff);
> +	rocket_pc_writel(core, INTERRUPT_CLEAR, clear);
>  

[Severity: High]
Can a DMA error on RK3576 trigger a race condition that corrupts hardware
state?

On RK3576, a DMA error drops OPERATION_ENABLE to 0 and raises a DMA error
interrupt. This simultaneously triggers the threaded IRQ handler and the
poll timer workqueue, which will both see OPERATION_ENABLE == 0.

Both threads will concurrently invoke rocket_job_handle_irq(). Because this
function clears OPERATION_ENABLE outside the job_lock, one thread can write
0 to OPERATION_ENABLE right after the other thread has already submitted the
next job and written 1.

Does this permanently halt the hardware by leaving the newly submitted job
disabled?

>  	scoped_guard(mutex, &core->job_lock)
>  		if (core->in_flight_job) {
> [ ... ]
>  			iommu_detach_group(NULL, iommu_group_get(core->dev));

[Severity: High]
This is a pre-existing issue, but does this leak the IOMMU group reference?

The call to iommu_group_get() increments the reference count, but
iommu_detach_group() does not consume or release it.

Since job completion is a hot path, will this quickly overflow the refcount
or leak memory? Should the driver just use the already cached
core->iommu_group instead?

> @@ -424,9 +514,22 @@ static irqreturn_t rocket_job_irq_handler(int irq, void *data)
>  {
>  	struct rocket_core *core = data;
>  	u32 raw_status = rocket_pc_readl(core, INTERRUPT_RAW_STATUS);

[Severity: High]
This is a pre-existing issue, but can this unconditional MMIO read cause a
bus fault if the device is suspended?

Because the driver uses IRQF_SHARED, another device could trigger
rocket_job_irq_handler() while the NPU is runtime suspended and powered down.
Reading from a powered-down device typically returns garbage data or causes a
synchronous external abort.

Should we use pm_runtime_get_if_active() to verify the device is powered
before touching MMIO registers?

> @@ -486,6 +593,10 @@ int rocket_job_init(struct rocket_core *core)
> [ ... ]
>  err_sched:
>  	drm_sched_fini(&core->sched);

[Severity: High]
This is a pre-existing issue, but can calling drm_sched_fini() here cause a
kernel panic?

If drm_sched_init() fails internally (e.g., due to memory allocation failure),
the waitqueues and lists inside the scheduler structure are left uninitialized.
When we jump to the err_sched label, drm_sched_fini() will unconditionally
try to wake up these uninitialized waitqueues.

> @@ -486,6 +593,10 @@ int rocket_job_init(struct rocket_core *core)
>  
>  void rocket_job_fini(struct rocket_core *core)
>  {
> +	atomic_set(&core->poll_active, 0);
> +	hrtimer_cancel(&core->poll_timer);
> +	cancel_work_sync(&core->poll_work);
> +
>  	drm_sched_fini(&core->sched);
>  

[Severity: High]
Is there a teardown order race that allows the hrtimer to be re-armed after
cancellation?

The poll timer and workqueue are cancelled before stopping the DRM scheduler.
If a concurrent scheduler thread pulls a pending job and calls
rocket_job_hw_submit(), it will restart poll_timer.

Can this lead to a use-after-free when the timer fires on freed memory after
teardown completes? Should we stop the scheduler before cancelling the timer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260718031146.3368811-1-gahing@gahingwoo.com?part=6