Re: [PATCH v3 24/47] clk: qcom: gcc-sm8150: Tie the CX power domain to controller

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Enabling runtime PM is ineffective because existing critical clocks permanently hold the runtime PM usage counter.

Pre-existing issues:
- [High] Memory leak and Use-After-Free if `gdsc_register()` fails during probe.
--

--- Patch [24]: [PATCH v3 24/47] clk: qcom: gcc-sm8150: Tie the CX power domain to controller ---
commit ff2aa55fcb39d64d3dce326e500a4af7e5279711
Author: Abel Vesa <abel.vesa@oss.qualcomm.com>

clk: qcom: gcc-sm8150: Tie the CX power domain to controller

This commit enables runtime PM for the controller, allowing CX to be powered
down once all GCC consumers have released their clocks and GDSCs.

> diff --git a/drivers/clk/qcom/gcc-sm8150.c b/drivers/clk/qcom/gcc-sm8150.c
> index a93d1f412a7b6..1404996d33c31 100644
> --- a/drivers/clk/qcom/gcc-sm8150.c
> +++ b/drivers/clk/qcom/gcc-sm8150.c
> @@ -3771,6 +3771,7 @@ static const struct qcom_cc_desc gcc_sm8150_desc = {
>  	.num_resets = ARRAY_SIZE(gcc_sm8150_resets),
>  	.gdscs = gcc_sm8150_gdscs,
>  	.num_gdscs = ARRAY_SIZE(gcc_sm8150_gdscs),
> +	.use_rpm = true,

[Severity: Medium]
Does this actually allow the device to suspend?

The GCC driver registers multiple clocks with the CLK_IS_CRITICAL flag,
such as gcc_disp_ahb_clk. During registration, the clock core automatically
calls clk_core_prepare() on these clocks, which in turn calls
clk_pm_runtime_get() and increments the provider device's runtime PM usage
counter. 

Since these clocks are never unprepared, won't the usage counter never
drop to zero, preventing the device from entering RPM_SUSPENDED and keeping
the CX power domain permanently active?

[Severity: High]
This isn't a bug introduced by this patch, but is there a memory leak and
use-after-free risk if driver probe fails in gdsc_register()?

In drivers/clk/qcom/gdsc.c:gdsc_register(), gdsc_init() links the domain
into the global gpd_list via pm_genpd_init().

If a subsequent iteration fails:

drivers/clk/qcom/gdsc.c:gdsc_register() {
    ...
		ret = gdsc_init(scs[i]);
		if (ret)
			return ret;
    ...
}

Or if subdomain addition or provider registration fails:

drivers/clk/qcom/gdsc.c:gdsc_register() {
    ...
err_pm_subdomain_remove:
	gdsc_pm_subdomain_remove(desc, i);

	return ret;
}

The function returns without unregistering the GDSCs initialized in previous
iterations (pm_genpd_remove() is not called). Because the cleanup action is
only added on success via devm_add_action_or_reset(), do the successfully
initialized GDSCs remain in the global gpd_list?

If the module is later unloaded, could this leave dangling pointers in the
global list and cause a kernel panic upon the next traversal?

>  };
>  
>  static const struct of_device_id gcc_sm8150_match_table[] = {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260824-qcom-tie-cx-to-gcc-v3-0-f1205a92682b@oss.qualcomm.com?part=24