Re: [PATCH v4 03/10] 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] Asynchronous background task (DMA completion or hardirq) races with stream teardown, causing a lockless access and NULL pointer dereference.
- [High] The driver calls clk_set_rate() and clk_set_parent() from the ALSA trigger callback, which runs in atomic context with spinlocks held.
--

commit d41629e308808a974831ffabbba6ec2a77dd96e8
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..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;

[Severity: High]
This is a pre-existing issue, but does fsi_dai_trigger() call sleepable
clock functions while holding an atomic spinlock?

The ALSA core invokes the fsi_dai_trigger() callback in atomic context with
the PCM stream spinlock held.

During SNDRV_PCM_TRIGGER_START (handled earlier in this function), it calls
fsi_hw_startup() which enables the clock via fsi_clk_enable(). This
eventually invokes the clock's set_rate function (like fsi_clk_set_rate_cpg()
or fsi_clk_set_rate_external()) and calls clk_set_rate() and/or
clk_set_parent().

Under the Common Clock Framework, clk_set_rate() and clk_set_parent() acquire
the global prepare_lock mutex, which can sleep.

Can this trigger a "scheduling while atomic" bug or deadlock?

>  	case SNDRV_PCM_TRIGGER_STOP:
> +		fsi_stream_stop(fsi, io);
>  		if (!ret)
>  			ret = fsi_hw_shutdown(fsi, dai->dev);
> -		fsi_stream_stop(fsi, io);
>  		fsi_stream_quit(fsi, io);

[Severity: High]
This is also a pre-existing issue, but does this teardown sequence race with
asynchronous DMA completion?

When fsi_stream_stop() invokes dmaengine_terminate_all(), it is an
asynchronous operation and doesn't wait for pending callbacks like
fsi_dma_complete() to finish.

Immediately after this, fsi_stream_quit() executes and sets
io->substream = NULL.

If fsi_dma_complete() runs concurrently on another CPU, it could execute
fsi_pointer_update() which directly dereferences io->substream->runtime
without a NULL check:

fsi_pointer_update() {
    struct snd_pcm_substream *substream = io->substream;
    struct snd_pcm_runtime *runtime = substream->runtime; // NULL dereference
    ...
}

A similar race exists in the PIO path where fsi_pio_transfer() checks
fsi_stream_is_working() under lock, but drops it before accessing
io->substream in fsi_pio_get_area().

Could this lead to a kernel panic?

>  		break;
>  	}

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