Re: [PATCH v3 05/15] clk: qcom: clk-rpmh: Add support for Kuno RPMh clocks

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

Pre-existing issues:
- [High] Probe function mutates globally shared static state, causing accumulative corruption on probe retry or unbind/bind.
--

--- Patch [5]: [PATCH v3 05/15] clk: qcom: clk-rpmh: Add support for Kuno RPMh clocks ---
commit 6a55939d348aa54a2946c4b7160f00630d3c9710
Author: Hardeep Sharma <[email protected]>

clk: qcom: clk-rpmh: Add support for Kuno RPMh clocks

Add the RPMh clock description for the Qualcomm Kuno SoC so the
qcom,kuno-rpmh-clk compatible can provide the RPMh-managed clocks to
consumers: the CXO div-2 (and always-on variant), RF clock 1 (and
always-on variant), the QPIC BCM clock and the IPA clock.

> diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c
> index 6367b2a5a4e08..d8c3a6b12d4a4 100644
> --- a/drivers/clk/qcom/clk-rpmh.c
> +++ b/drivers/clk/qcom/clk-rpmh.c

[ ... ]

> @@ -943,6 +943,20 @@ static const struct clk_rpmh_desc clk_rpmh_kaanapali = {
>  	.num_clks = ARRAY_SIZE(kaanapali_rpmh_clocks),
>  };
>  
> +static struct clk_hw *kuno_rpmh_clocks[] = {
> +	[RPMH_CXO_CLK]		= &clk_rpmh_bi_tcxo_div2.hw,
> +	[RPMH_CXO_CLK_A]	= &clk_rpmh_bi_tcxo_div2_ao.hw,
> +	[RPMH_RF_CLK1]		= &clk_rpmh_rf_clk1_a.hw,
> +	[RPMH_RF_CLK1_A]	= &clk_rpmh_rf_clk1_a_ao.hw,
> +	[RPMH_QPIC_CLK]		= &clk_rpmh_qpic_clk.hw,
> +	[RPMH_IPA_CLK]		= &clk_rpmh_ipa.hw,
> +};

[Severity: High]
This is a pre-existing issue, but does adding this new clock array expose it
to state corruption during probe retries or unbind/bind sequences?

Looking at clk_rpmh_probe() in drivers/clk/qcom/clk-rpmh.c, if a clock
resource is missing or if probe fails transitorily and is retried, the
function mutates the statically allocated array:

drivers/clk/qcom/clk-rpmh.c:clk_rpmh_probe() {
    ...
    hw_clks[i] = NULL;
    ...
    rpmh_clk->res_addr += res_addr;
    rpmh_clk->dev = &pdev->dev;
    ...
}

Since kuno_rpmh_clocks is a globally shared static structure, writing NULL here
permanently erases the clock definition for any future bindings.

Additionally, modifying res_addr accumulates the hardware address on subsequent
probe attempts, leading to corrupted register offsets.

Could we avoid mutating these static clock structures by allocating a per-device
copy during probe, or isolating the runtime state from the static clock
descriptions?

> +
> +static const struct clk_rpmh_desc clk_rpmh_kuno = {
> +	.clks = kuno_rpmh_clocks,
> +	.num_clks = ARRAY_SIZE(kuno_rpmh_clocks),
> +};
> +

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5