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 <[email protected]> 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/[email protected]?part=6
