Re: [PATCH v3 03/10] ASoC: renesas: fsi: Fix trigger stop ordering

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] TOCTOU race and lack of synchronization in stream stopping leads to NULL pointer dereference.
- [High] Teardown asymmetry in `fsi_remove` leads to use-after-free and NULL pointer dereference.
- [High] Lockless read-modify-write on shared FSI port registers causes hardware corruption.
- [High] Lockless modifications to shared `clock->count` cause reference count corruption.
--

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

ASoC: renesas: fsi: Fix trigger stop ordering

This commit reorders calls in the stop trigger to execute fsi_stream_stop()
before fsi_hw_shutdown(). It ensures register accesses complete before the
clock is disabled to prevent a system hang.

> diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
> index 8cbd7acc26f49..94ab2e4908108 100644
> --- a/sound/soc/renesas/fsi.c
> +++ b/sound/soc/renesas/fsi.c
> @@ -1586,9 +1586,9 @@ static int fsi_dai_trigger(struct snd_pcm_substream *substream, int cmd,
>  			ret = fsi_stream_transfer(io);
>  		break;
>  	case SNDRV_PCM_TRIGGER_STOP:

Playback and capture substreams on the same FSI port share port-level
configuration registers like CKG1, CKG2, and OUT_SEL.

Since the ALSA core serializes triggers at the substream level using
the self_group.lock, playback and capture triggers for the same DAI
can execute concurrently on different CPUs.

Does fsi_hw_startup() perform lockless read-modify-write operations on these
shared registers, and can this lack of exclusion lead to lost writes and
corrupted hardware state?

> +		fsi_stream_stop(fsi, io);

During a stream stop, fsi_stream_stop() disables DMA via
dmaengine_terminate_all() and masks device interrupts, but it does not appear
to wait for in-flight DMA callbacks or IRQ handlers to finish.

Concurrently, an executing IRQ handler calling fsi_pio_transfer() will
temporarily take master->lock in fsi_stream_is_working(), drop it, and then
locklessly access io->substream->runtime inside fsi_pio_get_area().

Similarly, an asynchronous fsi_dma_complete() callback unconditionally
dereferences io->substream in fsi_pointer_update().

Since there is no dmaengine_terminate_sync() or synchronize_irq(), could
fsi_stream_quit() race with these callbacks and clear io->substream,
resulting in a NULL pointer dereference?

The driver uses managed resources during fsi_probe(), meaning the IRQ handler
and ALSA component are unregistered after fsi_remove() returns. However,
fsi_remove() explicitly tears down the DMA streams via fsi_stream_remove(),
which calls dma_release_channel(io->chan) and sets io->chan to NULL.

Because the ALSA component is still fully registered and active while
fsi_remove() completes, userspace can still trigger ALSA operations.

If a STOP trigger occurs during this window, would calling
dmaengine_terminate_all(io->chan) here cause a NULL pointer dereference?

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

The shared clock state for an FSI port is tracked using clock->count inside
fsi_priv.

Since playback and capture triggers for the same port can execute
concurrently on different CPUs, do fsi_clk_enable() and fsi_clk_disable()
manually modify this counter without holding any lock?

Could concurrent updates to clock->count corrupt the counter, potentially
leaving the peripheral clocks enabled permanently or disabling them while a
stream is still active?

> -		fsi_stream_stop(fsi, io);
>  		fsi_stream_quit(fsi, io);
>  		break;
>  	}

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