[PATCH] ASoC: xilinx: formatter_pcm: clear stream pointers on close

[PATCH] ASoC: xilinx: formatter_pcm: clear stream pointers on close

[Rosen Penev]

xlnx_formatter_pcm_close() never clears adata->play_stream or
adata->capture_stream, and on reset failure it also skips
xlnx_formatter_disable_irqs(), leaving the hardware IOC interrupts
enabled while ALSA tears the stream down.  A subsequent interrupt then
dereferences the dangling substream pointer and calls
snd_pcm_period_elapsed() on a closed stream whose runtime has already
been freed, causing a WARN_ON or crash.  Since the IRQ is never
disabled, this repeats on every interrupt.

Disable the IRQs before resetting the formatter, and clear the stream
pointers in close so the IRQ handlers' NULL checks catch straggler
interrupts.  Do the cleanup even when the reset fails.

Fixes: 6f6c3c36f091 ("ASoC: xlnx: add pcm formatter platform driver")
Assisted-by: opencode:deepseek-v4-flash-free
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 sound/soc/xilinx/xlnx_formatter_pcm.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/sound/soc/xilinx/xlnx_formatter_pcm.c b/sound/soc/xilinx/xlnx_formatter_pcm.c
index 7eba3a0205f1..e1d2f1cadf8d 100644
--- a/sound/soc/xilinx/xlnx_formatter_pcm.c
+++ b/sound/soc/xilinx/xlnx_formatter_pcm.c
@@ -421,15 +421,19 @@ static int xlnx_formatter_pcm_close(struct snd_soc_component *component,
 	int ret;
 	struct xlnx_pcm_stream_param *stream_data =
 			substream->runtime->private_data;
+	struct xlnx_pcm_drv_data *adata = dev_get_drvdata(component->dev);
+
+	xlnx_formatter_disable_irqs(stream_data->mmio, substream->stream);
+
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+		adata->play_stream = NULL;
+	else
+		adata->capture_stream = NULL;
 
 	ret = xlnx_formatter_pcm_reset(stream_data->mmio);
-	if (ret) {
+	if (ret)
 		dev_err(component->dev, "audio formatter reset failed\n");
-		goto err_reset;
-	}
-	xlnx_formatter_disable_irqs(stream_data->mmio, substream->stream);
 
-err_reset:
 	kfree(stream_data);
 	return 0;
 }
-- 
2.55.0

Re: [PATCH] ASoC: xilinx: formatter_pcm: clear stream pointers on close

[Michal Simek]

On 8/7/26 02:20, Rosen Penev wrote:
> xlnx_formatter_pcm_close() never clears adata->play_stream or
> adata->capture_stream, and on reset failure it also skips
> xlnx_formatter_disable_irqs(), leaving the hardware IOC interrupts
> enabled while ALSA tears the stream down.  A subsequent interrupt then
> dereferences the dangling substream pointer and calls
> snd_pcm_period_elapsed() on a closed stream whose runtime has already
> been freed, causing a WARN_ON or crash.  Since the IRQ is never
> disabled, this repeats on every interrupt.
> 
> Disable the IRQs before resetting the formatter, and clear the stream
> pointers in close so the IRQ handlers' NULL checks catch straggler
> interrupts.  Do the cleanup even when the reset fails.
> 
> Fixes: 6f6c3c36f091 ("ASoC: xlnx: add pcm formatter platform driver")
> Assisted-by: opencode:deepseek-v4-flash-free
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
>   sound/soc/xilinx/xlnx_formatter_pcm.c | 14 +++++++++-----
>   1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/sound/soc/xilinx/xlnx_formatter_pcm.c b/sound/soc/xilinx/xlnx_formatter_pcm.c
> index 7eba3a0205f1..e1d2f1cadf8d 100644
> --- a/sound/soc/xilinx/xlnx_formatter_pcm.c
> +++ b/sound/soc/xilinx/xlnx_formatter_pcm.c
> @@ -421,15 +421,19 @@ static int xlnx_formatter_pcm_close(struct snd_soc_component *component,
>   	int ret;
>   	struct xlnx_pcm_stream_param *stream_data =
>   			substream->runtime->private_data;
> +	struct xlnx_pcm_drv_data *adata = dev_get_drvdata(component->dev);
> +
> +	xlnx_formatter_disable_irqs(stream_data->mmio, substream->stream);
> +
> +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> +		adata->play_stream = NULL;
> +	else
> +		adata->capture_stream = NULL;
>   
>   	ret = xlnx_formatter_pcm_reset(stream_data->mmio);
> -	if (ret) {
> +	if (ret)
>   		dev_err(component->dev, "audio formatter reset failed\n");
> -		goto err_reset;
> -	}
> -	xlnx_formatter_disable_irqs(stream_data->mmio, substream->stream);
>   
> -err_reset:
>   	kfree(stream_data);
>   	return 0;
>   }

I am not convinced that this is correct. Based on
https://docs.amd.com/r/en-US/pg330-audio-formatter/S2MM-Control-Register-0x10

when you trigger reset (bit 1)
"After completion of a soft reset, all registers and bits are in the Reset State."

Default value for BIT 13 (AUD_CTRL_IOC_IRQ_MASK) is 1.
It means after your change interrupt on completion is enabled not disabled.

Katta: Can you please confirm it?

Rosen: Did you test this on HW?

Thanks,
Michal