Re: [PATCH v5 10/11] ASoC: renesas: fsi: add fsi_clk_prepare/unprepare()

[sashiko-bot@kernel.org]

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

New issues:
- [High] The interrupt handler `fsi_interrupt` reads and writes hardware registers before verifying if any streams are active, risking system hangs.

Pre-existing issues:
- [High] `fsi_clk_enable()` incorrectly returns `-EINVAL` and improperly manages `clock->count` when a second concurrent stream is started, breaking full-duplex operation.
- [High] The driver calls `clk_set_rate` and `clk_set_parent` from the atomic PCM trigger callback, causing a 'scheduling while atomic' bug.
- [High] Opening or closing a stream resets the shared clock rate via `fsi_clk_invalid()`, corrupting the state of an already running concurrent stream and causing permanent clock reference leaks.
- [High] Lockless access to the shared `clock->count` variable leads to race conditions and unbalanced clock reference counts when playback and capture are triggered concurrently.
--

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

ASoC: renesas: fsi: add fsi_clk_prepare/unprepare()

Add fsi_clk_prepare() and fsi_clk_unprepare() helpers and call them
from fsi_dai_startup() and fsi_dai_shutdown(). This ensures clk_prepare()
and clk_unprepare() are executed from sleepable contexts and keeps clocks
prepared only while audio streams are active.

> diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
> index 6c432c7235a41..80e87b815c1b6 100644
> --- a/sound/soc/renesas/fsi.c
> +++ b/sound/soc/renesas/fsi.c
> @@ -730,6 +730,54 @@ static int fsi_clk_is_valid(struct fsi_priv *fsi)
>  		fsi->clock.rate;
>  }
>  
> +static int fsi_clk_prepare(struct fsi_priv *fsi)
[ ... ]
> +static void fsi_clk_unprepare(struct fsi_priv *fsi)
[ ... ]
> +}
> +
>  static int fsi_clk_enable(struct device *dev,
>  			  struct fsi_priv *fsi)
>  {

[Severity: High]
This is a pre-existing issue, but does fsi_clk_enable() break when a second
concurrent stream is started?

In fsi_clk_enable():

	int ret = -EINVAL;

	if (!fsi_clk_is_valid(fsi))
		return ret;

	if (0 == clock->count) {
		...
	}

	return ret;

If clock->count > 0, such as when starting a capture stream while playback is
already active, the zero check is skipped entirely. The function then returns
the unassigned ret (-EINVAL), causing the ALSA trigger callback to fail.

[Severity: High]
This isn't a bug introduced by this patch, but does fsi_clk_enable() cause a
"scheduling while atomic" bug?

The ALSA fsi_dai_trigger() callback executes in an atomic context with the PCM
substream spinlock held and interrupts disabled. On a START trigger, it calls
fsi_hw_startup(), which invokes fsi_clk_enable():

	if (0 == clock->count) {
		ret = clock->set_rate(dev, fsi);

The clock->set_rate() functions invoke clk_set_rate() and clk_set_parent(),
which acquire the clk_prepare_lock mutex and can sleep.

Calling them from an atomic context causes a BUG. While this patch moves
clk_prepare() to the sleepable startup callback, rate setting remains in the
atomic trigger path.

[Severity: High]
This is a pre-existing issue, but could lockless access to the shared
clock->count variable lead to race conditions?

The fsi_priv struct and its clock->count member are shared between playback
and capture substreams on the same DAI. ALSA PCM trigger callbacks are
serialized per-substream, not per-DAI, allowing playback and capture triggers
to execute concurrently.

fsi_clk_enable() checks if count is zero and increments it without any
locking. If both streams start concurrently, both CPUs can observe count == 0,
execute the clk_enable logic simultaneously, and increment count to 2, but
clk_enable() was called twice for each clock.

When stopping, fsi_clk_disable() uses `if (1 == clock->count--)`. The first
stop drops count from 2 to 1 and does not call clk_disable(). The second stop
drops count from 1 to 0 and calls clk_disable() once. This leaves the clock
enable counts permanently unbalanced by +1.

> @@ -1580,7 +1628,7 @@ static int fsi_dai_startup(struct snd_pcm_substream *substream,
>  
>  	fsi_clk_invalid(fsi);

[Severity: High]
This is a pre-existing issue, but does calling fsi_clk_invalid() here corrupt
the state of concurrent active streams?

Because fsi_priv is shared between playback and capture, opening a new stream
while another is actively running overwrites the operating stream's clock rate
to 0.

When the active stream is eventually stopped, fsi_hw_shutdown() calls
fsi_clk_disable(). Because fsi_clk_is_valid() relies on the now zeroed
clock->rate, it evaluates to false.

This causes fsi_clk_disable() to return -EINVAL immediately without
decrementing clock->count or disabling the hardware clocks, resulting in a
permanent clock enable leak.

>  
> -	return 0;
> +	return fsi_clk_prepare(fsi);
>  }
>  
>  static void fsi_dai_shutdown(struct snd_pcm_substream *substream,
> @@ -1588,6 +1636,7 @@ static void fsi_dai_shutdown(struct snd_pcm_substream *substream,
>  {
>  	struct fsi_priv *fsi = fsi_get_priv(substream);
>  
> +	fsi_clk_unprepare(fsi);

[Severity: High]
Could unpreparing the clock here risk a system hang in the interrupt handler?

If an in-flight or spurious IRQ fires after streams are shut down, the
fsi_interrupt() handler executes. If the SPU clock is disabled, the handler
immediately accesses hardware registers before verifying if any streams are
active:

    u32 int_st = fsi_irq_get_status(master);

    /* clear irq status */
    fsi_master_mask_set(master, SOFT_RST, IR, 0);
    fsi_master_mask_set(master, SOFT_RST, IR, IR);

Accessing INT_ST and SOFT_RST registers with the SPU clock gated will cause
the system to hang.

>  	fsi_clk_invalid(fsi);
>  }

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