Re: [PATCH v2 3/3] ASoC: sunxi: sun4i-spdif: 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] This problem wasn't introduced by this patch, but the devres cleanup order for ALSA components triggers a kernel crash during module unload or driver unbind.
- [High] This problem wasn't introduced by this patch, but the driver incorrectly handles the 3-byte audio format `SNDRV_PCM_FORMAT_S20_3LE`, causing severe audio corruption.
--

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

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

This commit reorders the clock enable sequence during runtime resume.
It enables the APB bus clock before the SPDIF module clock to ensure
register accesses do not fail.

> 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)
>  {
> -	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);

[Severity: High]
This isn't a bug introduced by this patch, but the devres cleanup order for
ALSA components appears to trigger a kernel crash during module unload or
driver unbind.

In sun4i_spdif_probe(), the ALSA component and DMA engine are registered via
devm_snd_soc_register_component() and devm_snd_dmaengine_pcm_register().
The devres teardown runs after the driver's sun4i_spdif_remove() function
returns.

However, sun4i_spdif_remove() explicitly calls pm_runtime_disable() and
sun4i_spdif_runtime_suspend(), which disable the apb_clk bus clock.

When the devres teardown later unregisters the ASoC component, it forces any
active PCM streams to stop, invoking sun4i_spdif_trigger() with
SNDRV_PCM_TRIGGER_STOP. This calls sun4i_snd_txctrl_off(), which writes to
hardware registers via regmap.

Because the APB clock has already been disabled by the remove function, do
these regmap accesses hit unclocked hardware and result in a synchronous
external abort (kernel panic) on sunxi platforms?

[Severity: High]
This is a pre-existing issue, but does the driver incorrectly handle the
3-byte audio format SNDRV_PCM_FORMAT_S20_3LE, causing severe audio
corruption?

The driver advertises support for SNDRV_PCM_FMTBIT_S20_3LE in SUN4I_FORMATS,
which means 20-bit audio samples are tightly packed into exactly 3 bytes in
memory.

However, in sun4i_spdif_hw_params(), the DMA slave bus width
(dma_params_tx.addr_width) is left at DMA_SLAVE_BUSWIDTH_4_BYTES for this
format. This configures the dst_addr_width of the DMA controller to 4 bytes.

In the ALSA generic DMA engine code and sunxi-dma driver, the undefined
src_addr_width defaults to match the 4-byte destination width.

Because the memory buffer contains tightly packed 3-byte samples, does
commanding the DMA controller to perform 4-byte memory reads misalign the
stream immediately (reading part of the next sample on every fetch), leading
to complete data corruption and unusable audio output?

>  
>  	return ret;
>  }

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