Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [High] The error path for `noc_init` failure in `rocket_core_init` calls `rocket_job_fini()` instead of `rocket_core_fini()`, leaking an IOMMU group reference and leaving PM runtime active. Pre-existing issues: - [High] The `core->clks` array lacks initialization of its `id` fields before being passed to `devm_clk_bulk_get()`, causing the driver to configure the rate on the wrong clock in `rk3568_noc_init()`. - [High] Failed probe leaks `struct rocket_device` and associated DRM structures because they are devm-allocated against a global persistent platform device. -- commit 60f18710aa215157b0b6ac91ed70f00cc049eb2c Author: Midgy BALON <[email protected]> accel: rocket: Add RK3568 SoC support The RK3568 has a single core of the same NVDLA-derived NPU IP as the RK3588, with a 32-bit AXI master. Unlike the RK3588 it must be powered on and de-idled through the PMU, and its PVTPLL clock started via SCMI, before the NPU is reachable. Add rk3568_soc_data with an noc_init callback performing this bring-up. > diff --git a/drivers/accel/rocket/rocket_core.c > b/drivers/accel/rocket/rocket_core.c > index 09c445af7de73..a8de876365873 100644 > --- a/drivers/accel/rocket/rocket_core.c > +++ b/drivers/accel/rocket/rocket_core.c > @@ -88,6 +88,15 @@ int rocket_core_init(struct rocket_core *core) > return err; > } > > + if (core->soc_data->noc_init) { > + err = core->soc_data->noc_init(core); > + if (err) { > + pm_runtime_put_sync(dev); > + rocket_job_fini(core); [Severity: High] If noc_init() fails, should this call rocket_core_fini() instead of rocket_job_fini()? Calling rocket_job_fini() here bypasses the rest of the teardown that rocket_core_fini() handles. This leaves the PM runtime enabled and autosuspend incorrectly configured, and skips iommu_group_put() which leaks the IOMMU group reference. > + return err; > + } > + } > + > version = rocket_pc_readl(core, VERSION); [ ... ] > diff --git a/drivers/accel/rocket/rocket_drv.c > b/drivers/accel/rocket/rocket_drv.c > index c18840e5aff76..5a72d0b5f4dff 100644 > --- a/drivers/accel/rocket/rocket_drv.c > +++ b/drivers/accel/rocket/rocket_drv.c [ ... ] > @@ -217,12 +219,63 @@ static void rocket_remove(struct platform_device *pdev) > } > } > > +/* > + * On RK3568 the NPU NOC bus is gated and idle out of reset and must be > + * powered on and de-idled through the PMU before the NPU is reachable. PMU > + * registers use a write-mask protocol: the upper 16 bits enable writes to > the > + * matching lower 16 bits. > + * > + * The NPU's high-speed clock is a PVTPLL managed by TF-A via SCMI and must > be > + * running before the NOC acknowledges the de-idle request. Force a real > SCMI > + * rate change (an intermediate rate defeats the clock framework's > + * unchanged-rate shortcut) now that the power domain is on and clocks > enabled. > + */ > +#define ROCKET_RK3568_SCMI_CLK 2 > + > +static int rk3568_noc_init(struct rocket_core *core) > +{ > + struct regmap *pmu; > + unsigned int val; > + int ret; > + > + clk_set_rate(core->clks[ROCKET_RK3568_SCMI_CLK].clk, 600000000UL); > + clk_set_rate(core->clks[ROCKET_RK3568_SCMI_CLK].clk, 1000000000UL); [Severity: High] This is a pre-existing issue, but does core->clks actually contain the correct SCMI clock here? Looking at rocket_core_init(), devm_clk_bulk_get() is called without first initializing the .id fields of the core->clks array: drivers/accel/rocket/rocket_core.c:rocket_core_init() { ... err = devm_clk_bulk_get(dev, ARRAY_SIZE(core->clks), core->clks); ... } Because the IDs are uninitialized (NULL), clk_get() will repeatedly return the very first clock of the device. Does this mean these clk_set_rate() calls are erroneously forcing a rate change on the wrong clock while leaving the PVTPLL unconfigured? [ ... ] [Severity: High] This is a pre-existing issue, but could probe failures (such as noc_init returning an error) cause an unbounded memory leak? In rocket_probe(), the global drm_dev is passed to rocket_device_init(): drivers/accel/rocket/rocket_drv.c:rocket_probe() { ... rdev = rocket_device_init(drm_dev, &rocket_drm_driver, soc_data); ... } Inside rocket_device_init(), memory is allocated against this global device's devres list: drivers/accel/rocket/rocket_device.c:rocket_device_init() { ... rdev = devm_drm_dev_alloc(dev, rocket_drm_driver, struct rocket_device, ddev); ... } If probe fails, rocket_probe() calls rocket_device_fini() to unregister the DRM device. However, it does not free the devm-allocated memory because the allocations are tied to the global drm_dev that persists until module unload. Does this mean every deferred probe or bind/unbind action allocates new memory on the global devres list? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
