Re: [PATCH v12 2/2] clk: qcom: Add LPASS VA CSR heartbeat pulse clock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] clk: qcom: enable runtime PM after PM clocks in lpass_va_csr_probe()
- [Medium] clk: qcom: lpass-va-csr: stale REGCACHE_MAPLE state across runtime suspend
--

commit 5fda051342340a0aea8fc87d53199c8e2fe790df
Author: Sarath Ganapathiraju <sarath.ganapathiraju@oss.qualcomm.com>

clk: qcom: Add LPASS VA CSR heartbeat pulse clock

This commit adds the Qualcomm LPASS VA CSR clock driver, modeling the
hardware rate generator as a clock provider that manages LPASS vote clocks
and restores counter and delay registers via runtime PM.

> diff --git a/drivers/clk/qcom/lpass-va-csr.c b/drivers/clk/qcom/lpass-va-csr.c
> new file mode 100644
> index 0000000000000..e5fb870aa36de
> --- /dev/null
> +++ b/drivers/clk/qcom/lpass-va-csr.c
> @@ -0,0 +1,180 @@

[ ... ]

> +#define LPASS_RATE_GEN_CTRL		0xd000
> +#define LPASS_RATE_GEN_COUNTER_0	0xd004
> +#define LPASS_RATE_GEN_DELAY		0xd010
> +
> +#define LPASS_RATE_GEN_MAX_REG		LPASS_RATE_GEN_DELAY
> +
> +#define LPASS_RG_CTRL_EN		BIT(0)

[ ... ]

> +static const struct regmap_config lpass_rate_gen_regmap_config = {
> +	.name = "lpass_rate_gen",
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = 4,
> +	.max_register = LPASS_RATE_GEN_MAX_REG,
> +	.cache_type = REGCACHE_MAPLE,
> +};
> +
> +struct lpass_va_csr {
> +	const struct lpass_va_csr_data *data;
> +	struct clk_regmap hb;
> +};
> +
> +static const struct clk_ops lpass_va_csr_hb_ops = {
> +	.enable = clk_enable_regmap,
> +	.disable = clk_disable_regmap,
> +	.is_enabled = clk_is_enabled_regmap,
> +};
> +
> +static int lpass_va_csr_runtime_resume(struct device *dev)
> +{
> +	struct lpass_va_csr *csr = dev_get_drvdata(dev);
> +	const struct lpass_va_csr_data *data = csr->data;
> +	int ret;
> +
> +	ret = pm_clk_resume(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(csr->hb.regmap, LPASS_RATE_GEN_COUNTER_0,
> +			   data->counter_0);
> +	if (ret)
> +		goto err_pm_clk_suspend;
> +
> +	ret = regmap_write(csr->hb.regmap, LPASS_RATE_GEN_DELAY, data->delay);
> +	if (ret)
> +		goto err_pm_clk_suspend;
> +
> +	return 0;
> +
> +err_pm_clk_suspend:
> +	pm_clk_suspend(dev);
> +	return ret;
> +}
> +
> +static int lpass_va_csr_runtime_suspend(struct device *dev)
> +{
> +	return pm_clk_suspend(dev);
> +}

[Severity: Medium]
Can setting .cache_type = REGCACHE_MAPLE in lpass_rate_gen_regmap_config
cause clk_enable_regmap() and clk_is_enabled_regmap() to use stale cached
state for LPASS_RATE_GEN_CTRL after LPASS register context is lost across
runtime suspend?

Neither lpass_va_csr_runtime_suspend() nor lpass_va_csr_runtime_resume()
marks the regcache dirty or drops cached registers, and
lpass_va_csr_runtime_resume() only rewrites LPASS_RATE_GEN_COUNTER_0 and
LPASS_RATE_GEN_DELAY.

If LPASS_RATE_GEN_CTRL has LPASS_RG_CTRL_EN set in hardware when first read
(for example, when loaded as a module or booted with clk_ignore_unused and
inspected via debugfs clk_summary -> clk_core_is_enabled() ->
clk_is_enabled_regmap() -> regmap_read()), _regmap_read() caches that
enabled bit in the maple cache.

If the device then runtime-suspends and loses LPASS register context,
hardware resets LPASS_RATE_GEN_CTRL to 0 while the cache retains the set
bit. On a subsequent clk_prepare_enable(), clk_enable_regmap() in
drivers/clk/qcom/clk-regmap.c calls regmap_update_bits():

drivers/clk/qcom/clk-regmap.c:clk_enable_regmap() {
    ...
    return regmap_update_bits(rclk->regmap, rclk->enable_reg,
                              rclk->enable_mask, val);
}

Because _regmap_update_bits() sees the cached value already has
enable_mask set, it skips the hardware write and leaves the clock disabled
in hardware.

Should .cache_type be omitted from lpass_rate_gen_regmap_config (or
LPASS_RATE_GEN_CTRL marked volatile / the cache invalidated across runtime
suspend)?

> +
> +static int lpass_va_csr_probe(struct platform_device *pdev)
> +{

[ ... ]

> +	platform_set_drvdata(pdev, csr);
> +
> +	ret = devm_pm_runtime_enable(dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to enable PM runtime\n");
> +
> +	ret = devm_pm_clk_create(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = of_pm_clk_add_clks(dev);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "failed to get vote clocks\n");

[Severity: High]
What happens if runtime PM is enabled before the PM vote clocks are created
and added in lpass_va_csr_probe()?

In drivers/base/power/clock_ops.c, pm_clk_resume() returns 0 immediately
when dev_to_psd(dev) is NULL or the clock list is empty:

drivers/base/power/clock_ops.c:pm_clk_resume() {
    struct pm_subsys_data *psd = dev_to_psd(dev);
    ...
    if (!psd)
        return 0;

Because devm_pm_runtime_enable(dev) is called before devm_pm_clk_create(dev)
and of_pm_clk_add_clks(dev), a runtime resume triggered in that window (such
as via sysfs power/control) runs lpass_va_csr_runtime_resume(), receives 0
from pm_clk_resume(dev), and calls regmap_write() on
LPASS_RATE_GEN_COUNTER_0 and LPASS_RATE_GEN_DELAY without the vote clocks
enabled.

In addition, because devres callbacks execute in reverse registration order
on driver unbind or probe error, pm_clk_destroy() (registered by
devm_pm_clk_create()) runs before pm_runtime_disable_action() (registered by
devm_pm_runtime_enable()):

drivers/base/power/clock_ops.c:pm_clk_destroy() {
    ...
    dev_pm_put_subsys_data(dev);

    list_for_each_entry_safe_reverse(ce, c, &list, node) {
        list_del(&ce->node);
        __pm_clk_remove(ce);
    }
}

This disables and removes the vote clocks and clears dev->power.subsys_data
while runtime PM is still enabled on the device.

Should devm_pm_clk_create(dev) and of_pm_clk_add_clks(dev) be called before
devm_pm_runtime_enable(dev) in lpass_va_csr_probe()?

[ ... ]

> +}
> +

[ ... ]

>+MODULE_DESCRIPTION("Qualcomm LPASS VA CSR heartbeat pulse clock provider");
>+MODULE_LICENSE("GPL");

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021-master-v12-0-9b3df51fd1d7@oss.qualcomm.com?part=2