Re: [PATCH v3 10/10] ASoC: renesas: fsi: Add SPU clock control in hw_startup/shutdown

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Unreachable SPU clock disable logic for clock masters leads to resource leak.
- [High] Shared use of `spu_count` for both clock preparation and enablement causes API violations and clock state corruption.
- [High] Missing rollback of SPU clock enablement on error path causes resource leak.
- [High] Unhandled error in `__fsi_resume` leads to system hang if clock enable fails.
- [Medium] Unprotected read of `spu_count` causes a data race.
- [Low] Misleading indentation in `fsi_hw_startup()`.
--

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

ASoC: renesas: fsi: Add SPU clock control in hw_startup/shutdown

This commit enables and disables the SPU clock in fsi_hw_startup()
and fsi_hw_shutdown() to prevent system hangs caused by hardware
register accesses while the clock is disabled.

> diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
> index c3ac8b30f342b..5307085c06158 100644
> --- a/sound/soc/renesas/fsi.c
> +++ b/sound/soc/renesas/fsi.c
> @@ -1579,6 +1579,19 @@ static int fsi_hw_startup(struct fsi_priv *fsi,
>  			  struct device *dev)
>  {
>  	u32 data = 0;
> +	int ret;
> +
> +	/* enable spu clock */
> +	if (fsi->master->clk_spu) {
> +		scoped_guard(spinlock_irqsave, &fsi->master->lock) {
> +			if (fsi->master->spu_count == 0) {
> +				ret = clk_enable(fsi->master->clk_spu);
> +				if (ret < 0)
> +					return ret;

If clk_enable() fails and returns an error here, how does __fsi_resume()
handle it? Looking at __fsi_resume(), it ignores the return value of
fsi_hw_startup() and unconditionally calls fsi_stream_start().

Could this attempt to access hardware registers and trigger a system hang if
the SPU clock failed to enable?

> +			}
> +				fsi->master->spu_count++;

This isn't a bug, but the indentation of fsi->master->spu_count++ makes it
appear as though it is conditionally executed inside the if block.

Does reusing fsi->master->spu_count here conflict with its use in
fsi_clk_prepare() and fsi_clk_unprepare()?

Since fsi_clk_prepare() checks spu_count without the fsi->master->lock,
could concurrent stream operations cause a data race?

Also, consider this sequence with concurrent streams:

Stream 1 opens: spu_count is 0, calls clk_prepare()
Stream 1 starts: spu_count is 0, calls clk_enable(), spu_count becomes 1
Stream 2 opens: spu_count is 1, skips clk_prepare()
Stream 1 stops: spu_count drops to 0, calls clk_disable()
Stream 1 closes: spu_count is 0, calls clk_unprepare()
Stream 2 starts: spu_count is 0, calls clk_enable()

Will Stream 2 try to call clk_enable() on an unprepared clock? Could a
separate counter be used for enablement to avoid breaking the clock state
machine?

> +		}
> +	}

[ ... ]

>  	/* start master clock */
>  	if (fsi_is_clk_master(fsi))
>  		return fsi_clk_enable(dev, fsi);

If fsi_clk_enable() fails here, does this code leak the SPU clock enablement?

Since the function returns the error directly, is there a need to call
clk_disable(fsi->master->clk_spu) and decrement fsi->master->spu_count
before returning?

>  
>  	return 0;
>  }
> @@ -1636,6 +1649,12 @@ static int fsi_hw_shutdown(struct fsi_priv *fsi,
>  	/* stop master clock */
>  	if (fsi_is_clk_master(fsi))
>  		return fsi_clk_disable(dev, fsi);
> +	if (fsi->master->clk_spu) {
> +		scoped_guard(spinlock_irqsave, &fsi->master->lock) {
> +			if (--fsi->master->spu_count == 0)
> +				clk_disable(fsi->master->clk_spu);
> +		}
> +	}
>  
>  	return 0;
>  }

Does this bypass the SPU clock disable logic entirely when the FSI device
operates as a clock master?

Since fsi_clk_disable() returns directly, it looks like fsi->master->clk_spu
is never disabled and fsi->master->spu_count remains unbalanced.

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