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

[sashiko-bot@kernel.org]

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

New issues:
- [High] The patch adds `.use_rpm = true` to allow the CX power domain to power down when idle, but `CLK_IS_CRITICAL` clocks permanently prevent the device from suspending, entirely defeating the patch's goal.
- [High] Enabling runtime PM introduces a permanent PM runtime usage count leak on probe deferral.

Pre-existing issues:
- [High] Missing `sync_state` callback causes GDSCs initialized as active by the bootloader to stay powered on indefinitely, defeating the patch's purpose of powering down CX.
- [High] If `gdsc_init()` or `pm_genpd_add_subdomain()` fails in `gdsc_register()`, previously initialized generic PM domains are not removed, leading to a Use-After-Free.
--

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

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

This commit sets use_rpm to true in the gcc_sdm670_desc configuration to
enable runtime PM for the controller. The goal is to allow the CX power
domain to be powered down when all consumers have released their clocks and
GDSCs.

> diff --git a/drivers/clk/qcom/gcc-sdm845.c b/drivers/clk/qcom/gcc-sdm845.c
> index 6d0f9cede5cfe..57d54b45fa527 100644
> --- a/drivers/clk/qcom/gcc-sdm845.c
> +++ b/drivers/clk/qcom/gcc-sdm845.c
> @@ -3948,6 +3948,7 @@ static const struct qcom_cc_desc gcc_sdm670_desc = {
>  	.num_resets = ARRAY_SIZE(gcc_sdm845_resets),
>  	.gdscs = gcc_sdm670_gdscs,
>  	.num_gdscs = ARRAY_SIZE(gcc_sdm670_gdscs),
> +	.use_rpm = true,

[Severity: High]
Does setting use_rpm to true actually allow the device to suspend?

When use_rpm is true, clk_core_prepare() takes a PM runtime vote on the GCC
device. However, critical clocks are permanently prepared at registration:

drivers/clk/qcom/gcc-sdm845.c:gcc_camera_ahb_clk {
	...
	.flags = CLK_IS_CRITICAL,
}

Since critical clocks are never unprepared, the runtime PM vote is never
dropped, which appears to keep the CX power domain permanently active. Does
this prevent the patch from achieving its power-savings objective?


[Severity: High]
Will enabling runtime PM here introduce a usage count leak on probe deferral?

Critical clocks take a PM runtime reference during registration. If probe
defers later (for example in qcom_cc_icc_register()), the clocks are
unregistered via clk_unregister().

However, clk_unregister() doesn't unprepare critical clocks or drop the PM
runtime reference:

drivers/clk/clk.c:clk_unregister() {
	if (clk->core->prepare_count)
		pr_warn("%s: unregistering prepared clock: %s\n",
				__func__, clk->core->name);
	...
}

Does this lead to permanently unbalanced usage counts every time probe defers?

>  };
>  
>  static const struct qcom_cc_desc gcc_sdm845_desc = {

[Severity: High]
This isn't a bug introduced by this patch, but does the lack of a sync_state
callback in gcc_sdm845_driver prevent the CX power domain from powering down?

When GDSCs are initialized as active by the bootloader, pm_genpd_init() is
called with is_off=false, and the genpd framework sets stay_on=true. This
flag is only cleared when the device driver's sync_state callback fires.

Since gcc_sdm845_driver lacks a sync_state callback (like
of_genpd_sync_state), the stay_on flag is never cleared:

drivers/clk/qcom/gcc-sdm845.c:gcc_sdm845_driver {
	.probe		= gcc_sdm845_probe,
	.driver		= {
		.name	= "gcc-sdm845",
		.of_match_table = gcc_sdm845_match_table,
	},
}

Could this cause active GDSCs to stay powered on indefinitely?


[Severity: High]
This is a pre-existing issue, but can the error path in gdsc_register() lead
to a use-after-free?

If gdsc_init() or pm_genpd_add_subdomain() fails during the loop in
gdsc_register(), it exits immediately and only cleans up subdomains:

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

	return ret;
}

The error path fails to call pm_genpd_remove() for the previously
initialized generic PM domains, leaving them in the global gpd_list. If the
module is later unloaded, the statically allocated GDSC structures are
freed, which might cause a kernel panic on the next gpd_list traversal.

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