Re: [PATCH 3/3] ASoC: rockchip: rockchip_pdm: Reorder clock enable sequence

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Unchecked return value of `pm_runtime_get_sync()` in `rockchip_pdm_set_fmt()` can lead to hardware access while suspended.
- [High] Manual disable of hardware clocks and PM runtime before devm teardown can cause bus faults.
--

commit 8f19ac4b34f49c4a08218121167b705580c4469a
Author: bui duc phuc <phucduc.bui@gmail.com>

ASoC: rockchip: rockchip_pdm: Reorder clock enable sequence

Enable the 'hclk' bus clock before the 'clk' controller clock during runtime
resume. The bus clock provides the register access interface and should be
enabled before the controller clock.

> diff --git a/sound/soc/rockchip/rockchip_pdm.c b/sound/soc/rockchip/rockchip_pdm.c
> index c69cdd6f24994..8f78f7bc1806c 100644
> --- a/sound/soc/rockchip/rockchip_pdm.c
> +++ b/sound/soc/rockchip/rockchip_pdm.c

[ ... ]

While reviewing this patch, I noticed a couple of pre-existing issues in the
same file:

sound/soc/rockchip/rockchip_pdm.c:rockchip_pdm_set_fmt() {
...
	pm_runtime_get_sync(cpu_dai->dev);
	regmap_update_bits(pdm->regmap, PDM_CLK_CTRL, mask, val);
	pm_runtime_put(cpu_dai->dev);
...
}

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to ignore the
return value of pm_runtime_get_sync() here?

If the runtime resume fails (for example, if clock enablement or power
domain restoration fails), the function ignores the negative error code
and proceeds to call regmap_update_bits().

Accessing hardware registers when the hardware is powered down or unclocked
can lead to a synchronous external abort (bus fault) and a kernel panic.

Should pm_runtime_resume_and_get() be used instead, and the error handled?


sound/soc/rockchip/rockchip_pdm.c:rockchip_pdm_remove() {
...
static void rockchip_pdm_remove(struct platform_device *pdev)
{
	struct rk_pdm_dev *pdm = dev_get_drvdata(&pdev->dev);

	pm_runtime_disable(&pdev->dev);
	if (!pm_runtime_status_suspended(&pdev->dev))
		rockchip_pdm_runtime_suspend(&pdev->dev);

	clk_disable_unprepare(pdm->clk);
	clk_disable_unprepare(pdm->hclk);
}
...
}

[Severity: High]
This is a pre-existing issue, but can manually disabling the clocks and PM
runtime before the devm teardown cause bus faults?

The driver registers the ASoC component using
devm_snd_soc_register_component() in rockchip_pdm_probe(), which defers
unregistration until after the remove() callback returns.

If the device is unbound via sysfs while an audio stream is active, ALSA
will attempt to tear down the stream during the deferred devres
unregistration. This triggers operations like
rockchip_pdm_trigger(SNDRV_PCM_TRIGGER_STOP), which writes to the
hardware registers.

Since the clocks and power domains were already forcibly disabled in
remove(), this could result in a bus fault and kernel panic.

Should the driver use devm_pm_runtime_enable() with a devres action for
clock disable, or unregister the component manually before turning off
resources?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260522100318.73474-1-phucduc.bui@gmail.com?part=3