Re: [PATCH v5 09/11] ASoC: renesas: fsi: Add SPU clock support

[sashiko-bot@kernel.org]

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

New issues:
- [Low] Redundant device tree clock lookup and devres allocation for missing optional clock.

Pre-existing issues:
- [High] TOCTOU race in interrupt processing leading to NULL pointer dereference and unclocked register accesses.
--

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

ASoC: renesas: fsi: Add SPU clock support

FSI register accesses on the r8a7740 require the SPU bus clock to be
enabled. Add support for acquiring and managing the SPU clock via the
device tree to ensure proper register access.

> diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
> index 13988c7dc926e..6c432c7235a41 100644
> --- a/sound/soc/renesas/fsi.c
> +++ b/sound/soc/renesas/fsi.c
[ ... ]
> @@ -1004,6 +1006,13 @@ static int fsi_clk_init(struct device *dev, struct fsi_priv *fsi)
>  	if (IS_ERR(clock->own))
>  		return dev_err_probe(dev, PTR_ERR(clock->own), "Can't get fck clock\n");
>  
> +	if (!master->clk_spu) {
> +		master->clk_spu = devm_clk_get_optional(dev, "spu");
> +		if (IS_ERR(master->clk_spu))
> +			return dev_err_probe(dev, PTR_ERR(master->clk_spu),
> +					"Can't get spu clock\n");
> +	}
> +

[Severity: Low]
Does this code result in redundant device tree lookups and devres allocations
if the optional clock is missing?

Since fsi_clk_init() is called twice (once for FSIA and once for FSIB), if the
optional "spu" clock is not in the device tree, devm_clk_get_optional()
returns NULL.

The second time fsi_clk_init() is called, the (!master->clk_spu) check will
pass again, causing another devm_clk_get_optional() lookup and allocating a
second devres tracking node for a NULL clock pointer.

Could the clock acquisition be moved to a master-level initialization function
instead?

[Severity: High]
This is a pre-existing issue, but does a TOCTOU race condition exist between
interrupt processing and ALSA trigger stops that could lead to a NULL pointer
dereference or external abort?

In the interrupt handler path, functions like fsi_pio_transfer() and
fsi_count_fifo_err() check stream activity by calling fsi_stream_is_working():

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

	buf = fsi_pio_get_area(fsi, io);
    ...
}

However, fsi_stream_is_working() acquires master->lock, checks if
io->substream is valid, and immediately drops the lock before returning.

Concurrently, ALSA core can call fsi_dai_trigger(SNDRV_PCM_TRIGGER_STOP),
which invokes fsi_stream_quit(). This function takes master->lock, sets
io->substream to NULL, drops the lock, and disables the hardware clocks.

If CPU2 stops the stream right after CPU1's fsi_stream_is_working() drops the
lock, CPU1 can then call fsi_pio_get_area() and unconditionally dereference
io->substream->runtime, causing a fatal NULL pointer dereference.

Similarly, in fsi_count_fifo_err(), the same race window exists before
calling fsi_reg_read(fsi, DOFF_ST), which can trigger a synchronous external
abort if the clock has already been disabled by CPU2.

Could we hold the lock across the entire interrupt processing sequence, or
validate io->substream safely under the lock without dropping it prematurely?

>  	/* external clock */
>  	if (xck) {
>  		clock->xck = devm_clk_get_optional(dev, is_porta ? "xcka" : "xckb");

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