Re: [PATCH v3 04/10] 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 5 potential issue(s) to consider:
- [Critical] Incomplete Fix / Unprotected Hardware Access in fsi_stream_transfer()
- [Critical] Uncleared Interrupt Status Causes Guaranteed Hang on Next IRQ
- [Critical] Pre-existing TOCTOU Race Leading to NULL Pointer Dereference in IRQ Handler
- [High] Data Race / TOCTOU on fsi->running_streams
- [High] State Tracking Variables Leak on START Failure
--

commit a5f51696dc0fd84e1f16198b25e33b2b5da818dc
Author: bui duc phuc <phucduc.bui@gmail.com>
Subject: ASoC: renesas: fsi: Fix register access from in-flight IRQ after shutdown

This commit attempts to fix a system hang caused by in-flight interrupt
handlers accessing hardware registers after the clocks are shut down. It
tracks running streams and adds early returns in the interrupt path when no
streams are active.

> diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
> index 94ab2e4908108..fc8428cb1ba73 100644
> --- a/sound/soc/renesas/fsi.c
> +++ b/sound/soc/renesas/fsi.c
> @@ -460,6 +463,9 @@ static int fsi_get_current_fifo_samples(struct fsi_priv *fsi,
>  
>  static void fsi_count_fifo_err(struct fsi_priv *fsi)
>  {
> +	if (fsi->running_streams == 0)
> +		return;
> +

Does this check completely protect against hardware access during shutdown?

The interrupt handler calls fsi_stream_transfer() before reaching
fsi_count_fifo_err() and fsi_irq_clear_status(). The fsi_stream_transfer()
function calls handlers that directly read hardware registers, such as
DOFF_ST inside fsi_get_current_fifo_samples(). Because this hardware access
remains completely unprotected by the new running_streams check, would an
in-flight interrupt handler still access the shut-down hardware and cause
the exact system hang this patch is intended to fix?

Additionally, is there a time-of-check to time-of-use data race when
evaluating fsi->running_streams here?

The value is read locklessly in the interrupt handler, while it is updated
under fsi->master->lock in the process context during fsi_dai_trigger().
An in-flight interrupt on one processor could evaluate running_streams > 0
locklessly and proceed. Concurrently, another processor could execute
fsi_dai_trigger(SNDRV_PCM_TRIGGER_STOP), acquire the lock, decrement the
counter to 0, and disable the hardware clocks. When the interrupt handler
resumes, it would execute hardware accesses on the disabled clock.

As a side note, there appears to be a pre-existing time-of-check to
time-of-use issue leading to a potential NULL pointer dereference in the
interrupt handler. In fsi_pio_transfer(), the handler calls
fsi_stream_is_working(), which temporarily takes master->lock to verify
io->substream != NULL. It then drops the lock and unconditionally
dereferences io->substream->runtime. Could a concurrent trigger stop on
another processor execute fsi_stream_quit() and set io->substream = NULL
between the check and the use, causing a kernel panic when the interrupt
handler resumes?

[ ... ]
> @@ -681,6 +687,9 @@ static void fsi_irq_clear_status(struct fsi_priv *fsi)
>  	u32 data = 0;
>  	struct fsi_master *master = fsi_get_master(fsi);
>  
> +	if (fsi->running_streams == 0)
> +		return;
> +

Can returning early here without clearing the interrupt status cause a
guaranteed hang on the next interrupt?

If an in-flight or delayed interrupt fires as the stream is shut down, the
handler skips clearing its port's interrupt bits in the shared master int_st
register. Because this register is shared between port A and port B, the
uncleared bit remains permanently asserted. When the other active port
subsequently generates an interrupt, fsi_interrupt() will read int_st, see
the stale bit for the shut-down port, and unconditionally call
fsi_stream_transfer() for it. This forces a hardware register access on the
shut-down port while its clocks are disabled.

[ ... ]
> @@ -1573,10 +1582,17 @@ static int fsi_dai_trigger(struct snd_pcm_substream *substream, int cmd,
>  {
>  	struct fsi_priv *fsi = fsi_get_priv(substream);
>  	struct fsi_stream *io = fsi_stream_get(fsi, substream);
> +	bool need_shutdown = false;
>  	int ret = 0;
>  
>  	switch (cmd) {
>  	case SNDRV_PCM_TRIGGER_START:
> +		scoped_guard(spinlock_irqsave, &fsi->master->lock) {
> +			if (!io->running) {
> +				io->running = true;
> +				fsi->running_streams++;
> +			}
> +		}
>  		fsi_stream_init(fsi, io, substream);
>  		if (!ret)
>  			ret = fsi_hw_startup(fsi, io, dai->dev);

Does this leak the state tracking variables if the start sequence fails?

If fsi_hw_startup() or fsi_stream_start() fails and returns an error,
the framework aborts the start sequence and does not invoke
SNDRV_PCM_TRIGGER_STOP. Because the error handling path does not revert
io->running and decrement fsi->running_streams, these variables could
permanently leak. This would prevent running_streams from reaching 0 on
subsequent valid stops, permanently bypassing fsi_hw_shutdown() and leaving
the hardware clocks powered on indefinitely.

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