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

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The patch attempts to allow the CX power domain to power down when idle by enabling runtime PM for the GCC controller, but critical clocks defined in the driver will keep the controller permanently active.
- [Medium] Enabling runtime PM on a clock controller with `CLK_IS_CRITICAL` clocks causes a permanent runtime PM reference leak on probe failure or driver unbind.

Pre-existing issues:
- [High] Reset operations lack runtime PM protection, leading to unpowered register accesses if the controller suspends.
--

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

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

Enable 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-sdm845.c b/drivers/clk/qcom/gcc-sdm845.c
> index 57d54b45fa527..cde863dcf1851 100644
> --- a/drivers/clk/qcom/gcc-sdm845.c
> +++ b/drivers/clk/qcom/gcc-sdm845.c
> @@ -3959,6 +3959,7 @@ static const struct qcom_cc_desc gcc_sdm845_desc = {
>  	.num_resets = ARRAY_SIZE(gcc_sdm845_resets),
>  	.gdscs = gcc_sdm845_gdscs,
>  	.num_gdscs = ARRAY_SIZE(gcc_sdm845_gdscs),
> +	.use_rpm = true,

[Severity: Medium]
Does enabling runtime PM actually allow the controller to suspend given
the CLK_IS_CRITICAL clocks defined in this driver?

For example, gcc_camera_ahb_clk is defined with the CLK_IS_CRITICAL flag.
During clock registration, the clock framework prepares these always-on
clocks and calls pm_runtime_resume_and_get() on the provider. Since these
clocks are never unprepared, the usage count will never drop to zero,
which seems to prevent the CX power domain from ever powering down.

[Severity: Medium]
Additionally, if qcom_cc_really_probe() fails later (for instance, during
qcom_cc_icc_register), devres will clean up by calling clk_unregister().

Since clk_unregister() intentionally does not unprepare clocks, does this
leave a permanent runtime PM reference leak for any clocks marked with 
CLK_IS_CRITICAL that were registered before the failure?

[Severity: High]
This isn't a bug introduced by this patch, but enabling runtime PM
exposes a potential crash during reset operations.

In drivers/clk/qcom/reset.c, qcom_reset_set_assert() accesses hardware
registers without taking a runtime PM reference:

qcom_reset_set_assert() {
    ...
    regmap_update_bits(rst->regmap, map->reg, mask, assert ? mask : 0);
    ...
}

If the controller successfully suspends, and a consumer driver asserts a 
reset before explicitly preparing its clocks, will this access unpowered 
hardware and cause a synchronous external abort?

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

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