Re: [PATCH v2 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 deferral or driver unbind/rebind permanently corrupts RPMh clock resource addresses and structures.
--

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

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

This commit adds the RPMh clock description for the Qualcomm Kuno SoC. It
provides the RPMh-managed clocks to consumers via the qcom,kuno-rpmh-clk
compatible.

> 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 static array expose
the Kuno SoC to a permanent corruption bug during probe deferral?

In clk_rpmh_probe(), if a resource is missing, the driver modifies the
global static SoC clock array permanently:

drivers/clk/qcom/clk-rpmh.c:clk_rpmh_probe() {
    ...
    if (!has_state) {
        hw_clks[i] = NULL;
        continue;
    }
    ...
}

Because kuno_rpmh_clocks[] is a globally shared static array, wouldn't a
probe deferral or an unbind/rebind cycle permanently drop missing resources
from all future probe attempts?

Additionally, clk_rpmh_probe() mutates the global static clock structures
themselves:

drivers/clk/qcom/clk-rpmh.c:clk_rpmh_probe() {
    ...
    rpmh_clk->res_addr += res_addr;
    ...
}

If devm_clk_hw_register() returns -EPROBE_DEFER after this addition,
would the next probe attempt add the base address again, resulting in an
accumulated incorrect offset and potentially causing out-of-bounds RPMh
transactions?

> +
> +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