Re: [RFC PATCH v4 1/9] accel: rocket: Introduce per-SoC rocket_soc_data

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The shared IRQ handler accesses hardware registers without checking if the device is active.
- [High] The runtime suspend callback disables hardware clocks without synchronizing the shared IRQ.
- [High] The global `rdev` pointer is not reset to NULL when initialization fails, causing a kernel panic on subsequent probes.
- [High] Hardware registers are read during probe without ensuring the device resets are explicitly deasserted.
--

commit e068c6d40922642be56ef8ce61fd76a8d5727d7c
Author: Midgy BALON <midgy971@gmail.com>

accel: rocket: Introduce per-SoC rocket_soc_data

Add a per-SoC data structure carried in the OF match table, currently
holding only the NPU AXI address width, and use it for the per-core DMA
mask instead of a hardcoded 40-bit value.

> diff --git a/drivers/accel/rocket/rocket_core.c b/drivers/accel/rocket/rocket_core.c
> index b3b2fa9ba645a..09c445af7de73 100644
> --- a/drivers/accel/rocket/rocket_core.c
> +++ b/drivers/accel/rocket/rocket_core.c
> @@ -21,6 +22,10 @@ int rocket_core_init(struct rocket_core *core)
>  	u32 version;
>  	int err = 0;
>  
> +	core->soc_data = of_device_get_match_data(dev);
> +	if (!core->soc_data)
> +		return dev_err_probe(dev, -EINVAL, "missing SoC match data\n");
> +
>  	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),

[Severity: High]
This isn't a bug introduced by this patch, but I noticed hardware registers are
read during probe without ensuring the device resets are explicitly deasserted.

Later in rocket_core_init():

	version = rocket_pc_readl(core, VERSION);

If the bootloader leaves the NPU in an asserted reset state, attempting to
read its registers will cause a synchronous external abort. Would it make
sense to deassert the resets before accessing registers here?

[ ... ]

> diff --git a/drivers/accel/rocket/rocket_drv.c b/drivers/accel/rocket/rocket_drv.c
> index 8bbbce594883e..384c38e13acce 100644
> --- a/drivers/accel/rocket/rocket_drv.c
> +++ b/drivers/accel/rocket/rocket_drv.c
[ ... ]
> @@ -213,8 +213,12 @@ static void rocket_remove(struct platform_device *pdev)
>  	}
>  }

[Severity: High]
This is a pre-existing issue, but I noticed the global rdev pointer is not
reset to NULL when initialization fails in rocket_probe() in this file:

	rdev = rocket_device_init(drm_dev, &rocket_drm_driver, soc_data);
	if (IS_ERR(rdev)) {
		...
		return PTR_ERR(rdev);
	}

If this fails during the first core's probe, an error pointer is assigned to
the global rdev pointer without being cleared. On the next core's probe, the
check for NULL will fail and it will blindly dereference the error pointer.
Could this be addressed by clearing the global pointer on the error path?

[Severity: High]
This is a pre-existing issue, but the runtime suspend callback in this driver
disables hardware clocks without synchronizing the shared IRQ:

rocket_device_runtime_suspend() {
	...
	clk_bulk_disable_unprepare(ARRAY_SIZE(rdev->cores[core].clks), ...);
	...
}

Should this call synchronize_irq() first to avoid a race condition where the
IRQ handler might be executing mid-flight on another CPU while the hardware
is powered down?

[Severity: High]
This is a pre-existing issue, but in drivers/accel/rocket/rocket_job.c, the
shared IRQ handler accesses hardware registers without checking if the device
is active:

rocket_job_irq_handler() {
	...
	u32 raw_status = rocket_pc_readl(core, INTERRUPT_RAW_STATUS);
	...
}

Because the IRQ is registered with IRQF_SHARED, if the handler is invoked by
another device while the NPU is runtime suspended, reading the registers can
result in invalid data (~0u) and spurious interrupts. Should it use
pm_runtime_get_if_active() before reading the status register?

[ ... ]

>  static const struct of_device_id dt_match[] = {
> -	{ .compatible = "rockchip,rk3588-rknn-core" },
> +	{ .compatible = "rockchip,rk3588-rknn-core", .data = &rk3588_soc_data },
>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, dt_match);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260613070116.438906-1-midgy971@gmail.com?part=1