Re: [PATCH] clk: qcom: smd-rpm: Skip proxy votes on clocks for QCM2290

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] `clk_smd_rpm_set_rate` silently ignores rate updates when the clock is disabled, leading to incorrect hardware state upon enable.
--

commit 8065b2a650b0e543b5eb4069b2bca8805e5bbb71
Author: Imran Shaik <imran.shaik@oss.qualcomm.com>

    clk: qcom: smd-rpm: Skip proxy votes on clocks for QCM2290
    
    clk_smd_rpm_handoff() votes both active and sleep RPM resource states for
    every clock, keeping them non-zero until a consumer takes over. If there
    is no consumer, those clocks will remain active in the idle scenario as
    well, and the sleep vote is never cleared, blocking XO shutdown.
    
    Introduce the skip_clks_handoff flag to handle this on QCM2290 clocks,
    keeping other targets unaffected.

> diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c
> index 487d715ed5d21..b78c8cc730b59 100644
> --- a/drivers/clk/qcom/clk-smd-rpm.c
> +++ b/drivers/clk/qcom/clk-smd-rpm.c

[ ... ]

> @@ -1358,13 +1360,15 @@ static int rpm_smd_clk_probe(struct platform_device *pdev)
>  			goto err;
>  	}
>  
> -	for (i = 0; i < num_clks; i++) {
> -		if (!rpm_smd_clks[i])
> -			continue;
> +	if (!desc->skip_clks_handoff) {
> +		for (i = 0; i < num_clks; i++) {
> +			if (!rpm_smd_clks[i])
> +				continue;
>  
> -		ret = clk_smd_rpm_handoff(rpm_smd_clks[i]);
> -		if (ret)
> -			goto err;
> +			ret = clk_smd_rpm_handoff(rpm_smd_clks[i]);
> +			if (ret)
> +				goto err;
> +		}
>  	}
>  

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential issue in
how rate updates are handled for disabled clocks. In clk_smd_rpm_set_rate():

drivers/clk/qcom/clk-smd-rpm.c:clk_smd_rpm_set_rate() {
	...
	if (!r->enabled)
		return 0;
	...
	r->rate = rate;

	return 0;
}

If a consumer calls clk_set_rate() on a disabled clock and then calls
clk_prepare_enable(), doesn't the early return in clk_smd_rpm_set_rate() skip
saving the requested rate?

Since r->rate = rate is bypassed, wouldn't the subsequent enable operation
transmit the stale initialization rate to the RPM hardware instead of the newly
requested rate?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910-clk-smd-rpm-skip-proxy-v1-1-1cb5694a99d9@oss.qualcomm.com?part=1