[PATCH RFC 0/3] clk: Restore the clocks of a provider after a hibernation

[PATCH RFC 0/3] clk: Restore the clocks of a provider after a hibernation

[Abel Vesa]

When a hibernation image is restored, the clock hardware has been through
a power cycle and a boot kernel, while the clock framework in the image
still holds the rates, parents and enable state the consumers asked for
before the image was created. The consumers ask for the same rates again
when they resume, the framework sees no change, and the clocks the boot
kernel did not touch keep whatever the boot firmware left in them.

Whether the kernel that loads the image probed a given controller at
all depends on whether its driver is built in or available from the
initramfs. On the Qualcomm Glymur CRD, the display clock controller
driver is a module that it does not probe, so after the restore the DPU
runs at the 333MHz the firmware configured the PLL for instead of the
717MHz the framework believes it runs at, underflows, and the panel
shows a solid blue screen.

The first patch adds clk_restore_context_dev(), which re-applies the
cached state of the clocks registered by a provider device, parents
first, as an alternative to the global clk_restore_context() that
depends on every clock implementing the restore_context operation. The
second one adds a hibernation restore callback to the Qualcomm clock
controller common code, which redoes the probe time configuration and
restores the clocks, and the third one uses it for the Glymur display
clock controller.

Sent as RFC, as the same applies to every clock controller and I would
like to know whether this is the right level to handle it at before
wiring up the rest. In particular:

- Instead of programming the cached state into the hardware, the
  framework could read the parents and rates back from the hardware
  after the restore, so that the consumers asking for the same rates
  again go through the normal clk_set_rate() path. Would that be
  preferred?

- Programming the cached rate of a clock that is not enabled may not
  work on every clock type, for example on an RCG whose source is off.

- The Qualcomm callback finds its state through devres, which only
  covers drivers that call qcom_cc_really_probe() once per device.

Clocks left enabled by the firmware but not by the framework stay
enabled, and the phase and duty cycle are not restored.

Prepared with help from an LLM-based assistant.

Signed-off-by: Abel Vesa <abel.vesa@oss.qualcomm.com>
---
Abel Vesa (3):
      clk: Add a way to restore the clocks of a provider after a power cycle
      clk: qcom: common: Restore the clock controller after hibernation
      clk: qcom: dispcc-glymur: Restore the clocks after hibernation

 drivers/clk/clk.c                | 68 ++++++++++++++++++++++++++++++++++++++++
 drivers/clk/qcom/common.c        | 52 +++++++++++++++++++++++++++++-
 drivers/clk/qcom/common.h        |  2 ++
 drivers/clk/qcom/dispcc-glymur.c |  2 ++
 include/linux/clk-provider.h     |  2 ++
 5 files changed, 125 insertions(+), 1 deletion(-)
---
base-commit: f1ac44c32cba88ad65dcecd4a279661d14e93651
change-id: 20261023-b4-clk-restore-after-hibernation-74fff1dea5f2

Best regards,
--  

Abel

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

[Abel Vesa]

When a hibernation image is restored, the hardware has been through a
power cycle and a boot kernel, while the clock framework still has the
rates, parents and enable state the consumers asked for before the image
was created. The consumers, when resumed, ask for the same rates again,
which the framework sees as no change and never programs, so the clocks
the boot kernel did not touch keep whatever the boot firmware left in
them.

The existing clk_restore_context() depends on every clock implementing
the restore_context operation and acts on all clocks at once, which does
not fit a provider that can only touch its own registers once its power
domain is back.

Add clk_restore_context_dev(), which re-applies the cached parent, rate
and prepare/enable state to the hardware of the clocks registered by the
given device, parents before children. It is meant for the hibernation
restore callback of a provider, so it is declared in clk-provider.h.

The restore_context operation is not used, as it relies on a previous
save_context, which only clk_save_context() does. Clocks left enabled
by the firmware but not by the framework stay enabled, and the phase
and duty cycle are not restored.

Signed-off-by: Abel Vesa <abel.vesa@oss.qualcomm.com>
---
 drivers/clk/clk.c            | 68 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/clk-provider.h |  2 ++
 2 files changed, 70 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 4d2cc86bfbeb..316c3c85f247 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);
+	}
+
+	if (core->rate && core->ops->set_rate)
+		core->ops->set_rate(core->hw, core->rate,
+				    parent ? parent->rate : 0);
+
+	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);
+	}
+}
+
+static void clk_core_restore_context_dev(struct clk_core *core,
+					 struct device *dev)
+{
+	struct clk_core *child;
+
+	if (core->dev == dev)
+		clk_core_restore_state(core);
+
+	hlist_for_each_entry(child, &core->children, child_node)
+		clk_core_restore_context_dev(child, dev);
+}
+
+/**
+ * clk_restore_context_dev - restore the clocks of a provider after poweroff
+ * @dev: the device that registered the clocks
+ *
+ * Program the cached state of the clocks registered by @dev back into the
+ * hardware, parents before children, after a power cycle the framework does
+ * not know about, such as a hibernation restore.
+ */
+void clk_restore_context_dev(struct device *dev)
+{
+	struct clk_core *core;
+
+	clk_prepare_lock();
+
+	hlist_for_each_entry(core, &clk_root_list, child_node)
+		clk_core_restore_context_dev(core, dev);
+
+	hlist_for_each_entry(core, &clk_orphan_list, child_node)
+		clk_core_restore_context_dev(core, dev);
+
+	clk_prepare_unlock();
+}
+EXPORT_SYMBOL_GPL(clk_restore_context_dev);
+
 /**
  * clk_enable - ungate a clock
  * @clk: the clk being ungated
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index a355d1ee5189..c3a095e8da5a 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -1439,6 +1439,8 @@ void clk_unregister(struct clk *clk);
 
 void clk_hw_unregister(struct clk_hw *hw);
 
+void clk_restore_context_dev(struct device *dev);
+
 /* helper functions */
 const char *__clk_get_name(const struct clk *clk);
 const char *clk_hw_get_name(const struct clk_hw *hw);

-- 
2.54.0

[PATCH RFC 2/3] clk: qcom: common: Restore the clock controller after hibernation

[Abel Vesa]

Nothing puts a clock controller back into the state the clock framework
expects after a hibernation image is restored. The hardware has been
through a power cycle and through the kernel that loaded the image,
which may have programmed other rates and parents, or not probed the
controller at all, depending on whether its driver is built in or
available from the initramfs. The rates and parents the consumers asked
for before the image was created are not programmed again either, since
the framework sees no change.

On the Glymur CRD, the display clock controller driver is a module that
the kernel loading the image does not probe, so the probe time PLL
configuration is missing and the DPU runs at the 333MHz the boot
firmware left the PLL at instead of the 717MHz the framework believes,
underflows, and the panel shows a solid blue screen.

Add a restore callback that redoes the probe time configuration and
restores the cached state of the clocks, for the drivers to use.

Signed-off-by: Abel Vesa <abel.vesa@oss.qualcomm.com>
---
 drivers/clk/qcom/common.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++-
 drivers/clk/qcom/common.h |  2 ++
 2 files changed, 53 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
index 08fd62ac780d..ff243fc78188 100644
--- a/drivers/clk/qcom/common.c
+++ b/drivers/clk/qcom/common.c
@@ -7,6 +7,7 @@
 #include <linux/module.h>
 #include <linux/regmap.h>
 #include <linux/platform_device.h>
+#include <linux/pm.h>
 #include <linux/clk-provider.h>
 #include <linux/interconnect-clk.h>
 #include <linux/pm_runtime.h>
@@ -26,6 +27,8 @@ struct qcom_cc {
 	struct clk_regmap **rclks;
 	size_t num_rclks;
 	struct dev_pm_domain_list *pd_list;
+	const struct qcom_cc_desc *desc;
+	struct regmap *regmap;
 };
 
 const
@@ -321,6 +324,10 @@ static void qcom_cc_clk_regs_configure(struct device *dev, const struct qcom_cc_
 		data->clk_regs_configure(dev, regmap);
 }
 
+static void qcom_cc_release(struct device *dev, void *res)
+{
+}
+
 int qcom_cc_really_probe(struct device *dev,
 			 const struct qcom_cc_desc *desc, struct regmap *regmap)
 {
@@ -333,9 +340,13 @@ int qcom_cc_really_probe(struct device *dev,
 	size_t num_clk_hws = desc->num_clk_hws;
 	struct clk_hw **clk_hws = desc->clk_hws;
 
-	cc = devm_kzalloc(dev, sizeof(*cc), GFP_KERNEL);
+	cc = devres_alloc(qcom_cc_release, sizeof(*cc), GFP_KERNEL);
 	if (!cc)
 		return -ENOMEM;
+	devres_add(dev, cc);
+
+	cc->desc = desc;
+	cc->regmap = regmap;
 
 	ret = devm_pm_domain_attach_list(dev, NULL, &cc->pd_list);
 	if (ret < 0 && ret != -EEXIST)
@@ -434,6 +445,45 @@ int qcom_cc_really_probe(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(qcom_cc_really_probe);
 
+/* Redo the probe time setup and the cached clock state after a power cycle */
+static int qcom_cc_restore(struct device *dev)
+{
+	struct qcom_cc *cc = devres_find(dev, qcom_cc_release, NULL, NULL);
+	const struct qcom_cc_desc *desc;
+	int ret = 0;
+
+	if (!cc)
+		return 0;
+
+	desc = cc->desc;
+
+	if (desc->use_rpm) {
+		ret = pm_runtime_resume_and_get(dev);
+		if (ret)
+			return ret;
+	}
+
+	if (desc->driver_data) {
+		ret = qcom_cc_clk_pll_configure(desc->driver_data, cc->regmap);
+		if (ret)
+			goto put_rpm;
+
+		qcom_cc_clk_regs_configure(dev, desc->driver_data, cc->regmap);
+	}
+
+	clk_restore_context_dev(dev);
+
+put_rpm:
+	if (desc->use_rpm)
+		pm_runtime_put_sync(dev);
+
+	return ret;
+}
+
+EXPORT_GPL_DEV_PM_OPS(qcom_cc_pm_ops) = {
+	.restore = pm_sleep_ptr(qcom_cc_restore),
+};
+
 int qcom_cc_probe(struct platform_device *pdev, const struct qcom_cc_desc *desc)
 {
 	struct regmap *regmap;
diff --git a/drivers/clk/qcom/common.h b/drivers/clk/qcom/common.h
index 6f2406f8839e..227669609a50 100644
--- a/drivers/clk/qcom/common.h
+++ b/drivers/clk/qcom/common.h
@@ -4,6 +4,7 @@
 #ifndef __QCOM_CLK_COMMON_H__
 #define __QCOM_CLK_COMMON_H__
 
+struct dev_pm_ops;
 struct platform_device;
 struct regmap_config;
 struct clk_regmap;
@@ -86,6 +87,7 @@ extern int qcom_cc_really_probe(struct device *dev,
 				struct regmap *regmap);
 extern int qcom_cc_probe(struct platform_device *pdev,
 			 const struct qcom_cc_desc *desc);
+extern const struct dev_pm_ops qcom_cc_pm_ops;
 extern int qcom_cc_probe_by_index(struct platform_device *pdev, int index,
 				  const struct qcom_cc_desc *desc);
 

-- 
2.54.0

[PATCH RFC 3/3] clk: qcom: dispcc-glymur: Restore the clocks after hibernation

[Abel Vesa]

The display clock controller driver is a module that the kernel loading
a hibernation image does not probe, so after the image is restored its
PLLs and RCGs still hold what the boot firmware configured, while the
display consumers believe their rates are set. Use the common restore
callback.

Signed-off-by: Abel Vesa <abel.vesa@oss.qualcomm.com>
---
 drivers/clk/qcom/dispcc-glymur.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/clk/qcom/dispcc-glymur.c b/drivers/clk/qcom/dispcc-glymur.c
index 32cc5226b4de..7ea51b8c8e64 100644
--- a/drivers/clk/qcom/dispcc-glymur.c
+++ b/drivers/clk/qcom/dispcc-glymur.c
@@ -6,6 +6,7 @@
 #include <linux/clk-provider.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/pm.h>
 #include <linux/regmap.h>
 
 #include <dt-bindings/clock/qcom,glymur-dispcc.h>
@@ -1968,6 +1969,7 @@ static struct platform_driver disp_cc_glymur_driver = {
 	.driver = {
 		.name = "dispcc-glymur",
 		.of_match_table = disp_cc_glymur_match_table,
+		.pm = pm_sleep_ptr(&qcom_cc_pm_ops),
 	},
 };
 

-- 
2.54.0

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

[sashiko-bot]

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

Re: [PATCH RFC 2/3] clk: qcom: common: Restore the clock controller after hibernation

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] clk: qcom: common: dangling stack pointer in cc->desc after probe
- [High] clk: qcom: common: qcom_cc_restore() corrupts clk_rcg2_shared_ops state
--

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

clk: qcom: common: Restore the clock controller after hibernation

This commit adds a hibernation restore callback to qcom_cc_pm_ops that
redoes probe-time PLL and register configuration and calls
clk_restore_context_dev() to restore cached clock state after hibernation.

> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
> index 08fd62ac780d3..ff243fc781887 100644
> --- a/drivers/clk/qcom/common.c
> +++ b/drivers/clk/qcom/common.c

[ ... ]

> @@ -333,9 +340,13 @@ int qcom_cc_really_probe(struct device *dev,
>  	size_t num_clk_hws = desc->num_clk_hws;
>  	struct clk_hw **clk_hws = desc->clk_hws;
>  
> -	cc = devm_kzalloc(dev, sizeof(*cc), GFP_KERNEL);
> +	cc = devres_alloc(qcom_cc_release, sizeof(*cc), GFP_KERNEL);
>  	if (!cc)
>  		return -ENOMEM;
> +	devres_add(dev, cc);
> +
> +	cc->desc = desc;
> +	cc->regmap = regmap;

[Severity: High]
Can storing the desc pointer in cc->desc leave a dangling stack pointer after
probe returns?

Several Qualcomm clock controller drivers allocate a local struct qcom_cc_desc
on the stack in their probe functions to customize fields per SoC variant and
pass &desc to qcom_cc_really_probe():

drivers/clk/qcom/camcc-sa8775p.c:cam_cc_sa8775p_probe() {
	struct qcom_cc_desc desc = cam_cc_sa8775p_desc;
	...
		desc.driver_data = &cam_cc_qcs8300_driver_data;
	}

	ret = qcom_cc_really_probe(&pdev->dev, &desc, regmap);
}

drivers/clk/qcom/videocc-sm8350.c:video_cc_sm8350_probe() {
	struct qcom_cc_desc desc = video_cc_sm8350_desc;
	...
		desc.driver_data = &video_cc_sc8280xp_driver_data;
	...
	ret = qcom_cc_really_probe(&pdev->dev, &desc, regmap);
}

drivers/clk/qcom/gcc-ipq5018.c:gcc_ipq5018_probe() {
	struct qcom_cc_desc ipq5018_desc = gcc_ipq5018_desc;
	...
	return qcom_cc_really_probe(&pdev->dev, &ipq5018_desc, regmap);
}

When qcom_cc_restore() later dereferences cc->desc (checking desc->use_rpm and
desc->driver_data), the probe stack frame has already been reclaimed. Would it
be safer to store desc->use_rpm and desc->driver_data directly in struct
qcom_cc instead of retaining the desc pointer?

>  
>  	ret = devm_pm_domain_attach_list(dev, NULL, &cc->pd_list);
>  	if (ret < 0 && ret != -EEXIST)
> @@ -434,6 +445,45 @@ int qcom_cc_really_probe(struct device *dev,
>  }
>  EXPORT_SYMBOL_GPL(qcom_cc_really_probe);
>  
> +/* Redo the probe time setup and the cached clock state after a power cycle */
> +static int qcom_cc_restore(struct device *dev)
> +{
> +	struct qcom_cc *cc = devres_find(dev, qcom_cc_release, NULL, NULL);
> +	const struct qcom_cc_desc *desc;
> +	int ret = 0;
> +
> +	if (!cc)
> +		return 0;
> +
> +	desc = cc->desc;
> +
> +	if (desc->use_rpm) {
> +		ret = pm_runtime_resume_and_get(dev);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (desc->driver_data) {
> +		ret = qcom_cc_clk_pll_configure(desc->driver_data, cc->regmap);
> +		if (ret)
> +			goto put_rpm;
> +
> +		qcom_cc_clk_regs_configure(dev, desc->driver_data, cc->regmap);
> +	}
> +
> +	clk_restore_context_dev(dev);

[Severity: High]
Does calling clk_restore_context_dev() here corrupt the hardware state of
clocks using clk_rcg2_shared_ops (as well as clk_rcg2_shared_floor_ops and
clk_rcg2_shared_no_init_park_ops)?

In clk_core_restore_state(), the clock framework restores a clock by calling
set_parent(), set_rate(), prepare(), and enable() in sequence:

drivers/clk/clk.c:clk_core_restore_state() {
	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);
	}

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

	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);
	}
}

Because clk_rcg2_shared_ops does not implement .is_enabled,
clk_hw_is_enabled(hw) evaluates to (core->enable_count > 0).

When a shared RCG is enabled across hibernation (core->enable_count > 0),
clk_rcg2_shared_set_parent() and __clk_rcg2_shared_set_rate() see
clk_hw_is_enabled(hw) == true and write the new configuration directly to
hardware CFG_REG without updating rcg->parked_cfg:

drivers/clk/qcom/clk-rcg2.c:__clk_rcg2_shared_set_rate() {
	...
	if (!clk_hw_is_enabled(hw))
		return __clk_rcg2_configure(rcg, f, &rcg->parked_cfg);

	return clk_rcg2_shared_force_enable_clear(hw, f);
}

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

drivers/clk/qcom/clk-rcg2.c: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);
	...
}

Since runtime rate and parent changes while the clock was enabled never
updated rcg->parked_cfg, won't clk_rcg2_shared_enable() overwrite CFG_REG with
a stale rcg->parked_cfg and clobber the rate and parent just programmed by
set_parent() and set_rate()?

Conversely, when core->enable_count == 0, clk_core_restore_state() does not
call .init (clk_rcg2_shared_init()) or .disable, and set_parent()/set_rate()
only update rcg->parked_cfg in memory without writing to hardware registers,
leaving any RCG enabled by boot firmware unparked in hardware.

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