Re: [PATCH v12 01/14] accel/rocket: request the core clocks by name

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Probe deferral, missing clocks, or device unbind leaks devres allocations attached to the global `drm_dev`.
- [High] Incorrect core tracking logic in `rocket_remove` and `rocket_probe` causes active cores to be orphaned, unreachable, or silently overwritten upon partial unbind.
--

commit 4baecbc1d36cdb2c0321f080e879383800570708
Author: Igor Paunovic <royalnet026@gmail.com>

accel/rocket: request the core clocks by name

This patch fixes an issue where the NPU driver failed to set the .id members
of its clk_bulk_data struct before requesting them. Without IDs, clk_bulk_get
defaulted to returning the first clock in the device tree for all entries. The
clocks are now properly requested by name as required by the DT binding.

> diff --git a/drivers/accel/rocket/rocket_core.c b/drivers/accel/rocket/rocket_core.c
> index b3b2fa9ba645a..5dd260bacbff6 100644
> --- a/drivers/accel/rocket/rocket_core.c
> +++ b/drivers/accel/rocket/rocket_core.c
> @@ -28,6 +28,10 @@ int rocket_core_init(struct rocket_core *core)
>  	if (err)
>  		return dev_err_probe(dev, err, "failed to get resets for core %d\n", core->index);
>  
> +	core->clks[0].id = "aclk";
> +	core->clks[1].id = "hclk";
> +	core->clks[2].id = "npu";
> +	core->clks[3].id = "pclk";
>  	err = devm_clk_bulk_get(dev, ARRAY_SIZE(core->clks), core->clks);
>  	if (err)
>  		return dev_err_probe(dev, err, "failed to get clocks for core %d\n", core->index);

[Severity: High]
This is a pre-existing issue, but does this failure path cause an unbounded
resource leak if devm_clk_bulk_get() defers probe or fails?

If rocket_core_init() fails here, rocket_probe() cleans up by calling
rocket_device_fini() and setting the global rdev pointer to NULL:

drivers/accel/rocket/rocket_drv.c:rocket_probe() {
    ...
	ret = rocket_core_init(&rdev->cores[core]);
	if (ret) {
		rdev->num_cores--;

		if (rdev->num_cores == 0) {
			rocket_device_fini(rdev);
			rdev = NULL;
		}
	}
    ...
}

However, rdev and its cores array are allocated in rocket_device_init() using
devm_* functions attached to drm_dev, which is a static global device:

drivers/accel/rocket/rocket_device.c:rocket_device_init() {
    ...
	rdev = devm_drm_dev_alloc(dev, rocket_drm_driver, struct rocket_device, ddev);
    ...
	rdev->cores = devm_kcalloc(dev, num_cores, sizeof(*rdev->cores), GFP_KERNEL);
    ...
}

Because drm_dev is global and persists across probe deferrals, its devres
list is never cleared. When rocket_probe() executes again, a new rdev is
allocated on the same global device, permanently leaking the previous one.

[Severity: High]
This is also a pre-existing issue, but if this failure path is hit (or if a
core is unbound via sysfs), does the core tracking logic in rocket_remove()
and rocket_probe() orphan the remaining active cores?

If a core other than the most-recently-probed one fails or unbinds,
rocket_remove() blindly decrements rdev->num_cores:

drivers/accel/rocket/rocket_drv.c:rocket_remove() {
    ...
	rocket_core_fini(&rdev->cores[core]);
	rdev->num_cores--;
    ...
}

Without shifting array elements to compact the list, active cores located
past the shrunken num_cores boundary can no longer be found for teardown or
power management:

drivers/accel/rocket/rocket_drv.c:find_core_for_dev() {
    ...
	for (unsigned int core = 0; core < rdev->num_cores; core++) {
		if (dev == rdev->cores[core].dev)
			return core;
	}
    ...
}

This could cause power management callbacks to fail with -ENODEV while the
hardware is still running. Furthermore, if a new core binds later,
rocket_probe() will insert it at rdev->num_cores, which could silently
overwrite the live context of the stranded core.

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