[PATCH] ASoC: sunxi: sun50i-dmic: Reorder clock enable sequence

[PATCH] ASoC: sunxi: sun50i-dmic: Reorder clock enable sequence

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

Enable the bus clock before the DMIC module clock during
runtime resume.
The bus clock provides the register access interface and
should be enabled before the module clock.

Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 sound/soc/sunxi/sun50i-dmic.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/sound/soc/sunxi/sun50i-dmic.c b/sound/soc/sunxi/sun50i-dmic.c
index eddfebe16616..7aff1afdf265 100644
--- a/sound/soc/sunxi/sun50i-dmic.c
+++ b/sound/soc/sunxi/sun50i-dmic.c
@@ -323,16 +323,16 @@ static int sun50i_dmic_runtime_suspend(struct device *dev)
 
 static int sun50i_dmic_runtime_resume(struct device *dev)
 {
-	struct sun50i_dmic_dev *host  = dev_get_drvdata(dev);
+	struct sun50i_dmic_dev *host = dev_get_drvdata(dev);
 	int ret;
 
-	ret = clk_prepare_enable(host->dmic_clk);
+	ret = clk_prepare_enable(host->bus_clk);
 	if (ret)
 		return ret;
 
-	ret = clk_prepare_enable(host->bus_clk);
+	ret = clk_prepare_enable(host->dmic_clk);
 	if (ret) {
-		clk_disable_unprepare(host->dmic_clk);
+		clk_disable_unprepare(host->bus_clk);
 		return ret;
 	}
 
-- 
2.43.0

Re: [PATCH] ASoC: sunxi: sun50i-dmic: Reorder clock enable sequence

[sashiko-bot]

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

Pre-existing issues:
- [High] Accessing ALSA kcontrols while the device is runtime-suspended accesses a clock-gated register, causing a kernel crash.
- [High] The device's clocks are forcibly disabled in `remove()` while the ALSA component and DMA engine are still registered via devm, leading to a fatal bus stall.
--

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

ASoC: sunxi: sun50i-dmic: Reorder clock enable sequence

Enable the bus clock before the DMIC module clock during runtime resume.
The bus clock provides the register access interface and should be enabled
before the module clock.

> diff --git a/sound/soc/sunxi/sun50i-dmic.c b/sound/soc/sunxi/sun50i-dmic.c
> index eddfebe166169..7aff1afdf265f 100644
> --- a/sound/soc/sunxi/sun50i-dmic.c
> +++ b/sound/soc/sunxi/sun50i-dmic.c
> @@ -323,16 +323,16 @@ static int sun50i_dmic_runtime_suspend(struct device *dev)
>  
>  static int sun50i_dmic_runtime_resume(struct device *dev)
>  {
> -	struct sun50i_dmic_dev *host  = dev_get_drvdata(dev);
> +	struct sun50i_dmic_dev *host = dev_get_drvdata(dev);
>  	int ret;
>  
> -	ret = clk_prepare_enable(host->dmic_clk);
> +	ret = clk_prepare_enable(host->bus_clk);
>  	if (ret)
>  		return ret;
>  
> -	ret = clk_prepare_enable(host->bus_clk);
> +	ret = clk_prepare_enable(host->dmic_clk);
>  	if (ret) {
> -		clk_disable_unprepare(host->dmic_clk);
> +		clk_disable_unprepare(host->bus_clk);
>  		return ret;
>  	}
>  

[Severity: High]
This isn't a bug introduced by this patch, but since we are modifying the
runtime resume clock sequence, does this driver allow userspace to access
clock-gated registers when the audio stream is idle?

The driver defines ALSA volume controls (like sun50i_dmic_controls) that
map directly to hardware registers. The regmap configuration uses
REGCACHE_NONE and does not set .pm_runtime = true.

Since regmap accesses go directly to the hardware without using
pm_runtime_get_sync() in the control .get/.put handlers, reading or writing
a volume control from userspace when the device is runtime suspended would
access the DMIC registers while bus_clk is disabled. Will this trigger an
asynchronous external abort or bus error and crash the kernel?

[Severity: High]
This is a pre-existing issue, but the device's clocks appear to be forcibly
disabled in remove() while the ALSA component and DMA engine are still
registered.

The ALSA component and DMA engine are registered using
devm_snd_soc_register_component() and devm_snd_dmaengine_pcm_register(), so
they are unregistered after the sun50i_dmic_remove() callback returns.

However, sun50i_dmic_remove() manually disables the clocks:

static void sun50i_dmic_remove(struct platform_device *pdev)
{
	pm_runtime_disable(&pdev->dev);
	if (!pm_runtime_status_suspended(&pdev->dev))
		sun50i_dmic_runtime_suspend(&pdev->dev);
}

If a PCM stream is active during unbind, will the running DMA controller or
subsequent stop triggers during component unregistration attempt to access
the now clock-gated DMIC registers and cause a system crash?

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