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

[sashiko-bot@kernel.org]

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

New issues:
- [High] Missing teardown of `poll_timer` and `poll_work` causes use-after-free on module unload or device teardown.
- [High] Stale `poll_work` execution can prematurely complete and corrupt newly submitted jobs because pending work is not cancelled.

Pre-existing issues:
- [High] The shared IRQ handler accesses hardware registers without ensuring the device is active, causing potential crashes if the device is suspended.
- [High] Unbalanced `iommu_group_get()` leaks an IOMMU group reference on every job completion.
--

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

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 CBUF (convolution buffer) has its own clock domain, so the core
   needs six clocks rather than four;
 - the BIU reset moved into the power domain, leaving one reset here;
 - the NPU spans two power domains, and a device with more than one is
   skipped by the driver-core single-domain auto-attach, so the list has
   to be attached explicitly;
 - the DPU completion interrupt is armed exactly as on RK3588 but never
   reaches the GIC. The completion is visible in INTERRUPT_RAW_STATUS,
   so sample that from an hrtimer rather than wait for an interrupt that
   does not come. The interrupt stays armed, so if it ever does arrive
   the normal handler finalises the job first.
    
Select all of that from of_device_id match data so the RK3588 path keeps
its existing counts and behaviour unchanged.

> diff --git a/drivers/accel/rocket/rocket_drv.c b/drivers/accel/rocket/rocket_drv.c
> index 8bbbce594883e..7f7dfa3742fc8 100644
> --- 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);
>  
>  	return 0;
>  }

[Severity: High]
This is a pre-existing issue, but can the shared IRQ handler crash if it runs
while the device is suspended?

The NPU driver registers its interrupt handler rocket_job_irq_handler() with
IRQF_SHARED. The hardirq handler unconditionally reads the
INTERRUPT_RAW_STATUS MMIO register without calling pm_runtime_get_if_active().

Additionally, rocket_device_runtime_suspend() disables hardware clocks here
without calling synchronize_irq().

If another device sharing the IRQ line triggers an interrupt while the NPU
is runtime-suspended, the handler will attempt to read powered-down hardware.
Should the suspend callback use synchronize_irq() and the IRQ handler verify
the device is active?

> diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
> index bb77b6bf0f231..e9103a93020cc 100644
> --- a/drivers/accel/rocket/rocket_job.c
> +++ b/drivers/accel/rocket/rocket_job.c
[ ... ]
> @@ -341,8 +358,42 @@ static struct dma_fence *rocket_job_run(struct drm_sched_job *sched_job)
>  	return ERR_PTR(ret);
>  }
>  
> +static void rocket_job_handle_irq(struct rocket_core *core);
> +
> +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;
> +
> +	raw = rocket_pc_readl(core, INTERRUPT_RAW_STATUS);
> +	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;
> +}
> +
> +static void rocket_poll_work_fn(struct work_struct *work)
> +{
> +	struct rocket_core *core = container_of(work, struct rocket_core, poll_work);
> +
> +	rocket_job_handle_irq(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);
> +	}
> +

[Severity: High]
Can a stale poll_work execution corrupt the next job?

If a real hardware IRQ arrives after poll_timer queues poll_work, the real
IRQ handles the completion and cancels the timer here. However, poll_work
could remain pending in the workqueue.

The DRM scheduler could then immediately submit the next job. When the stale
poll_work executes, it calls rocket_job_handle_irq() which would incorrectly
mark the new job as finished, signal its completion fence early, detach its
IOMMU group, and halt the hardware.

Does the driver need a way to cancel the pending work or ignore it if it is
no longer relevant, both here and in rocket_reset()?

[Severity: High]
This is a pre-existing issue, but does this function leak an IOMMU group
reference on every job completion?

Further down in rocket_job_handle_irq(), the code calls:

	iommu_detach_group(NULL, iommu_group_get(core->dev));

The iommu_group_get() function increments the kobject reference count, but
iommu_detach_group() does not consume it. Because there is no matching
iommu_group_put(), a reference is leaked every time a job finishes. Should
this use the existing core->iommu_group pointer instead?

>  	pm_runtime_mark_last_busy(core->dev);
>  
>  	rocket_pc_writel(core, OPERATION_ENABLE, 0x0);
[ ... ]
> @@ -460,6 +511,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);

[Severity: High]
Does this code leak the timer and work on module unload?

Since poll_timer and poll_work are initialized here, shouldn't they be
cancelled in rocket_job_fini() with hrtimer_cancel() and cancel_work_sync()?

Because the rocket_core struct is allocated with devm_kcalloc(), it is freed
shortly after driver removal. If the timer fires or the pending work executes
after the memory is freed, it could cause a use-after-free crash.

>  	spin_lock_init(&core->fence_lock);
>  	mutex_init(&core->job_lock);
>

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