Re: [PATCH v3 14/47] clk: qcom: gcc-qdu1000: 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:
- [Low] Missing DT Binding Update for `power-domains` property.

Pre-existing issues:
- [High] System crash (synchronous external abort) due to unclocked register access during `clk_round_rate()` on DFS clocks.
- [High] System crash due to missing PM Runtime protection in reset operations.
--

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

clk: qcom: gcc-qdu1000: 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-qdu1000.c b/drivers/clk/qcom/gcc-qdu1000.c
> index 915bb9b4ff813..172303ac4c9bb 100644
> --- a/drivers/clk/qcom/gcc-qdu1000.c
> +++ b/drivers/clk/qcom/gcc-qdu1000.c
> @@ -2649,6 +2649,7 @@ static const struct qcom_cc_desc gcc_qdu1000_desc = {
>  	.num_resets = ARRAY_SIZE(gcc_qdu1000_resets),
>  	.gdscs = gcc_qdu1000_gdscs,
>  	.num_gdscs = ARRAY_SIZE(gcc_qdu1000_gdscs),
> +	.use_rpm = true,

[Severity: Low]
Does enabling runtime PM here introduce a dependency on a power-domains
property that needs to be documented in the controller's specific binding?
Without updating qcom,qdu1000-gcc.yaml, might this lead to missing DT
constraints or dtbs_check warnings?

[Severity: High]
This isn't a bug introduced by this patch, but enabling runtime PM for this
controller exposes a pre-existing issue in the DFS clocks implementation.

When a consumer driver calls clk_round_rate() on an unprepared DFS clock,
the CCF clk_round_rate() API calls determine_rate() without holding a PM
runtime reference.

If the GCC device is suspended (CX domain off), can this sequence cause a
synchronous external abort due to unclocked register access?

drivers/clk/qcom/clk-rcg2.c:clk_rcg2_dfs_determine_rate() {
    ...
    ret = clk_rcg2_dfs_populate_freq_table(rcg);
    ...
}

drivers/clk/qcom/clk-rcg2.c:clk_rcg2_dfs_populate_freq_table() {
    ...
    regmap_read(rcg->clkr.regmap, rcg->cmd_rcgr + SE_PERF_DFSR(l), &cfg);
    ...
}

[Severity: High]
This is a pre-existing issue, but enabling runtime PM on the QDU1000
controller also exposes a vulnerability in the reset operations.

The reset functions appear to access hardware registers directly without
using pm_runtime_resume_and_get() on the GCC device.

If a consumer asserts or deasserts a reset while the GCC provider is
runtime suspended, will this unclocked register access crash the system?

drivers/clk/qcom/reset.c:qcom_reset_set_assert() {
    ...
    regmap_update_bits(rst->regmap, map->reg, mask, assert ? mask : 0);
    ...
}

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

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