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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Clock reference counting logic in `fsi_clk_enable()` permanently blocks subsequent active streams.
- [High] Asynchronous tasks (IRQ and DMA callbacks) dereference `io->substream` after it is freed by `fsi_stream_quit()`, causing a TOCTOU NULL pointer dereference.
- [High] Hardware DMA channels are released before ALSA component unregistration, leading to a Use-After-Free during teardown.
- [High] The ALSA trigger callback calls `clk_set_rate()`, which acquires sleepable mutexes, from an atomic context.
- [High] Missing synchronization with the IRQ handler before disabling the SPU clock in `TRIGGER_STOP`, causing hardware hangs.
- [High] Missing error rollback in `TRIGGER_START` leads to a permanent resource leak and device failure.
--

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

ASoC: renesas: fsi: Fix trigger stop ordering

Call fsi_stream_stop() before fsi_hw_shutdown(). This matches the existing
order in the suspend path.
This change ensures all register accesses during stream shutdown are fully
completed before disabling the clocks.

> 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);

[Severity: High]
This is a pre-existing issue, but if fsi_stream_transfer() fails during
SNDRV_PCM_TRIGGER_START, the error is returned immediately without rolling
back earlier steps like fsi_hw_startup() and fsi_clk_enable().

Could this leak the clock->count reference and leave hardware clocks
permanently enabled?

[Severity: High]
This isn't a bug introduced by this patch, but does fsi_dai_trigger() run in
atomic context since .nonatomic = 1 is not set on the DAI?

If so, is it safe to call fsi_hw_startup() which calls fsi_clk_enable() and
subsequently clock->set_rate() (which calls clk_set_rate())?

clk_set_rate() acquires a sleepable mutex:
fsi_dai_trigger() -> fsi_hw_startup() -> fsi_clk_enable() -> clk_set_rate()

Could this trigger a scheduling while atomic panic?

[Severity: High]
This is a pre-existing issue, but looking at fsi_clk_enable(), if a second
stream tries to start while one is already active (clock->count > 0), does
the function return -EINVAL instead of 0?

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

Could this permanently fail full-duplex operations by preventing
simultaneous playback and capture?

Also, could the lack of atomic protection on clock->count lead to races
between independent substream triggers?

>  		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 isn't a bug introduced by this patch, but fsi_stream_quit() sets
io->substream = NULL under master->lock.

Is there a race window with asynchronous tasks like the IRQ handler or DMA
callbacks?

fsi_pio_transfer() checks fsi_stream_is_working() which verifies
io->substream but drops the lock before returning:

sound/soc/renesas/fsi.c:fsi_pio_transfer() {
	if (!fsi_stream_is_working(fsi, io))
		return -EINVAL;
	buf = fsi_pio_get_area(fsi, io);
	...
}

Could fsi_stream_quit() nullify io->substream immediately after the lock is
dropped, causing fsi_pio_get_area() to dereference a NULL pointer when
accessing io->substream->runtime?

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

[Severity: High]
This is a pre-existing issue, but since fsi_hw_shutdown() disables the SPU
hardware clock, could this cause a hardware hang if the IRQ handler is still
running concurrently on another CPU?

Without synchronize_irq() to wait for completion, couldn't fsi_interrupt()
access hardware registers like CPU_INT_ST or SOFT_RST after the clock is
disabled, resulting in an external abort or bus fault?

>  		break;
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but does fsi_remove() prematurely
destroy hardware channels before devm tears down the ALSA interface?

sound/soc/renesas/fsi.c:fsi_remove() {
	...
	fsi_stream_remove(&master->fsia);
	fsi_stream_remove(&master->fsib);
	...
}

Since the ALSA component is registered using
devm_snd_soc_register_component(), it is unregistered later during the
devres cleanup phase.

Could userspace trigger operations on the still-visible PCM devices that
call dmaengine_prep_dma_cyclic() with a NULL io->chan, resulting in a
use-after-free during teardown?

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