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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] rocket_remove() decrements rdev->num_cores without shifting array elements, making subsequent cores inaccessible.
- [High] IOMMU group reference count leak on every job completion in rocket_job_handle_irq().
- [High] Double free / invalid cleanup when drm_sched_init() fails.
- [High] Unbounded memory leak of rdev and rdev->cores arrays on probe deferral.
- [High] Shared IRQ handler unconditionally accesses hardware registers when clocks and power domains might be disabled.
- [High] Global rdev static variable is left containing an error pointer if rocket_device_init() fails, causing a kernel panic on subsequent probes.
- [High] Registers are accessed while the device might still be held in reset, leading to bus hangs.
--

commit 31505652560107069dc219815082a4ea6f173762
Author: Igor Paunovic <royalnet026@gmail.com>

accel/rocket: request the core clocks by name

This commit fixes a clock handling bug where devm_clk_bulk_get() was
called with uninitialized clock IDs, resulting in the driver repeatedly
requesting the first clock from the device tree and ignoring the others.
The clock IDs are now explicitly set to "aclk", "hclk", "npu", and
"pclk" before requesting them, ensuring the correct clocks are prepared
and enabled.

> 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 later in this same function:

drivers/accel/rocket/rocket_core.c:rocket_core_init() {
    ...
	version = rocket_pc_readl(core, VERSION);
    ...
}

Are these registers accessed while the device might still be held in reset?
Since devm_reset_control_bulk_get_exclusive() is called earlier, shouldn't
the resets be explicitly deasserted before this first memory-mapped read to
prevent bus hangs?


[Severity: High]
This is a pre-existing issue, but in rocket_drv.c:rocket_remove():

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

Does decrementing rdev->num_cores without shifting array elements make
subsequent cores inaccessible? Since find_core_for_dev() loops up to
rdev->num_cores, if an earlier core is removed, wouldn't the highest
index core fall outside the loop bounds?


[Severity: High]
This is a pre-existing issue, but in rocket_job.c:rocket_job_handle_irq():

drivers/accel/rocket/rocket_job.c:rocket_job_handle_irq() {
    ...
			iommu_detach_group(NULL, iommu_group_get(core->dev));
    ...
}

Does this code leak the IOMMU group reference count? iommu_group_get()
increments the reference count, but iommu_detach_group() consumes the
pointer without decrementing it, which could prevent the group from ever
being freed on driver unbind.


[Severity: High]
This is a pre-existing issue, but in rocket_job.c:rocket_job_init():

drivers/accel/rocket/rocket_job.c:rocket_job_init() {
    ...
	ret = drm_sched_init(&core->sched, &args);
	if (ret) {
		dev_err(core->dev, "Failed to create scheduler: %d.", ret);
		goto err_sched;
	}

	return 0;

err_sched:
	drm_sched_fini(&core->sched);
    ...
}

Can this lead to a double free or NULL pointer dereference? If
drm_sched_init() fails, it internally frees its resources and returns an
error. Calling drm_sched_fini() in the error path could attempt to stop
uninitialized workqueues and free resources again.


[Severity: High]
This is a pre-existing issue, but in 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);
    ...
}

Does this cause an unbounded memory leak on probe deferral?
rocket_device_init() allocates devres tied to the global drm_dev,
which outlives the probe failure. Since rdev is then reset to NULL
during probe teardown, would a subsequent probe retry allocate an
entirely new set of structures without freeing the old ones?


[Severity: High]
This is a pre-existing issue, but in rocket_job.c:rocket_job_init():

drivers/accel/rocket/rocket_job.c:rocket_job_init() {
    ...
	ret = devm_request_threaded_irq(core->dev, core->irq,
					rocket_job_irq_handler,
					rocket_job_irq_handler_thread,
					IRQF_SHARED, dev_name(core->dev),
					core);
    ...
}

Does this shared IRQ handler unconditionally access hardware registers
when clocks and power domains might be disabled? If another device
sharing the IRQ line fires before pm_runtime_resume_and_get() is
called, could rocket_job_irq_handler() immediately execute a
memory-mapped read and cause a kernel panic?


[Severity: High]
This is a pre-existing issue, but in 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;
    ...
}

Is the global rdev static variable left containing an error pointer
if rocket_device_init() fails? On a subsequent probe retry, rdev
would no longer evaluate to NULL, leading directly to a dereference of
the error pointer when accessing rdev->num_cores.

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