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

[sashiko-bot@kernel.org]

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?

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