Re: [PATCH v11 12/14] 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:

Pre-existing issues:
- [High] Shared IRQ handler accesses hardware registers without checking PM state, and runtime suspend lacks synchronize_irq().
- [High] Reference leak of iommu_group in job completion path.
- [High] Array bounds mismatch and logic error in `find_core_for_dev` and core registration due to naive decrement on device removal.
- [High] Concurrent probe of multiple NPU cores causes data races in global rdev initialization and array management.
- [High] Leaking devm resources on unbound pseudo-device across rebinds or `-EPROBE_DEFER`.
- [High] A 64-bit DMA address is truncated to 32 bits when written to the hardware, leading to IOMMU faults if the address exceeds 4GB.
--

commit da7c000fc37b3b36196f66187168ddef41f790b6
Author: Jiaxing Hu <gahing@gahingwoo.com>

    accel/rocket: add RK3576 NPU (RKNN) support
    
    The RK3576 has two cores of the same RKNN block and a few platform
    differences:

[ ... ]

> diff --git a/drivers/accel/rocket/rocket_device.c b/drivers/accel/rocket/rocket_device.c
> index 46e6ee1e72c5f..923add5bdc87e 100644
> --- a/drivers/accel/rocket/rocket_device.c
> +++ b/drivers/accel/rocket/rocket_device.c
> @@ -9,6 +9,7 @@
>  #include <linux/of.h>
>  
>  #include "rocket_device.h"
> +#include "rocket_drv.h"
>  
>  struct rocket_device *rocket_device_init(struct platform_device *pdev,
>  					 const struct drm_driver *rocket_drm_driver)
> @@ -27,7 +28,11 @@ struct rocket_device *rocket_device_init(struct platform_device *pdev,
>  	ddev = &rdev->ddev;
>  	dev_set_drvdata(dev, rdev);
>  

[Severity: High]
This is a pre-existing issue, but does rocket_device_init() leak devm
resources if probe defers or unbinds?

It appears that devm_drm_dev_alloc() is called earlier in this function
and binds the allocation to the persistent global drm_dev instead of the
pdev being probed. If probe defers, or if the cores are unbound,
rocket_remove() simply clears the rdev pointer without freeing the memory
bound to drm_dev, leading to memory leaks on successive probe deferrals.

> -	for_each_compatible_node(core_node, NULL, "rockchip,rk3588-rknn-core")
> +	/*
> +	 * Count over the same match table the platform driver binds with, so
> +	 * that a core added there is counted here without a second edit.
> +	 */
> +	for_each_matching_node(core_node, rocket_dt_match)
>  		if (of_device_is_available(core_node))
>  			num_cores++;
>  

[ ... ]

> diff --git a/drivers/accel/rocket/rocket_drv.c b/drivers/accel/rocket/rocket_drv.c
> index 6e7dc91c5faac..e469629499fdb 100644
> --- a/drivers/accel/rocket/rocket_drv.c
> +++ b/drivers/accel/rocket/rocket_drv.c
> @@ -217,13 +217,23 @@ static void rocket_remove(struct platform_device *pdev)

[Severity: High]
This is a pre-existing issue, but are there concurrent probe data races here?

In rocket_probe(), the global rdev pointer is checked and assigned without
synchronization, and rdev->num_cores is used as an array index concurrently.
If multiple NPU cores are probed concurrently, could this lead to double
initialization or out-of-bounds array accesses?

Additionally, when rocket_remove() executes:

drivers/accel/rocket/rocket_drv.c:rocket_remove() {
    ...
    rdev->num_cores--;
    ...
}

It decrements the active core count without compacting the array.
Could a subsequent bind overwrite active devices and truncate search
boundaries in find_core_for_dev()?

>  static const struct rocket_soc_data rk3588_soc_data = {
>  	.num_clks = 4,
>  	.num_resets = 2,
> +	.multi_power_domain = false,
> +	.task_con_16bit = false,
>  };
>  

[ ... ]

> diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
> index 69e29f40f27a0..2a272c2ef6bed 100644
> --- a/drivers/accel/rocket/rocket_job.c
> +++ b/drivers/accel/rocket/rocket_job.c

[ ... ]

[Severity: High]
This is a pre-existing issue, but does this driver safely handle shared IRQs
during runtime suspend?

In rocket_job_irq_handler(), the code unconditionally reads from
INTERRUPT_RAW_STATUS:

drivers/accel/rocket/rocket_job.c:rocket_job_irq_handler() {
    ...
    u32 raw_status = rocket_pc_readl(core, INTERRUPT_RAW_STATUS);
    ...
}

However, rocket_device_runtime_suspend() disables clocks without calling
synchronize_irq(). Because this uses a shared IRQ line (IRQF_SHARED), if
another device on the same line triggers an interrupt while the NPU is
suspended, could reading these unpowered hardware registers cause a system
hang or external abort?

[Severity: High]
This isn't a bug introduced by this patch, but is there a reference leak of
the IOMMU group in the job completion path?

In rocket_job_next_locked():

drivers/accel/rocket/rocket_job.c:rocket_job_next_locked() {
    ...
    iommu_detach_group(NULL, iommu_group_get(core->dev));
    ...
}

This calls iommu_group_get() inline, which increments the reference count,
but there appears to be no corresponding iommu_group_put(), meaning the
reference is leaked on every job completion.

> @@ -142,10 +165,17 @@ static void rocket_job_hw_submit(struct rocket_core *core, struct rocket_job *jo
>  	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);
>  

[Severity: High]
This is a pre-existing issue, but could the DMA address get truncated here?

The driver explicitly asks the IOMMU for up to 40-bit IOVAs via
dma_set_mask_and_coherent(dev, DMA_BIT_MASK(40)) in rocket_core_init().

However, the hardware submission path writes the command buffer address
using a 32-bit register write:

drivers/accel/rocket/rocket_job.c:rocket_job_hw_submit() {
    ...
    rocket_pc_writel(core, BASE_ADDRESS, task->regcmd);
    ...
}

Since task->regcmd is written to a 32-bit hardware register, and userspace
may pass a truncated IOVA, will this cause IOMMU faults if the address exceeds
4GB?

> -	rocket_pc_writel(core, TASK_CON, PC_TASK_CON_RESERVED_0(1) |
> -					 PC_TASK_CON_TASK_COUNT_CLEAR(1) |
> -					 PC_TASK_CON_TASK_NUMBER(1) |
> -					 PC_TASK_CON_TASK_PP_EN(1));
> +	if (core->soc->task_con_16bit)
> +		rocket_pc_writel(core, TASK_CON,
> +				 RK3576_PC_TASK_CON_LAST_LAYER_CLEAR |
> +				 RK3576_PC_TASK_CON_COUNT_CLEAR |
> +				 RK3576_PC_TASK_CON_PP_EN |
> +				 RK3576_PC_TASK_CON_TASK_NUMBER(1));
> +	else
> +		rocket_pc_writel(core, TASK_CON, PC_TASK_CON_RESERVED_0(1) |
> +						 PC_TASK_CON_TASK_COUNT_CLEAR(1) |
> +						 PC_TASK_CON_TASK_NUMBER(1) |
> +						 PC_TASK_CON_TASK_PP_EN(1));
>  
>  	rocket_pc_writel(core, TASK_DMA_BASE_ADDR, PC_TASK_DMA_BASE_ADDR_DMA_BASE_ADDR(0x0));
>

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