diff mbox series

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

Message ID 20260602113136.49494-1-phucduc.bui@gmail.com (mailing list archive)
State New
Headers
Series [v2] ASoC: sunxi: sun50i-dmic: Reorder clock enable sequence |

Commit Message

Bui Duc Phuc June 2, 2026, 11:31 a.m. UTC 
From: bui duc phuc <phucduc.bui@gmail.com>

Reorder the runtime resume clock enable sequence to match the hardware
dependency and ensure symmetry with the suspend path.
The bus clock drives the register interface and must be enabled before
the DMIC module clock. This aligns the resume sequence to be the
exact reverse of the suspend sequence.

Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---

NOTE: This patch is compile-tested only.
Changes in v2:
  - Clarify in the commit message that the resume sequence becomes the
    reverse of the suspend sequence.

 sound/soc/sunxi/sun50i-dmic.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

sashiko-bot@kernel.org June 2, 2026, 11:42 a.m. UTC | #1 
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] ALSA kcontrols can be accessed while the device is runtime suspended, leading to an external abort or system hang.
- [High] The driver manually disables runtime PM and suspends the hardware in remove() before devm unwinding unregisters the ASoC component, leading to potential MMIO accesses to unclocked registers and a kernel panic.
- [High] Missing reset_control_assert() on probe error paths and in driver removal.
--

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

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

Reorder the runtime resume clock enable sequence to match the hardware
dependency and ensure symmetry with the suspend path.
The bus clock drives the register interface and must be enabled before
the DMIC module clock. This aligns the resume sequence to be the
exact reverse of the suspend sequence.

> 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 is a pre-existing issue, but can the ALSA volume controls be accessed
while the device is runtime suspended?

The regmap configuration does not enable runtime PM:

static const struct regmap_config sun50i_dmic_regmap_config = {
	.reg_bits = 32,
	.reg_stride = 4,
	.val_bits = 32,
	.max_register = SUN50I_DMIC_VERSION,
	.cache_type = REGCACHE_NONE,
};

If a user application manipulates the exposed kcontrols in
sun50i_dmic_controls() while the audio stream is inactive, the device will be
in the RPM_SUSPENDED state with the bus clock disabled.

Since regmap will attempt an MMIO access on an unclocked peripheral without
waking it up, does this cause a synchronous external abort?

[Severity: High]
This is a pre-existing issue, but does sun50i_dmic_remove() create a race
condition with the devres teardown?

In sun50i_dmic_probe(), the ASoC component is registered using the devm API:

	ret = devm_snd_soc_register_component(&pdev->dev, &sun50i_dmic_component,
					      &sun50i_dmic_dai, 1);

However, sun50i_dmic_remove() manually suspends the hardware and turns off
the clocks before the component is unregistered:

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 the driver is unbound while an audio stream is actively running, the
clocks will be disabled first. Later, during devres unwinding, the ALSA
core tears down the stream and invokes sun50i_dmic_trigger() with
SNDRV_PCM_TRIGGER_STOP.

Will the regmap_update_bits() calls in the trigger callback attempt to
access unclocked hardware and cause a kernel panic?

[Severity: High]
This is a pre-existing issue, but is there a missing reset assertion during
driver removal or probe failure?

In sun50i_dmic_probe(), the reset line is deasserted:

	host->rst = devm_reset_control_get_optional_exclusive(&pdev->dev, NULL);
	if (IS_ERR(host->rst))
		return dev_err_probe(&pdev->dev, PTR_ERR(host->rst),
				     "Failed to get reset.\n");
	reset_control_deassert(host->rst);

However, I don't see a corresponding reset_control_assert() in the
error paths of probe, nor in sun50i_dmic_remove().

Does this leave the IP block actively running out of reset across driver
unbinds or re-probes, potentially causing logic errors or audio capture
failures on subsequent driver reloads?
diff mbox series

Patch

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;
 	}