Re: [PATCH v6 05/11] ASoC: renesas: fsi: Fix register access from in-flight IRQ after shutdown

[sashiko-bot@kernel.org]

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

New issues:
- [High] The newly added `fsi_stream_is_working()` checks introduce a Time-of-Check to Time-of-Use (TOCTOU) race condition with the clock disablement path.

Pre-existing issues:
- [High] The patch fails to prevent system hangs during in-flight IRQs because it misses the `fsi_stream_transfer()` path, which still performs unprotected hardware accesses and NULL pointer dereferences.
--

commit 6fe54dc43154d802e5ce8471d592fd6f73f622bd
Author: bui duc phuc <phucduc.bui@gmail.com>

ASoC: renesas: fsi: Fix register access from in-flight IRQ after shutdown

This commit attempts to fix system hangs caused by in-flight IRQs accessing
registers when the SPU clock is disabled. It introduces fsi_stream_is_working()
checks in fsi_count_fifo_err() and fsi_irq_clear_status() to abort early if no
streams are active.

> diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
> index 0bd0e0c8c5a30..3e3c6fd7c56b8 100644
> --- a/sound/soc/renesas/fsi.c
> +++ b/sound/soc/renesas/fsi.c
> @@ -470,6 +470,10 @@ static int fsi_get_current_fifo_samples(struct fsi_priv *fsi,
>  
>  static void fsi_count_fifo_err(struct fsi_priv *fsi)
>  {
> +	if (!fsi_stream_is_working(fsi, &fsi->playback) &&
> +	    !fsi_stream_is_working(fsi, &fsi->capture))
> +		return;
> +

[Severity: High]
Does this introduce a time-of-check to time-of-use race condition?

The fsi_stream_is_working() function acquires master->lock, checks the
substream, and immediately drops the lock. Because the lock is released before
the hardware access below:

	u32 ostatus = fsi_reg_read(fsi, DOFF_ST);
	u32 istatus = fsi_reg_read(fsi, DIFF_ST);

Could a concurrent ALSA shutdown sequence on another CPU execute
fsi_stream_quit() and fsi_hw_shutdown() to disable the SPU clock exactly in
this window, leading to the same system hang?

[ ... ]

> @@ -681,6 +685,10 @@ static void fsi_irq_clear_status(struct fsi_priv *fsi)
>  	u32 data = 0;
>  	struct fsi_master *master = fsi_get_master(fsi);
>  
> +	if (!fsi_stream_is_working(fsi, &fsi->playback) &&
> +	    !fsi_stream_is_working(fsi, &fsi->capture))
> +		return;
> +
>  	data |= AB_IO(1, fsi_get_port_shift(fsi, &fsi->playback));
>  	data |= AB_IO(1, fsi_get_port_shift(fsi, &fsi->capture));
>  

[Severity: High]
This isn't a bug introduced by this patch, but does the in-flight IRQ still
trigger hardware accesses and NULL pointer dereferences through other paths?

If fsi_interrupt() runs during shutdown, it calls fsi_stream_transfer()
unconditionally without checking fsi_stream_is_working() first:

sound/soc/renesas/fsi.c:fsi_interrupt() {
	if (int_st & AB_IO(1, AO_SHIFT))
		fsi_stream_transfer(&master->fsia.playback);
	...
}

In PIO mode, this leads to fsi_pio_push() which reads the hardware registers
before verifying the stream state:

	sample_space = io->fifo_sample_capa -
		       fsi_get_current_fifo_samples(fsi, io);

In DMA mode, it leads to fsi_dma_transfer() which dereferences io->substream
directly:

	struct snd_soc_dai *dai = fsi_get_dai(io->substream);
	struct snd_pcm_substream *substream = io->substream;

Can this still cause a system hang or a NULL pointer dereference if the
stream clock is already disabled and io->substream is set to NULL?

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