Re: [PATCH v4 06/10] ASoC: renesas: fsi: Move fsi_clk_init()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The newly introduced `fsi_stream_is_working()` function creates a Time-of-Check to Time-of-Use (TOCTOU) race condition because it drops the lock before returning.
- [High] The patchset attempts to prevent system hangs from in-flight IRQs when clocks are disabled, but fails because the interrupt handler still unconditionally accesses hardware registers before any stream status checks.
--

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

ASoC: renesas: fsi: Move fsi_clk_init()

This patch moves the fsi_clk_init() function after set_rate() functions to
prepare for subsequent refactoring.

> diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
> index 0f350bddeb1d5..43e6772c06d5d 100644
> --- a/sound/soc/renesas/fsi.c
> +++ b/sound/soc/renesas/fsi.c

[ ... ]

While reviewing this patch, I noticed a couple of pre-existing issues in
this file.

sound/soc/renesas/fsi.c:fsi_stream_is_working()
{
	struct fsi_master *master = fsi_get_master(fsi);

	guard(spinlock_irqsave)(&master->lock);

	return !!(io->substream && io->substream->runtime);
}

[Severity: High]
This isn't a bug introduced by this patch, but does this function introduce a
Time-of-Check to Time-of-Use (TOCTOU) race condition?

The fsi_stream_is_working() function checks if io->substream is active
while holding master->lock, but immediately drops the lock using a scoped
guard before returning.

In the interrupt handler, fsi_count_fifo_err() and fsi_irq_clear_status()
call this function and proceed to read hardware registers without holding any
locks if it returns true:

sound/soc/renesas/fsi.c:fsi_count_fifo_err()
{
	if (!fsi_stream_is_working(fsi, &fsi->playback) &&
	    !fsi_stream_is_working(fsi, &fsi->capture))
		return;

	u32 ostatus = fsi_reg_read(fsi, DOFF_ST);
	...
}

If a concurrent SNDRV_PCM_TRIGGER_STOP on another CPU executes
fsi_hw_shutdown() (which disables the SPU clock) right after
fsi_stream_is_working() returns true but before the register accesses,
can this lead to a fatal system hang when the interrupt handler accesses
registers with the disabled clock?

[ ... ]

sound/soc/renesas/fsi.c:fsi_interrupt()
{
	struct fsi_master *master = data;
	u32 int_st = fsi_irq_get_status(master);

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

[Severity: High]
This isn't a bug introduced by this patch, but does the interrupt handler
unconditionally access hardware registers before any stream status checks?

The commit message for a previous patch claims that the IRQ handler will
return immediately if no streams are active. However, fsi_interrupt()
unconditionally calls fsi_irq_get_status() and fsi_master_mask_set() at
the very beginning of the function, before any stream checks are performed.

These functions read and write the INT_ST and SOFT_RST registers, which
sit behind the SPU bus. If an in-flight IRQ executes after the SPU clock has
been disabled, will these initial register accesses immediately hang the
system, completely bypassing the downstream fsi_stream_is_working() checks?

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