Re: [PATCH v2] accel/rocket: request the core clocks by name

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] If `rocket_device_init` fails during the first core's probe, the global `rdev` pointer is left as an `ERR_PTR` rather than being reset to `NULL`, causing a kernel panic on subsequent probes.
- [High] The driver leaks the shared `rdev` and `rdev->cores` structures upon probe deferral or failure.
- [High] The driver incorrectly uses `rdev->num_cores` both as a count of active cores and as the boundary index, causing state corruption and resource leaks upon out-of-order unbind.
--

--- Patch [1]: [PATCH v2] accel/rocket: request the core clocks by name ---
commit 4687ac2abf99b6ec4120f9d98c0f4148fc2a6b97
Author: Igor Paunovic <royalnet026@gmail.com>
accel/rocket: request the core clocks by name

This commit fixes clock initialization by setting the proper names for the
"aclk", "hclk", "npu", and "pclk" clocks before requesting them. Previously,
all four clocks defaulted to index 0, which meant only the AXI clock was
controlled by the driver.

> 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 isn't a bug introduced by this patch, but this devm_clk_bulk_get() call
can defer probe. If rocket_core_init() fails here, I noticed a potential 
memory leak in the cleanup path.

In drivers/accel/rocket/rocket_device.c:rocket_device_init(), the rdev 
structure is allocated via devres attached to the static drm_dev device:

drivers/accel/rocket/rocket_device.c:rocket_device_init() {
...
	rdev = devm_drm_dev_alloc(dev, rocket_drm_driver, struct rocket_device, ddev);
...
}

However, if probe fails, drivers/accel/rocket/rocket_drv.c:rocket_probe() 
abandons it without freeing the devres:

drivers/accel/rocket/rocket_drv.c:rocket_probe() {
...
		if (rdev->num_cores == 0) {
			rocket_device_fini(rdev);
			rdev = NULL;
		}
...
}

Since drm_dev is never unbound while the module is loaded, does this leak 
the rdev and rdev->cores structures on every probe deferral?

[Severity: Critical]
This is a pre-existing issue, but looking at the error handling in 
drivers/accel/rocket/rocket_drv.c:rocket_probe():

drivers/accel/rocket/rocket_drv.c:rocket_probe() {
...
	if (rdev == NULL) {
		/* First core probing, initialize DRM device. */
		rdev = rocket_device_init(drm_dev, &rocket_drm_driver);
		if (IS_ERR(rdev)) {
			dev_err(&pdev->dev, "failed to initialize rocket device\n");
			return PTR_ERR(rdev);
		}
	}

	unsigned int core = rdev->num_cores;
...
}

If rocket_device_init() returns an error, rdev is left holding an ERR_PTR 
instead of being reset to NULL. Will a subsequent probe attempt try to 
dereference the poisoned rdev pointer when evaluating rdev->num_cores?

[Severity: High]
This is a pre-existing issue, but does the driver mishandle out-of-order 
unbinds? 

In drivers/accel/rocket/rocket_drv.c:find_core_for_dev(), num_cores is used
as a strict upper bound:

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;
	}
...
}

If an earlier core is unbound, num_cores is decremented. If a higher-indexed 
core is then unbound, find_core_for_dev() won't search far enough to find it. 

Furthermore, if another core is then bound, rocket_probe() inserts it using 
num_cores as the index:

drivers/accel/rocket/rocket_drv.c:rocket_probe() {
...
	rdev->cores[core].rdev = rdev;
	rdev->cores[core].dev = &pdev->dev;
...
}

Could this overwrite an active higher-indexed core that was never properly 
cleaned up?

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