Re: [PATCH v10 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: missing hardware state restoration on runtime resume
- [Medium] clk: qcom: missing NULL check for of_device_get_match_data()
--

commit 506d9b3aab397203423c9181e755579f9ddb3dd7
Author: Sarath Ganapathiraju <sarath.ganapathiraju@oss.qualcomm.com>

clk: qcom: Add LPASS VA CSR heartbeat pulse clock

This commit adds the LPASS VA CSR driver to model the rate generator as a
clock provider. It attaches DT vote clocks to runtime PM and programs the
rate generator registers during probe.

> diff --git a/drivers/clk/qcom/lpass-va-csr.c b/drivers/clk/qcom/lpass-va-csr.c
> new file mode 100644
> index 0000000000000..b4c0f88187ebb
> --- /dev/null
> +++ b/drivers/clk/qcom/lpass-va-csr.c
[ ... ]
> +static int lpass_va_csr_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	const struct lpass_va_csr_data *data = of_device_get_match_data(dev);
[ ... ]
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(regmap, LPASS_RATE_GEN_COUNTER_0, data->counter_0);

[Severity: Medium]
Is there a risk of a NULL pointer dereference here?

If the device is bound manually via sysfs rather than instantiated through
Device Tree, of_device_get_match_data() will return NULL. The data pointer
is then unconditionally dereferenced when writing to LPASS_RATE_GEN_COUNTER_0
and LPASS_RATE_GEN_DELAY.

[Severity: High]
Does this hardware configuration get lost during runtime suspend and resume
cycles?

When the clock is unprepared by the consumer, the PM runtime reference is
dropped. The pm_clk_suspend callback disables the vote clocks, which can
allow the LPASS power domain to collapse and erase the register context.
The data written here to LPASS_RATE_GEN_COUNTER_0 and LPASS_RATE_GEN_DELAY
is only configured once during probe.

> +	if (ret) {
> +		pm_runtime_put_sync(dev);
> +		return ret;
> +	}
> +
> +	ret = regmap_write(regmap, LPASS_RATE_GEN_DELAY, data->delay);
[ ... ]
> +static const struct dev_pm_ops lpass_va_csr_pm_ops = {
> +	SET_RUNTIME_PM_OPS(pm_clk_suspend, pm_clk_resume, NULL)
> +};

[Severity: High]
Should there be a regcache_sync() call to restore the hardware state?

Because the generic pm_clk_resume is used directly without an additional
wrapper that syncs the regmap cache, the counter and delay registers are
not reprogrammed after waking up from runtime suspend. The subsequent
clk_enable_regmap() will only write the control register, leaving the
hardware with uninitialized or reset parameters which could break audio
DMAs and codec synchronization.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930-master-v10-0-105207bc5275@oss.qualcomm.com?part=2