Re: [PATCH v8 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: lpass: missing pm_clk callbacks causes unclocked hardware access
- [Medium] clk: qcom: lpass: async pm_runtime_put() races with devres pm_runtime_disable()
--

commit 5684759a56eb84ac05a7585939de6fdaa87b92de
Author: Sarath Ganapathiraju <sarath.ganapathiraju@oss.qualcomm.com>

clk: qcom: Add LPASS VA CSR heartbeat pulse clock

The HeartBeat Pulse, also known as RateGen Pulse, synchronizes the
start of DMAs and Codec Interfaces for the audio usecases and can
serve as a periodic wakeup source for the DSP. Add the LPASS VA CSR
driver that models the rate generator as a clock provider so it is
enabled and disabled automatically alongside the other clocks during
runtime PM resume and suspend. Attach the DT vote clocks to runtime PM
and resume the device before programming the rate generator registers,
so register accesses happen only after the LPASS votes are active.

> diff --git a/drivers/clk/qcom/lpass-va-csr.c b/drivers/clk/qcom/lpass-va-csr.c
> new file mode 100644
> index 0000000000000..3c87c3319f156
> --- /dev/null
> +++ b/drivers/clk/qcom/lpass-va-csr.c

[ ... ]

> +	ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, &csr->hb.hw);
> +	if (ret)
> +		goto err_pm_runtime_put;
> +
> +	pm_runtime_put(dev);
> +
> +	return 0;
> +
> +err_pm_runtime_put:
> +	pm_runtime_put(dev);
> +
> +	return ret;
> +}

[Severity: Medium]
Will this asynchronous pm_runtime_put() race with the devres cleanup?

When probe fails, err_pm_runtime_put calls pm_runtime_put(), which queues an
asynchronous suspend. The probe function also uses devm_pm_runtime_enable(),
which queues a devres cleanup action that calls pm_runtime_disable() on
failure.

After the function returns, devres cleanup will invoke pm_runtime_disable(),
cancelling the pending asynchronous suspend request. This leaves the device
permanently active and its vote clocks enabled.

Could pm_runtime_put_sync() be used here instead to ensure the suspend
completes synchronously before devres cleanup?

[ ... ]

> +static struct platform_driver lpass_va_csr_driver = {
> +	.driver = {
> +		.name = "qcom-lpass-va-csr",
> +		.of_match_table = lpass_va_csr_dt_match,
> +	},
> +	.probe = lpass_va_csr_probe,
> +};

[Severity: High]
Does this driver need a dev_pm_ops structure assigned to handle the PM clock
callbacks?

The driver registers DT vote clocks using of_pm_clk_add_clks(), relying on the
PM framework to enable them during runtime resume. Without assigning a
dev_pm_ops structure mapped to pm_clk_suspend and pm_clk_resume in the
platform_driver, pm_runtime_resume_and_get() sets the device state to active
but leaves the clocks physically disabled.

Will the subsequent regmap_write() attempts in probe try to access unclocked
hardware registers and trigger a synchronous external abort on Qualcomm
platforms?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260917-master-v8-0-823d2c5d090a@oss.qualcomm.com?part=2