Re: [RFC PATCH v3 6/6] arm64: dts: rockchip: rk3576-rock-4d: enable NPU

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Missing cleanup of `poll_timer` and `poll_work` in `rocket_job_fini()` leads to Use-After-Free on unload.
- [High] The driver statically requires 6 clocks for RK3576, causing inevitable probe failure for `rknn_core_1`.
- [Medium] Using `regulator-always-on` for the dedicated NPU regulator prevents runtime power savings and bypasses genpd management.
- [Low] Board-level DeviceTree overrides SoC-internal NPU power domain topology, violating DT schema bindings.
--

--- Patch [6]: [RFC PATCH v3 6/6] arm64: dts: rockchip: rk3576-rock-4d: enable NPU ---
commit 317571d0650068ec684ea45a47d155fe5e0ea20c
Author: Jiaxing Hu <gahing@gahingwoo.com>

arm64: dts: rockchip: rk3576-rock-4d: enable NPU

This commit enables rknn_core_0 and its IOMMU on the Radxa ROCK 4D. It
supplies the core from vdd_npu_s0 and assigns multiple NPU power domains
to the single NPU node.

> diff --git a/arch/arm64/boot/dts/rockchip/rk3576-rock-4d.dts b/arch/arm64/boot/dts/rockchip/rk3576-rock-4d.dts
> --- a/arch/arm64/boot/dts/rockchip/rk3576-rock-4d.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3576-rock-4d.dts
[ ... ]
> @@ -442,6 +442,7 @@ regulator-state-mem {
>  			};
>  
>  			vdd_npu_s0: dcdc-reg2 {
> +				regulator-always-on;

[Severity: Medium]
Does setting regulator-always-on for the dedicated NPU regulator bypass 
the generic power domain management? It appears this will prevent the 
regulator from being powered down during runtime suspend, potentially 
wasting power when the accelerator is idle.

>  				regulator-boot-on;
>  				regulator-enable-ramp-delay = <400>;
[ ... ]
> +&rknn_core_0 {
> +	npu-supply = <&vdd_npu_s0>;
> +	/*
> +	 * Power BOTH NPU domains (NPU0 + NPU1) like the vendor's single NPU node,
> +	 * even though rocket computes only on core 0: the CBUF->CMAC read path is
> +	 * only fully powered with NPU1 up. rocket attaches the multi-PD list.
> +	 */
> +	power-domains = <&power RK3576_PD_NPU0>, <&power RK3576_PD_NPU1>;

[Severity: Low]
Will this board-level override cause a device tree schema validation 
failure? The rockchip,rk3588-rknn-core.yaml binding strictly enforces a 
maximum of 1 power domain:

Documentation/devicetree/bindings/npu/rockchip,rk3588-rknn-core.yaml:
  power-domains:
    maxItems: 1

[ ... ]

[Severity: High]
Does rocket_job_fini() in drivers/accel/rocket/rocket_job.c leave 
background polling active when the memory is freed?

drivers/accel/rocket/rocket_job.c:
void rocket_job_fini(struct rocket_core *core)

If the module is unloaded while a job is actively polling, the lack of 
hrtimer_cancel() and cancel_work_sync() calls could allow the timer or 
workqueue to execute against freed struct rocket_core memory, causing a 
use-after-free.

[Severity: High]
Will rknn_core_1 fail to probe on the RK3576 SoC due to a strict clock 
requirement?

drivers/accel/rocket/rocket_drv.c:rk3576_soc_data()
	.num_clks = 6,

drivers/accel/rocket/rocket_core.c:rocket_core_init()
	err = devm_clk_bulk_get(dev, core->soc->num_clks, core->clks);
	if (err)
		return dev_err_probe(dev, err, "failed to get clocks for core %d\n", core->index);

Because num_clks is statically defined as 6 for the entire SoC, and 
rknn_core_1 does not possess the aclk_cbuf and hclk_cbuf clocks, this bulk 
request will fail with -ENOENT when rknn_core_1 is enabled.

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