Re: [PATCH RFC 1/3] clk: Add a way to restore the clocks of a provider after a power cycle

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] clk: clk_core_restore_state() ignores set_rate_and_parent callback
- [High] clk: clk_core_restore_state() ignores CLK_OPS_PARENT_ENABLE and UNGATE flags
- [High] clk: clk_core_restore_state() calls set_parent/set_rate before enable
- [High] clk: clk_core_restore_state() passes unvalidated boot rate to set_rate()
- [Medium] clk: clk_core_restore_state() checks enable_count outside clk_enable_lock()
--

commit 78198c18d1a2d0f099c9c205f894c06eba8b44ff
Author: Abel Vesa <abel.vesa@oss.qualcomm.com>

clk: Add a way to restore the clocks of a provider after a power cycle

This commit adds clk_restore_context_dev() to reprogram the cached parent,
rate, and prepare/enable state into hardware for all clocks registered by a
given provider device after a power cycle such as hibernation restore.

> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 4d2cc86bfbeb6..316c3c85f247d 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1449,6 +1449,74 @@ void clk_restore_context(void)
>  }
>  EXPORT_SYMBOL_GPL(clk_restore_context);
>  
> +static int clk_fetch_parent_index(struct clk_core *core,
> +				  struct clk_core *parent);
> +
> +/* Program the cached parent, rate and prepare/enable state into the hardware */
> +static void clk_core_restore_state(struct clk_core *core)
> +{
> +	struct clk_core *parent = core->parent;
> +	unsigned long flags;
> +	int index;
> +
> +	lockdep_assert_held(&prepare_lock);
> +
> +	if (parent && core->num_parents > 1 && core->ops->set_parent) {
> +		index = clk_fetch_parent_index(core, parent);
> +		if (index >= 0)
> +			core->ops->set_parent(core->hw, index);
> +	}

[Severity: High]
What happens here when a clock implements core->ops->set_rate_and_parent
and cannot tolerate separate .set_parent() and .set_rate() calls?

In drivers/clk/clk.c, clk_change_rate() prefers .set_rate_and_parent() when
both parent and rate are updated:

	if (core->ops->set_rate_and_parent) {
		skip_set_rate = true;
		core->ops->set_rate_and_parent(core->hw, core->new_rate,
				best_parent_rate,
				core->new_parent_index);
	} else if (core->ops->set_parent) {
		core->ops->set_parent(core->hw, core->new_parent_index);
	}

In clk_core_restore_context_dev() -> clk_core_restore_state(), calling
core->ops->set_parent() and core->ops->set_rate() separately without
checking core->ops->set_rate_and_parent breaks clocks that rely on the
combined callback. For example, in drivers/clk/qcom/clk-rcg2.c,
clk_gfx3d_set_rate() is a no-op stub:

static int clk_gfx3d_set_rate(struct clk_hw *hw, unsigned long rate,
			      unsigned long parent_rate)
{
	/*
	 * We should never get here; clk_gfx3d_determine_rate() should always
	 * make us use a different parent than what we're currently using, so
	 * clk_gfx3d_set_rate_and_parent() should always be called.
	 */
	return 0;
}

On clk_gfx3d_ops, clk_rcg2_set_parent() only updates CFG_SRC_SEL_MASK and
only clk_gfx3d_set_rate_and_parent() programs the divider cgfx->div into
CFG_SRC_DIV_SHIFT. Similarly, c910_clk_set_rate() in
drivers/clk/thead/clk-th1520-ap.c returns -EOPNOTSUPP, and on standard
Qualcomm RCG2 and composite clocks, switching the parent mux before
reprogramming the divider can cause a transient frequency spike while the
divider still holds its boot value.

Should clk_core_restore_state() use core->ops->set_rate_and_parent() when
both parent and rate are being restored?

[Severity: High]
Also, could calling .set_parent() and .set_rate() here without honoring
CLK_OPS_PARENT_ENABLE and CLK_SET_RATE_UNGATE cause hardware update
timeouts when the parent or clock is currently disabled?

In drivers/clk/clk.c, clk_change_rate() prepares and enables the clock and
its parent around rate and parent programming when those flags are set:

	if (core->flags & CLK_SET_RATE_UNGATE) {
		clk_core_prepare(core);
		clk_core_enable_lock(core);
	}
	...
	if (core->flags & CLK_OPS_PARENT_ENABLE)
		clk_core_prepare_enable(parent);

During hibernation restore, provider restore callbacks run while many
configured clocks and their parents have prepare_count == 0 and
enable_count == 0. Even though clk_restore_context_dev() visits parents
before children, clk_core_restore_state() only enables the parent if
parent->enable_count is non-zero. When clk_core_restore_state() then calls
.set_parent() or .set_rate() on a child with CLK_OPS_PARENT_ENABLE (such as
Qualcomm RCG2 clocks using clk_rcg2_ops), update_config() in
drivers/clk/qcom/clk-rcg2.c cannot complete its hardware handshake while
the parent is gated:

	for (count = 500; count > 0; count--) {
		ret = regmap_read(rcg->clkr.regmap, rcg->cmd_rcgr + CMD_REG, &cmd);
		if (ret)
			return ret;
		if (!(cmd & CMD_UPDATE))
			return 0;
		udelay(1);
	}

	WARN(1, "%s: rcg didn't update its configuration.", name);
	return -EBUSY;

Should clk_core_restore_state() temporarily prepare and enable the parent
(for CLK_OPS_PARENT_ENABLE) and the clock itself (for CLK_SET_RATE_UNGATE)
around the parent and rate restore operations?

> +
> +	if (core->rate && core->ops->set_rate)
> +		core->ops->set_rate(core->hw, core->rate,
> +				    parent ? parent->rate : 0);

[Severity: High]
Is it safe to pass core->rate unconditionally to core->ops->set_rate() if
no consumer ever called clk_set_rate() on this clock?

At registration time in __clk_core_init(), core->rate is initialized from
core->ops->recalc_rate() based on the power-on reset register state (for
example, 19.2 MHz from XO on Qualcomm RCG2 clocks) rather than a rate
validated by .determine_rate().

For clocks using clk_rcg2_shared_floor_ops whose frequency table only
contains PLL frequencies above 19.2 MHz (such as
gcc_sdcc1_ice_core_clk_src in drivers/clk/qcom/gcc-eliza.c, where the table
starts at 100 MHz), passing the 19.2 MHz boot rate into
clk_rcg2_shared_floor_set_rate() -> __clk_rcg2_shared_set_rate() causes
qcom_find_freq_floor() to return NULL:

	switch (policy) {
	case FLOOR:
		f = qcom_find_freq_floor(rcg->freq_tbl, rate);
		break;
	case CEIL:
		f = qcom_find_freq(rcg->freq_tbl, rate);
		break;
	default:
		return -EINVAL;
	}

	if (!clk_hw_is_enabled(hw))
		return __clk_rcg2_configure(rcg, f, &rcg->parked_cfg);

	return clk_rcg2_shared_force_enable_clear(hw, f);

Because __clk_rcg2_shared_set_rate() does not check for a NULL f before
calling __clk_rcg2_configure(), dereferencing f->src triggers a NULL
pointer dereference. On CEIL-policy clocks (clk_rcg2_ops), passing an
unconfigured 19.2 MHz boot rate to clk_rcg2_set_rate() rounds up to the
lowest PLL entry in freq_tbl and switches the hardware mux to that PLL,
desynchronizing the hardware parent and rate from core->parent and
core->rate.

> +
> +	if (core->prepare_count && core->ops->prepare)
> +		core->ops->prepare(core->hw);
> +
> +	if (core->enable_count && core->ops->enable) {
> +		flags = clk_enable_lock();
> +		core->ops->enable(core->hw);
> +		clk_enable_unlock(flags);
> +	}
> +}

[Severity: High]
Won't calling .set_parent() and .set_rate() before .enable() corrupt the
restored configuration on clocks using clk_rcg2_shared_ops when
core->enable_count > 0?

In drivers/clk/clk.c, clk_core_is_enabled() falls back to
core->enable_count when core->ops->is_enabled is not implemented:

	if (!core->ops->is_enabled)
		return core->enable_count;

Because clk_rcg2_shared_ops in drivers/clk/qcom/clk-rcg2.c does not
implement .is_enabled, clk_hw_is_enabled(hw) returns true during
clk_core_restore_state() whenever core->enable_count > 0:

1. clk_rcg2_shared_set_parent() sees clk_hw_is_enabled(hw) == true, so it
   skips updating rcg->parked_cfg and calls clk_rcg2_set_parent() ->
   update_config() without force-enabling the RCG, which times out because
   child branches are not yet restored.

2. clk_rcg2_shared_set_rate() -> __clk_rcg2_shared_set_rate() also sees
   clk_hw_is_enabled(hw) == true, so it writes the rate directly to
   hardware via clk_rcg2_shared_force_enable_clear() without updating
   rcg->parked_cfg:

	if (!clk_hw_is_enabled(hw))
		return __clk_rcg2_configure(rcg, f, &rcg->parked_cfg);

	return clk_rcg2_shared_force_enable_clear(hw, f);

3. Immediately afterwards, clk_core_restore_state() calls
   core->ops->enable(), which invokes clk_rcg2_shared_enable():

	/* Write back the stored configuration corresponding to current rate */
	ret = regmap_write(rcg->clkr.regmap, rcg->cmd_rcgr + CFG_REG, rcg->parked_cfg);
	if (ret)
		return ret;

	ret = update_config(rcg);

This overwrites CFG_REG with the stale rcg->parked_cfg value, clobbering
the rate and parent that .set_parent() and .set_rate() just programmed.

[Severity: Medium]
In addition, can checking core->enable_count before acquiring
clk_enable_lock() race with a concurrent clk_disable()?

In drivers/clk/clk.c, clk_core_disable() decrements core->enable_count and
calls core->ops->disable() under enable_lock without holding prepare_lock:

	if (--core->enable_count > 0)
		return;

	trace_clk_disable(core);

	if (core->ops->disable)
		core->ops->disable(core->hw);

If core->enable_count is 1 when clk_core_restore_state() checks it outside
clk_enable_lock(), and a concurrent clk_disable() runs before
clk_enable_lock() is acquired, core->enable_count drops to 0 and the clock
is disabled, after which clk_core_restore_state() acquires
clk_enable_lock() and unconditionally calls core->ops->enable(core->hw).
Should core->enable_count be checked after acquiring clk_enable_lock(),
similar to clk_disable_unused_subtree()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028-b4-clk-restore-after-hibernation-v1-0-86cf62747a7f@oss.qualcomm.com?part=1