Re: [PATCH v6 03/11] ASoC: renesas: fsi: Fix trigger stop ordering

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Pre-existing data race in fsi_clk_enable() leads to premature clock disabling (bus hang) and breaks full-duplex operation.
- [High] Missing IRQ and DMA synchronization during stream shutdown leads to TOCTOU NULL pointer dereferences and hardware bus hangs.
--

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

ASoC: renesas: fsi: Fix trigger stop ordering

This patch changes the trigger stop ordering by calling fsi_stream_stop()
before fsi_hw_shutdown(). This ensures that the sequence matches the
existing order in the suspend path.

> This change ensures all register accesses during stream shutdown are fully
> completed before disabling the clocks.

[Severity: High]
This isn't a bug introduced by this patch, but is this statement accurate
without synchronization?

Without synchronize_irq() and dmaengine_terminate_sync() in the shutdown
sequence, an in-flight fsi_interrupt() or fsi_dma_complete() could run
concurrently.

If an in-flight interrupt handler accesses FSI registers after
fsi_hw_shutdown() disables the hardware clocks, could this trigger an
asynchronous bus hang?

> diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
> index 8cbd7acc26f49..800ac40f9680d 100644
> --- a/sound/soc/renesas/fsi.c
> +++ b/sound/soc/renesas/fsi.c
> @@ -1586,10 +1586,10 @@ static int fsi_dai_trigger(struct snd_pcm_substream *substream, int cmd,
>  			ret = fsi_stream_transfer(io);
>  		break;
>  	case SNDRV_PCM_TRIGGER_STOP:
> -		if (!ret)
> -			ret = fsi_hw_shutdown(fsi, dai->dev);
>  		fsi_stream_stop(fsi, io);
>  		fsi_stream_quit(fsi, io);

[Severity: High]
This is a pre-existing issue, but does fsi_stream_quit() introduce a race
with in-flight interrupts?

Because fsi_stream_quit() sets io->substream = NULL, could concurrent
execution cause NULL pointer dereferences in fsi_pio_get_area() or
fsi_pointer_update(), which dereference io->substream without validation?

> +		if (!ret)
> +			ret = fsi_hw_shutdown(fsi, dai->dev);

[Severity: High]
This is also a pre-existing issue, but does fsi_hw_shutdown() have a race
condition when managing the clocks?

fsi_hw_shutdown() could run concurrently with fsi_hw_startup() since ALSA
permits concurrent SNDRV_PCM_TRIGGER_START calls for playback and capture
on the same DAI.

Looking at fsi_clk_enable() and fsi_clk_disable() which are called from
these paths:

sound/soc/renesas/fsi.c:fsi_clk_enable() {
    ...
    int ret = -EINVAL;
    ...
    if (0 == clock->count) {
        // ...
        clock->count++;
    }
    return ret;
}

sound/soc/renesas/fsi.c:fsi_clk_disable() {
    ...
    if (1 == clock->count--) {
    ...
}

Could a lost update on the lockless clock->count variable lead to
prematurely disabling the clock when the first stream stops, turning off
FSI clocks while the second stream is still actively accessing registers?

Additionally, fsi_clk_enable() initializes ret to -EINVAL. If a second
stream tries to start (clock->count != 0), doesn't it unconditionally
return -EINVAL, breaking full-duplex operation?

>  		break;
>  	}

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