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

[sashiko-bot@kernel.org]

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