Re: [PATCH RFC 03/16] OPP: Retry failed default single-clock transitions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] opp: permanent transition failure due to strict clock rate bounds check
- [High] opp: permanent fast-path breakage via clk_get_rate mismatch
--

commit 45860eb920070f15eb727913a22f9b1eb403210a
Author: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

OPP: Retry failed default single-clock transitions

A failed OPP transition need not leave the clock at the last successful
OPP's frequency. If hardware changed before the error, the default
single-clock path can subsequently skip a necessary retry because its
cached OPP matches the request. It can also choose voltage and dependency
ordering relative to the wrong operating point.

> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 4ccce7b6afcb1..340911d2971e7 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1073,11 +1073,11 @@ _opp_config_clk_single(struct device *dev, struct opp_table *opp_table,
>  	}
>  
>  	ret = clk_set_rate(opp_table->clk, freq);
> +	opp_table->current_rate_single_clk = clk_get_rate(opp_table->clk);
>  	if (ret) {
> +		opp_table->config_dirty_single_clk = true;

[Severity: High]
Does this unconditionally replace the requested and rounded frequency with the
hardware readback?

If the clock driver's .recalc_rate (which drives clk_get_rate) differs even
slightly from .determine_rate (which drives clk_round_rate), the readback
will not strictly equal the requested freq.

>  		dev_err(dev, "%s: failed to set clock rate: %d\n", __func__,
>  			ret);
> -	} else {
> -		opp_table->current_rate_single_clk = freq;
>  	}
>  
>  	return ret;

[ ... ]

> @@ -1297,20 +1319,40 @@ static int _disable_opp_table(struct device *dev, struct opp_table *opp_table)
>  static int _set_opp(struct device *dev, struct opp_table *opp_table,
>  		    struct dev_pm_opp *opp, void *clk_data, bool forced)
>  {
> +	bool default_single = _opp_uses_default_single_clk(opp_table);
> +	struct dev_pm_opp *actual_opp __free(put_opp) = NULL;
>  	struct dev_pm_opp *old_opp;
>  	int scaling_down, ret;
>  
> -	if (unlikely(!opp))
> -		return _disable_opp_table(dev, opp_table);
> +	if (unlikely(!opp)) {

[ ... ]

>  	/* Find the currently set OPP if we don't know already */
>  	if (unlikely(!opp_table->current_opp))
>  		_find_current_opp(dev, opp_table);
>  
>  	old_opp = opp_table->current_opp;
> +	if (default_single && opp_table->config_dirty_single_clk) {
> +		actual_opp = _find_current_opp_strict(opp_table);
> +		if (IS_ERR(actual_opp))
> +			return PTR_ERR(actual_opp);

[Severity: High]
Does propagating this error directly cause a permanent transition failure?

If _find_current_opp_strict queries a gated clock (returning 0) or
a frequency that exceeds the highest registered OPP, it returns
ERR_PTR(-ERANGE).

Unlike the original _find_current_opp, this strict lookup lacks a fallback
and the -ERANGE error propagates here directly. This short-circuits the
hardware transition without clearing config_dirty_single_clk, permanently
sticking the OPP table in a dirty state where all future calls to
dev_pm_opp_set_rate instantly fail.

> +		old_opp = actual_opp;
> +	}
>  
>  	/* Return early if nothing to do */
> -	if (!forced && old_opp == opp && opp_table->enabled) {
> +	if (!forced && !opp_table->config_dirty_single_clk &&
> +	    old_opp == opp && opp_table->enabled) {
>  		dev_dbg_ratelimited(dev, "%s: OPPs are same, nothing to do\n", __func__);
>  		return 0;
>  	}

[ ... ]

> @@ -1460,7 +1513,8 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
>  		 * value of the frequency. In such a case, do not abort but
>  		 * configure the hardware to the desired frequency forcefully.
>  		 */
> -		forced = opp_table->current_rate_single_clk != freq;
> +		forced = opp_table->config_dirty_single_clk ||
> +			 opp_table->current_rate_single_clk != freq;

[Severity: High]
If clk_get_rate diverges from freq as mentioned in _opp_config_clk_single,
will this fast-path condition evaluate to true permanently?

This fast-path relies on strict equality between the cached rate and the
newly rounded freq. If they differ, it bypasses the early return in _set_opp
and forces full hardware reconfigurations on every clock change.

>  	}
>  
>  	return _set_opp(dev, opp_table, opp, &freq, forced);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014-topic-qc_clk_dvfs-v1-0-3041b788e9f7@oss.qualcomm.com?part=3