Re: [PATCH] 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] This problem wasn't introduced by this patch, but probe failure or probe deferral causes a devres memory leak on the global facade device (drm_dev).
- [High] This problem wasn't introduced by this patch, but the interrupt handler leaks an IOMMU group reference on every job completion.
--

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

This commit updates the rocket driver to explicitly request the core
clocks by name (aclk, hclk, npu, pclk). This ensures the correct clocks
are prepared and enabled during runtime power management, preventing a
latent issue where the driver held no reference to the actual NPU clock.

> 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);

[Severity: High]
This isn't a bug introduced by this patch, but does the increased likelihood
of probe deferral here expose a devres memory leak?

In rocket_device_init(), rdev and rdev->cores are allocated via devm_ on the
global drm_dev:

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

If devm_clk_bulk_get() returns -EPROBE_DEFER, rocket_probe() cleans up by
calling rocket_device_fini() and nullifies rdev:

drivers/accel/rocket/rocket_drv.c:rocket_probe() {
    ...
    err = rocket_core_init(core);
    if (err) {
        rocket_device_fini(rdev);
        rdev = NULL;
        ...
    }
    ...
}

Because drm_dev is a global platform device that is never unbound, the devres
allocations remain attached to it. On the next probe retry, new allocations
are made, leaking the previous ones.

[Severity: High]
This problem wasn't introduced by this patch, but does the interrupt handler
leak an IOMMU group reference on every job completion?

Looking at rocket_job_handle_irq(), when a job finishes, it detaches the
IOMMU group:

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

The iommu_group_get() function increments the IOMMU group's reference count,
but a matching iommu_group_put() is never called. Because this is a hot path
triggered by hardware interrupts, the reference count will quickly overflow,
permanently leaking the structure. 

Could the driver use its cached core->iommu_group pointer here instead, 
similar to how it is handled in rocket_reset()?

>  	if (err)
>  		return dev_err_probe(dev, err, "failed to get clocks for core %d\n", core->index);

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