diff mbox series

[v3,3/3] ASoC: sunxi: sun4i-spdif: Reorder clock enable sequence

Message ID 20260602114437.50324-4-phucduc.bui@gmail.com (mailing list archive)
State New
Headers
Series ASoC: sunxi: sun4i-spdif: Cleanup and runtime PM improvements |

Commit Message

Bui Duc Phuc June 2, 2026, 11:44 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 APB bus clock drives the register interface and must be enabled before
the functional 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 v3:
  - Clarify in the commit message that the resume sequence becomes the
    reverse of the suspend sequence.

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

Comments

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

Pre-existing issues:
- [High] Unbinding the driver while an audio stream is active will cause a fatal bus fault because hardware is suspended before the ASoC component is unregistered.
--

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

ASoC: sunxi: sun4i-spdif: Reorder clock enable sequence

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

> diff --git a/sound/soc/sunxi/sun4i-spdif.c b/sound/soc/sunxi/sun4i-spdif.c
> index f54eb14c9ed86..102db1a2afbb8 100644
> --- a/sound/soc/sunxi/sun4i-spdif.c
> +++ b/sound/soc/sunxi/sun4i-spdif.c
> @@ -643,15 +643,15 @@ static int sun4i_spdif_runtime_suspend(struct device *dev)
>  
>  static int sun4i_spdif_runtime_resume(struct device *dev)
>  {
[ ... ]
> -	ret = clk_prepare_enable(host->spdif_clk);
> -	if (ret)
> -		return ret;
>  	ret = clk_prepare_enable(host->apb_clk);
>  	if (ret)
> -		clk_disable_unprepare(host->spdif_clk);
> +		return ret;
> +	ret = clk_prepare_enable(host->spdif_clk);
> +	if (ret)
> +		clk_disable_unprepare(host->apb_clk);
>  
>  	return ret;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but unbinding the driver while an
audio stream is active can cause a fatal bus fault.

Looking at sun4i_spdif_remove(), it manually disables PM and suspends the
hardware:

sun4i_spdif_remove() {
    pm_runtime_disable(&pdev->dev);
    if (!pm_runtime_status_suspended(&pdev->dev))
        sun4i_spdif_runtime_suspend(&pdev->dev);
}

Because devres unregisters the ASoC component after sun4i_spdif_remove()
returns, the ALSA core responds to the unregistration by tearing down the
active stream. This invokes callbacks like sun4i_spdif_trigger() with
SNDRV_PCM_TRIGGER_STOP:

sun4i_spdif_trigger() {
    case SNDRV_PCM_TRIGGER_STOP:
    ...
        sun4i_snd_txctrl_off(substream, host);
        break;
}

This tries to write to the device registers via regmap to disable
transmission. Since the APB bus clock was already disabled in
sun4i_spdif_remove(), accessing the registers causes a Data Abort and panics
the kernel.

Could the PM teardown be managed using devm_add_action_or_reset(), registered
immediately after pm_runtime_enable() and before component registration, to
ensure the ASoC component is fully unregistered before the clocks are turned
off?
diff mbox series

Patch

diff --git a/sound/soc/sunxi/sun4i-spdif.c b/sound/soc/sunxi/sun4i-spdif.c
index f54eb14c9ed8..102db1a2afbb 100644
--- a/sound/soc/sunxi/sun4i-spdif.c
+++ b/sound/soc/sunxi/sun4i-spdif.c
@@ -643,15 +643,15 @@  static int sun4i_spdif_runtime_suspend(struct device *dev)
 
 static int sun4i_spdif_runtime_resume(struct device *dev)
 {
-	struct sun4i_spdif_dev *host  = dev_get_drvdata(dev);
+	struct sun4i_spdif_dev *host = dev_get_drvdata(dev);
 	int ret;
 
-	ret = clk_prepare_enable(host->spdif_clk);
+	ret = clk_prepare_enable(host->apb_clk);
 	if (ret)
 		return ret;
-	ret = clk_prepare_enable(host->apb_clk);
+	ret = clk_prepare_enable(host->spdif_clk);
 	if (ret)
-		clk_disable_unprepare(host->spdif_clk);
+		clk_disable_unprepare(host->apb_clk);
 
 	return ret;
 }