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

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 <[email protected]>
> ---
>   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