Re: [PATCH] ASoC: meson: aiu: fifo-spdif: soft reset the S/PDIF datapath on start/stop

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

New issues:
- [High] The S/PDIF fast domain soft reset is issued after the DMA FIFO is enabled on the START path, causing potential data loss and DMA desynchronization.
- [High] Calling `snd_soc_component_write()` in the ALSA PCM trigger callback causes a sleep-in-atomic lockdep BUG.

Pre-existing issues:
- [High] Preexisting sleeping in atomic context bug in the I2S FIFO trigger callback due to `snd_soc_component_write()` and `snd_soc_component_read()`.
--

commit 60846173cae07d2808fb45b843fa96a97f4f8eb6
Author: Christian Hewitt <[email protected]>

ASoC: meson: aiu: fifo-spdif: soft reset the S/PDIF datapath on start/stop
    
The I2S FIFO soft-resets its fast domain on start (AIU_RST_SOFT bit 0 +
AIU_I2S_SYNC read in aiu_fifo_i2s_trigger), mirroring the downstream
vendor driver's audio_out_i2s_enable(). The S/PDIF FIFO has no equivalent:
it only toggles the IEC958 DCU, so a stale datapath FIFO can be replayed,
producing the "machine gun noise" buffer underrun - on start when switching
outputs, and on stop when playback ends.

> diff --git a/sound/soc/meson/aiu-fifo-spdif.c b/sound/soc/meson/aiu-fifo-spdif.c
> index e0e00ec026dcc..826055a71421d 100644
> --- a/sound/soc/meson/aiu-fifo-spdif.c
> +++ b/sound/soc/meson/aiu-fifo-spdif.c

[ ... ]

> @@ -68,12 +69,16 @@ static int fifo_spdif_trigger(struct snd_pcm_substream *substream, int cmd,
>  	case SNDRV_PCM_TRIGGER_START:
>  	case SNDRV_PCM_TRIGGER_RESUME:
>  	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> +		snd_soc_component_write(component, AIU_RST_SOFT,
> +					AIU_RST_SOFT_958_FAST);
[Severity: High]
Does this introduce a race condition on start? In fifo_spdif_trigger(),
aiu_fifo_trigger() is called before the switch statement, which starts the
upstream DMA engine before we issue the downstream S/PDIF fast domain soft
reset.

Could this discard audio frames fetched by the DMA into the FIFO before the
reset, causing left/right channel swapping and audio artifacts?

Additionally, the ALSA PCM trigger callback executes in atomic context with
the stream spinlock held. Is it safe to call snd_soc_component_write() here?

sound/soc/soc-component.c:snd_soc_component_write() {
	int ret;

	mutex_lock(&component->io_mutex);
	ret = soc_component_write_no_lock(component, reg, val);
	mutex_unlock(&component->io_mutex);

	return ret;
}

Acquiring a sleeping mutex inside an atomic section can cause scheduling while
atomic bugs and potential system hangs.

This is a pre-existing issue, but the same sleeping in atomic context pattern
is present in the I2S FIFO trigger callback:

sound/soc/meson/aiu-fifo-i2s.c:aiu_fifo_i2s_trigger() {
	...
	snd_soc_component_write(component, AIU_RST_SOFT,
				AIU_RST_SOFT_I2S_FAST);
	snd_soc_component_read(component, AIU_I2S_SYNC);
	...
}

Both snd_soc_component_write() and snd_soc_component_read() take the sleeping
io_mutex inside the atomic I2S trigger callback.

>  		fifo_spdif_dcu_enable(component, true);
>  		break;
>  	case SNDRV_PCM_TRIGGER_SUSPEND:
>  	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
>  	case SNDRV_PCM_TRIGGER_STOP:
>  		fifo_spdif_dcu_enable(component, false);
> +		snd_soc_component_write(component, AIU_RST_SOFT,
> +					AIU_RST_SOFT_958_FAST);
[Severity: High]
Can this call to snd_soc_component_write() also cause a scheduling while
atomic regression? It will acquire the sleeping io_mutex while in the atomic
STOP/SUSPEND trigger context.

>  		break;
>  	default:
>  		return -EINVAL;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1

_______________________________________________
linux-amlogic mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-amlogic