Re: [PATCH 06/24] ASoC: amd: vangogh: Fix acp5x_dma_close() being skipped on rollback

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] ASoC: amd: vangogh: Uninitialized i2s_instance in acp5x_dma_close() corrupts concurrent streams
--

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

ASoC: amd: vangogh: Fix acp5x_dma_close() being skipped on rollback

This commit explicitly returns 0 on success in acp5x_dma_open() to ensure
ASoC correctly marks the component as opened. This prevents .close() from
being skipped during a rollback if .open() initially returned 1 from
snd_pcm_hw_constraint_integer().

> diff --git a/sound/soc/amd/vangogh/acp5x-pcm-dma.c b/sound/soc/amd/vangogh/acp5x-pcm-dma.c
> index 831e30e9b0426..33d59d992ee0c 100644
> --- a/sound/soc/amd/vangogh/acp5x-pcm-dma.c
> +++ b/sound/soc/amd/vangogh/acp5x-pcm-dma.c
> @@ -231,7 +231,7 @@ static int acp5x_dma_open(struct snd_soc_component *component,
>  	}
>  	i2s_data->acp5x_base = adata->acp5x_base;
>  	runtime->private_data = i2s_data;
> -	return ret;
> +	return 0;

[Severity: Critical]
By ensuring acp5x_dma_open() returns 0, ALSA SoC will now call
acp5x_dma_close() on rollback if a stream is aborted before hw_params()
is called.

Can this regression corrupt the state of concurrent streams?

When acp5x_dma_open() allocates i2s_data via kzalloc(), the i2s_instance
field is initialized to 0. It is only set to its correct value later in
acp5x_dma_hw_params().

If acp5x_dma_close() runs on rollback before hw_params() is called:

sound/soc/amd/vangogh/acp5x-pcm-dma.c:acp5x_dma_close() {
    ...
	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
		switch (ins->i2s_instance) {
		case I2S_HS_INSTANCE:
			adata->play_stream = NULL;
			break;
		case I2S_SP_INSTANCE:
		default:
			adata->i2ssp_play_stream = NULL;
		}
    ...
}

Because ins->i2s_instance is 0, it falls through to the default case
and sets adata->i2ssp_play_stream = NULL.

Since adata is shared per-device data, this incorrectly clears the stream
tracking pointer for a completely separate, concurrently running stream.

When the hardware fires a threshold interrupt for the active stream,
the handler will fail to clear it:

sound/soc/amd/vangogh/acp5x-pcm-dma.c:i2s_irq_handler() {
    ...
	if ((val & BIT(I2S_TX_THRESHOLD)) && vg_i2s_data->i2ssp_play_stream) {
		acp_writel(BIT(I2S_TX_THRESHOLD),
			   vg_i2s_data->acp5x_base + ACP_EXTERNAL_INTR_STAT);
		snd_pcm_period_elapsed(vg_i2s_data->i2ssp_play_stream);
		irq_flag = 1;
	}
    ...
}

Because the pointer was nullified, the interrupt remains asserted and the
handler returns IRQ_NONE, causing an infinite hardirq loop.

>  }
>  
>  static int acp5x_dma_hw_params(struct snd_soc_component *component,

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