Re: [RFC PATCH 7/9] accel/rocket: add RK3576 NPU (RKNN) support

[sashiko-bot@kernel.org]

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

New issues:
- [High] Unconditional clock and reset requests break driver initialization on RK3588.
- [High] Unhandled completion interrupts cause an interrupt storm on RK3588.
- [High] Using cancel_work() in rocket_reset() causes a race condition and hardware state corruption.
- [High] TOCTOU race in rocket_job_timedout() allows MMIO reads on a suspended device.
- [Medium] DPU and DPU_RDMA pre-arming logic is missing despite claims in the commit message.

Pre-existing issues:
- [High] Shared IRQ handler unconditionally accesses MMIO without verifying the device is active.
--

--- Patch [7]: [RFC PATCH 7/9] accel/rocket: add RK3576 NPU (RKNN) support ---
commit 44e533d6c3c461a28b2f0fb3ee35a8a9e75c974a
Author: Jiaxing Hu <gahing@gahingwoo.com>

accel/rocket: add RK3576 NPU (RKNN) support

Add RK3576 support to the rocket DRM accelerator driver (used with the
Mesa Teflon TFLite delegate).

[ ... ]
>   - map the DPU (0x4000) and DPU_RDMA (0x5000) blocks and pre-arm them

[Severity: Medium]
The commit message mentions mapping and pre-arming the DPU and DPU_RDMA
blocks, and the accessor macros like rocket_dpu_writel are defined in
rocket_core.h. However, looking at rocket_job_hw_submit() in
rocket_job.c, there are no writes to these registers. Was the pre-arming
logic omitted by accident? Without it, might the compute path stall after
loading one slice on RK3576?

> diff --git a/drivers/accel/rocket/rocket_core.c b/drivers/accel/rocket/rocket_core.c
> --- a/drivers/accel/rocket/rocket_core.c
> +++ b/drivers/accel/rocket/rocket_core.c
> @@ -22,12 +23,23 @@ int rocket_core_init(struct rocket_core *core)
>  	int err = 0;
>  
>  	core->resets[0].id = "srst_a";
> -	core->resets[1].id = "srst_h";
>  	err = devm_reset_control_bulk_get_exclusive(&pdev->dev, ARRAY_SIZE(core->resets),
>  						    core->resets);

[Severity: High]
Will this break initialization on RK3588? The srst_h reset line has been
removed from the array, meaning the RK3588 hardware might not be fully
reset during probe.

>  	if (err)
>  		return dev_err_probe(dev, err, "failed to get resets for core %d\n", core->index);
>  
> +	core->clks[0].id = "aclk";
> +	core->clks[1].id = "hclk";
> +	core->clks[2].id = "npu";
> +	core->clks[3].id = "pclk";
> +	/*
> +	 * RK3576: the CBUF (convolution buffer) has its own clock domain. The CNA
> +	 * fills the CBUF and CORE reads from it; without these the compute path
> +	 * stalls after loading one slice (RDMA, which bypasses the CBUF, still
> +	 * runs). The vendor keeps all NPU clocks on whenever powered.
> +	 */
> +	core->clks[4].id = "aclk_cbuf";
> +	core->clks[5].id = "hclk_cbuf";
>  	err = devm_clk_bulk_get(dev, ARRAY_SIZE(core->clks), core->clks);

[Severity: High]
Does this unconditional request for RK3576-specific clocks cause the probe
to fail on RK3588? If aclk_cbuf and hclk_cbuf do not exist in the RK3588
device tree, devm_clk_bulk_get() will return -ENOENT and break support for
the older platform.

[ ... ]
> 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
[ ... ]
> @@ -137,8 +148,21 @@ static void rocket_job_hw_submit(struct rocket_core *core, struct rocket_job *jo
>  	rocket_pc_writel(core, REGISTER_AMOUNTS,
>  			 PC_REGISTER_AMOUNTS_PC_DATA_AMOUNT((task->regcmd_count + 1) / 2 - 1));
>  
> -	rocket_pc_writel(core, INTERRUPT_MASK, PC_INTERRUPT_MASK_DPU_0 | PC_INTERRUPT_MASK_DPU_1);
> -	rocket_pc_writel(core, INTERRUPT_CLEAR, PC_INTERRUPT_CLEAR_DPU_0 | PC_INTERRUPT_CLEAR_DPU_1);
> +	/* Enable DMA-error interrupts; PC_DONE (bits 28-29) is polled, see above. */
> +	rocket_pc_writel(core, INTERRUPT_MASK,
> +			 PC_INTERRUPT_MASK_DPU_0    | PC_INTERRUPT_MASK_DPU_1    |
> +			 PC_INTERRUPT_MASK_CORE_0   | PC_INTERRUPT_MASK_CORE_1   |
> +			 PC_INTERRUPT_MASK_PPU_0    | PC_INTERRUPT_MASK_PPU_1    |
> +			 PC_INTERRUPT_MASK_CNA_CSC_0 | PC_INTERRUPT_MASK_CNA_CSC_1 |
> +			 PC_INTERRUPT_MASK_DMA_READ_ERROR |
> +			 PC_INTERRUPT_MASK_DMA_WRITE_ERROR);

[Severity: High]
Could this lead to an interrupt storm on RK3588? In rocket_job_hw_submit(),
completion interrupts like DPU_0 and CORE_0 are unmasked in hardware.

However, rocket_job_irq_handler() now masks INTERRUPT_RAW_STATUS to only
check DMA errors and returns IRQ_NONE. This means the completion bits are
never cleared and the interrupt is never acknowledged.

[ ... ]
> @@ -326,56 +352,99 @@ static struct dma_fence *rocket_job_run(struct drm_sched_job *sched_job)
[ ... ]
>  static void
>  rocket_reset(struct rocket_core *core, struct drm_sched_job *bad)
>  {
> +	struct rocket_job *job;
> +
>  	if (!atomic_read(&core->reset.pending))
>  		return;
>  
> +	atomic_set(&core->poll_active, 0);
> +	hrtimer_cancel(&core->poll_timer);
> +	cancel_work(&core->poll_work);

[Severity: High]
Is it safe to use the non-blocking cancel_work() here instead of
cancel_work_sync()? 

If a job timeout occurs while poll_work is actively executing on another
CPU, it will continue running alongside the reset sequence.

Since rocket_job_handle_irq() writes to OPERATION_ENABLE outside of the
job_lock, this unguarded write could abruptly halt a newly submitted job
after the reset completes.

[ ... ]
> @@ -385,7 +454,14 @@ static enum drm_gpu_sched_stat rocket_job_timedout(struct drm_sched_job *sched_j
>  	struct rocket_device *rdev = job->rdev;
>  	struct rocket_core *core = sched_to_core(rdev, sched_job->sched);
>  
> -	dev_err(core->dev, "NPU job timed out");
> +	if (pm_runtime_active(core->dev))
> +		dev_err(core->dev,
> +			"NPU job timed out: RAW_STATUS=0x%08x MASK=0x%08x OP_EN=0x%08x\n",
> +			rocket_pc_readl(core, INTERRUPT_RAW_STATUS),
> +			rocket_pc_readl(core, INTERRUPT_MASK),
> +			rocket_pc_readl(core, OPERATION_ENABLE));
> +	else
> +		dev_err(core->dev, "NPU job timed out (device not active)\n");

[Severity: High]
Is there a race condition here that could allow MMIO reads on a suspended
device? 

The lockless pm_runtime_active() check does not take a reference to
prevent suspension. If the device autosuspends in the narrow window between
the check and the rocket_pc_readl() calls, this might trigger a bus fault.
Would using pm_runtime_get_if_active() be safer?

[ ... ]
> @@ -420,14 +496,16 @@ 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 since the IRQ handler is registered
with IRQF_SHARED, could reading INTERRUPT_RAW_STATUS unconditionally cause
a kernel panic if the device is runtime suspended? 

If another device on the same IRQ line fires, reading unpowered MMIO
registers will trigger a synchronous external abort. Should this check
pm_runtime_get_if_active() first?

> +	/* Only bits 0-13 (DMA errors) can raise this IRQ; PC_DONE is polled. */
> +	u32 active = raw_status & 0x3fff;
> +
> +	if (!active)
> +		return IRQ_NONE;

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