Re: [PATCH RFC 01/16] clk: Propagate rate programming failures

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] clk: Undervoltage on partial rate change failure due to ABORT_RATE_CHANGE
--

commit 5ce89a94f0fcfcda3a335ff0e5689b0f216c3fd4
Author: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

clk: Propagate rate programming failures

This patch stops the clock rate-change walk at the first failure and
propagates the error back to the caller. It attempts to keep the topology,
temporary references, and terminal notifications consistent with the outcome.

> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 4d2cc86bfbeb6..95669775f8cd1 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c

[ ... ]

> @@ -2420,13 +2448,20 @@ static struct clk_core *clk_propagate_rate_change(struct clk_core *core,
>  						  unsigned long event)
>  {
>  	struct clk_core *child, *tmp_clk, *fail_clk = NULL;
> +	unsigned long old_rate = core->rate;
> +	unsigned long new_rate = core->new_rate;
>  	int ret = NOTIFY_DONE;
>  
> -	if (core->rate == core->new_rate)
> +	if (event != PRE_RATE_CHANGE)
> +		old_rate = core->old_rate;
> +	if (event == POST_RATE_CHANGE)
> +		new_rate = core->rate;
> +
> +	if (old_rate == new_rate)
>  		return NULL;
>  
>  	if (core->notifier_count) {
> -		ret = __clk_notify(core, event, core->rate, core->new_rate);
> +		ret = __clk_notify(core, event, old_rate, new_rate);

[Severity: High]
When ABORT_RATE_CHANGE is sent, this uses the original old_rate and new_rate
parameters. Will this cause consumers to revert their changes even if the
hardware rate already changed?

[ ... ]

> @@ -2474,53 +2509,83 @@ static void clk_change_rate(struct clk_core *core)
>  	if (!skip_set_rate && core->ops->set_rate)
> -		core->ops->set_rate(core->hw, core->new_rate, best_parent_rate);
> +		ret = core->ops->set_rate(core->hw, core->new_rate,
> +					  best_parent_rate);

[ ... ]

> -		clk_change_rate(child);
> +		ret = clk_change_rate(child);
> +		if (ret)
> +			goto runtime_put;

[Severity: High]
If the hardware rate is successfully changed for the parent here, but a
subsequent child failure aborts the walk, does the parent remain at the
new rate while the software aborts?

[ ... ]

> @@ -2613,7 +2686,14 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
>  	}
>  
>  	/* change the rates */
> -	clk_change_rate(top);
> +	ret = clk_change_rate(top);
> +	if (ret) {
> +		__clk_recalc_rates(top, true, 0);
> +		clk_propagate_rate_change(top, ABORT_RATE_CHANGE);
> +		goto err;
> +	}

[Severity: High]
If clk_change_rate() fails downstream after the parent hardware rate was
already increased, does broadcasting ABORT_RATE_CHANGE here result in
undervoltage?

For example, consumers like cpufreq or existing device notifiers might
receive the abort and revert their PRE_RATE_CHANGE voltage increases,
leaving the hardware running at the high new rate with the low old voltage.

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