Re: [PATCH RFC 02/16] clk: Acquire resources for clock transitions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] clk: ABORT_RATE_CHANGE sent after successful hardware rate transition
- [High] clk: WARN_ON_ONCE(1) in clk_nodrv_ops during clk_unregister
- [High] clk: Missing PM runtime management in resource acquire/release callbacks
- [Medium] clk: permanent resource_fault on clocks lacking acquire ops
--

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

clk: Acquire resources for clock transitions

This commit updates the Common Clock Framework to acquire resources for the
affected tree and parent paths before executing clock transitions. It defers
reconciliation until the tree walk completes to ensure safe fault recovery.
It also modifies parent-change failure paths to retain references explicitly.

> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 95669775f8cd1..46e01d8254e5b 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
[ ... ]
> @@ -2444,52 +2517,270 @@ static struct clk_core *clk_calc_new_rates(struct clk_core *core,
[ ... ]
> +static int
> +clk_resource_release(struct clk_resource_transaction *tx,
> +		     enum clk_resource_change_result result)
> +{
> +	struct clk_resource_acquisition *acquisition, *tmp;
> +	int first_error = 0;
> +	int ret;
> +
> +	list_for_each_entry_reverse(acquisition, &tx->acquisitions, node) {
> +		struct clk_core *core = acquisition->core;
> +
> +		if (result == CLK_RESOURCE_HW_CHANGED)
> +			core->resource_fault = true;
> +
> +		acquisition->transition.actual_rate = core->rate;
> +		acquisition->transition.actual_parent = core->parent ?
> +			core->parent->hw : NULL;
> +
> +		ret = core->ops->resource_release(core->hw,
> +						  &acquisition->transition,
> +						  acquisition->cookie,
> +						  result);

[Severity: High]
Does this code call resource_release() without PM runtime management?

clk_resource_acquire_tree() and clk_resource_release() invoke hardware ops on
descendant and ancestor clocks without invoking clk_pm_runtime_get() on them
first.

For descendants, clk_pm_runtime_get(child) is only called later inside
clk_change_rate(), and it is dropped as soon as clk_change_rate() unwinds.
Therefore, resource_release() will execute on children that no longer hold
an active PM runtime reference, potentially leading to unclocked/unpowered
register accesses and system faults.

> +		if (ret) {
> +			core->resource_fault = true;
> +			if (!first_error)
> +				first_error = ret;
> +		} else if (result == CLK_RESOURCE_COMPLETE) {
> +			core->resource_fault = false;
> +		}
> +	}

[Severity: Medium]
Will clocks lacking resource_acquire() ops get permanently tainted with
resource_fault = true?

When a clock transition fails resource acquisition in a parent path, the
explicitly targeted clock is unconditionally marked with
core->resource_fault = true in clk_core_set_rate_nolock() (e.g. at the
acquire_failed label).

If that target clock doesn't implement ops->resource_acquire(), it isn't
added to the transaction's tx.acquisitions list. Later, if a transition on a
different node succeeds, it iterates over tx.acquisitions to clear the fault
flag. Since the initially failed clock isn't in that list and isn't the new
target, its resource_fault flag stays true permanently, which bypasses
short-circuiting optimizations for it and all its ancestors.

> +
> +	list_for_each_entry_safe(acquisition, tmp, &tx->acquisitions, node) {
> +		list_del(&acquisition->node);
> +		kfree(acquisition);
> +	}
> +
> +	return first_error;
> +}
[ ... ]
> @@ -2649,8 +2951,14 @@ static unsigned long clk_core_req_round_rate_nolock(struct clk_core *core,
>  static int clk_core_set_rate_nolock(struct clk_core *core,
>  				    unsigned long req_rate)
>  {
[ ... ]
>  	/* change the rates */
> -	ret = clk_change_rate(top);
> +	ret = clk_change_rate(top, resource_managed);
>  	if (ret) {
>  		__clk_recalc_rates(top, true, 0);
> -		clk_propagate_rate_change(top, ABORT_RATE_CHANGE);
> +		clk_finish_rate_notifications(&tx, ABORT_RATE_CHANGE);
> +		release_ret = clk_resource_release(&tx,
> +						    CLK_RESOURCE_HW_CHANGED);
> +		if (release_ret)
> +			ret = release_ret;
>  		goto err;
>  	}
>  
> -	clk_propagate_rate_change(top, POST_RATE_CHANGE);
> +	ret = clk_resource_release(&tx, CLK_RESOURCE_COMPLETE);
> +	if (ret) {
> +		core->resource_fault = true;
> +		clk_finish_rate_notifications(&tx, ABORT_RATE_CHANGE);
> +		goto err;
> +	}

[Severity: Critical]
Is it intentional to send ABORT_RATE_CHANGE after the hardware rate has already
been successfully changed?

If clk_change_rate() succeeds, the clock is running at the new rate. If
clk_resource_release() subsequently fails, the code branches to send
ABORT_RATE_CHANGE.

Consumers (like voltage regulators or displays) receive this abort notification
and revert their hardware state to match the old clock rate (e.g., dropping
operating voltage). Because the clock hardware actually remains running at the
new, potentially higher rate, this creates a severe inconsistency where a
component operates at a high frequency with an inadequate voltage.

This identical pattern also appears in clk_core_set_parent_nolock().

> +	core->resource_fault = false;
> +
> +	clk_finish_rate_notifications(&tx, POST_RATE_CHANGE);
>  
>  	core->req_rate = req_rate;
>  err:
> +	clk_clear_new_child(top);
>  	clk_pm_runtime_put(core);
>  
>  	return ret;
> +
> +acquire_failed:
> +	core->resource_fault = true;
> +	release_ret = clk_resource_release(&tx,
> +					    CLK_RESOURCE_NO_HW_CHANGE);
> +	if (release_ret)
> +		ret = release_ret;
> +	goto err;

[ ... ]
> @@ -4776,14 +5221,25 @@ void clk_unregister(struct clk *clk)
>  
>  		/* Reparent all children to the orphan list. */
>  		hlist_for_each_entry_safe(child, t, &clk->core->children,
> -					  child_node)
> +					  child_node) {
> +			if (child->parent_fault) {
> +				pr_warn("%s: releasing unresolved parent fault for %s\n",
> +					__func__, child->name);
> +				clk_release_parent_fault(child);
> +			}
>  			clk_core_set_parent_nolock(child, NULL);
> +		}
>  	}
>  
>  	clk_core_evict_parent_cache(clk->core);
>  
>  	hash_del(&clk->core->hashtable_node);
>  	hlist_del_init(&clk->core->child_node);
> +	if (clk->core->parent_fault) {
> +		pr_warn("%s: releasing unresolved parent fault for %s\n",
> +			__func__, clk->core->name);
> +		clk_release_parent_fault(clk->core);
> +	}

[Severity: High]
Does this code trigger a WARN_ON_ONCE(1) due to invoking
clk_release_parent_fault() after operations have been neutralized?

Earlier in clk_unregister(), clk->core->ops is replaced with &clk_nodrv_ops to
neutralize hardware accesses.

When clk_release_parent_fault() is subsequently called here (or in the
child reparenting loop above), it cleans up retained migration references by
calling clk_core_disable_unprepare() on the faulted old or new parent, which
is the clock currently being unregistered.

Because the operations pointer has already been replaced, this call routes to
clk_nodrv_disable_unprepare(), which triggers an unconditional WARN_ON_ONCE(1).
It looks like the fault cleanup needs to occur before the ops pointer is
substituted.

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