Re: [RFC PATCH v4 3/9] accel: rocket: Add RK3568 SoC support

[sashiko-bot@kernel.org]

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

New issues:
- [High] `pm_runtime_put_sync()` fails to suspend the device synchronously due to active autosuspend, leaking runtime PM state when `noc_init` fails.
- [High] `rk3568_noc_init()` ignores `clk_set_rate()` failures and incorrectly uses short-circuiting logic, allowing initialization to proceed with an inactive compute clock.
- [High] The RK3568 NPU compute clock (PVTPLL) is not restarted during runtime resume, leading to a hardware hang.

Pre-existing issues:
- [High] Uninitialized bulk clock IDs cause the driver to silently fail at managing the NPU's actual clocks, fetching the identical default clock repeatedly.
- [High] `devm_drm_dev_alloc()` memory is leaked on probe deferral because it is allocated on a global facade device.
--

commit 18b5e0abd4d8cdab9122360b8a9bd630b4573645
Author: Midgy BALON <midgy971@gmail.com>

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.  Add rk3568_soc_data and its
compatible.
    
Unlike the RK3588, the RK3568 NPU's compute clock is a PVTPLL managed by
TF-A via SCMI; start it from an noc_init callback with a real rate change
(an intermediate rate defeats the clock framework's unchanged-rate
shortcut).  Powering on and de-idling the NPU NoC are left to the power
domain (genpd), which performs them when the IOMMU supplier is resumed,
so the driver does not poke the PMU directly.
    
If noc_init fails, unwind through rocket_core_fini() so the core is torn
down completely rather than leaking the runtime-PM and IOMMU state.

> diff --git a/drivers/accel/rocket/rocket_core.c b/drivers/accel/rocket/rocket_core.c
> index 09c445af7de73..779e951596a15 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);

[Severity: High]
Does this error path leak the runtime PM state?

Since rocket_core_init() enables autosuspend via pm_runtime_use_autosuspend(),
calling pm_runtime_put_sync() behaves asynchronously and schedules an
autosuspend timer instead of suspending immediately.

Then rocket_core_fini() calls pm_runtime_disable() which cancels pending
timers without suspending the device. Does this leave the device permanently
in the RPM_ACTIVE state and keep the power domain powered on?

> +			rocket_core_fini(core);
> +			return err;
> +		}
> +	}
> +
>  	version = rocket_pc_readl(core, VERSION);
>  	version += rocket_pc_readl(core, VERSION_NUM) & 0xffff;

[ ... ]

> diff --git a/drivers/accel/rocket/rocket_drv.c b/drivers/accel/rocket/rocket_drv.c
> index f0beed2d522c7..86484110ad6f0 100644
> --- a/drivers/accel/rocket/rocket_drv.c
> +++ b/drivers/accel/rocket/rocket_drv.c

[ ... ]

> @@ -223,12 +224,42 @@ static void rocket_remove(struct platform_device *pdev)
>  	}
>  }
>  
> +/*
> + * The NPU compute clock is a PVTPLL managed by TF-A via SCMI; spin it up
> + * with a real rate change (an intermediate rate defeats the clock
> + * framework's unchanged-rate shortcut).  Powering on and de-idling the NPU
> + * NoC are handled by the power domain (genpd) before the NPU is accessed.
> + */
> +static int rk3568_noc_init(struct rocket_core *core)
> +{
> +	struct clk *npu_clk;
> +
> +	npu_clk = of_clk_get_by_name(core->dev->of_node, "npu");
> +	if (IS_ERR(npu_clk))
> +		return dev_err_probe(core->dev, PTR_ERR(npu_clk),
> +				     "failed to get the NPU SCMI clock\n");
> +
> +	if (clk_set_rate(npu_clk, 600000000UL) ||
> +	    clk_set_rate(npu_clk, 1000000000UL))

[Severity: High]
Does this logical OR prevent the second rate change from executing if the
first one fails?

If clk_set_rate() fails for the first call, the short-circuiting logic will
prevent the second clk_set_rate() from running.

> +		dev_warn(core->dev, "failed to set the NPU compute clock rate\n");
> +	clk_put(npu_clk);
> +
> +	return 0;

[Severity: High]
Should this return an error code if the clock rate fails to set?

Returning 0 here allows initialization to proceed even if the compute clock
is inactive. When rocket_core_init() subsequently reads the NPU VERSION
register over the AXI bus, will this cause a system hang due to the
inactive clock?

> +}

[Severity: High]
This isn't a bug introduced by this patch, but does rocket_core_init() fail
to properly initialize the bulk clock IDs?

When rdev->cores is allocated with devm_kcalloc(), the core->clks array is
initialized with NULL pointers for the .id fields. 

When rocket_core_init() later fetches the clocks:

	err = devm_clk_bulk_get(dev, ARRAY_SIZE(core->clks), core->clks);

With NULL .id strings, clk_get() will repeatedly fetch the first clock listed
in the device tree's clocks property for all elements. Does this leave the
other NPU clocks unmanaged and potentially disabled, leading to AXI bus
hangs?

[Severity: High]
This isn't a bug introduced by this patch, but is there a memory leak in the
probe deferral path for devm_drm_dev_alloc()?

In rocket_device_init(), rdev is allocated on the global drm_dev:

    rdev = devm_drm_dev_alloc(drm_dev->dev, ...);

If rocket_core_init() fails (e.g. returning -EPROBE_DEFER), rocket_probe()
clears the rdev pointer but the devm resources are tied to the static global
drm_dev which is never removed. 

When probe is retried, does this result in allocating a new struct
rocket_device and leaking the previous allocation permanently?

[Severity: High]
Does the driver fail to restart the NPU compute clock during runtime resume?

During runtime suspend, the NPU power domain (DOMAIN_RK3568) is powered off
and the PVTPLL loses state. 

On runtime resume, rocket_device_runtime_resume() enables the bulk clocks:

    err = clk_bulk_prepare_enable(ARRAY_SIZE(rdev->cores[core].clks),
                                  rdev->cores[core].clks);

But it doesn't re-apply the rate-change sequence used in rk3568_noc_init() to
force TF-A to spin the clock up. Will this cause the hardware to hang on the
next NPU access because the compute clock remains off?

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