Re: [PATCH v3 09/10] ASoC: renesas: fsi: Use clock prepare handling in startup/shutdown

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] State mismatch between stream open/close (startup/shutdown) and stream start/stop (trigger) causes unbalanced clk_prepare() and clk_unprepare() calls.
- [High] Unbalanced `clk_unprepare()` in the error cleanup path of `fsi_clk_prepare()`.
--

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

ASoC: renesas: fsi: Use clock prepare handling in startup/shutdown

This commit updates the FSI driver to use fsi_clk_prepare() and
fsi_clk_unprepare() during the DAI startup and shutdown phases to
reduce power usage and ensure clock operations run in non-atomic contexts.

> diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
> index b93809b5da15e..c3ac8b30f342b 100644
> --- a/sound/soc/renesas/fsi.c
> +++ b/sound/soc/renesas/fsi.c
> @@ -1644,9 +1644,16 @@ static int fsi_dai_startup(struct snd_pcm_substream *substream,
>  			   struct snd_soc_dai *dai)
>  {
>  	struct fsi_priv *fsi = fsi_get_priv(substream);
> +	int ret;
>  
>  	fsi_clk_invalid(fsi);
>  
> +	if (fsi->clock.count == 0) {
> +		ret = fsi_clk_prepare(fsi);

Is it safe to use clock.count to guard the prepare and unprepare operations
in startup and shutdown?

Since clock.count is incremented and decremented during trigger (stream
start/stop) rather than startup/shutdown (stream open/close), this can lead
to state mismatches in multi-stream or full-duplex operations.

For instance, if Playback is opened and started (count=1), and then Capture
is opened, Capture will see count=1 and skip preparing the clock. If Playback
is then stopped and closed, it will unprepare the clock.

If Capture later starts, it will attempt to enable an unprepared clock,
violating the Common Clock Framework and potentially causing hardware failures.

Similarly, this can cause resource leaks if multiple streams are opened before
starting, or if trigger fails and underflows clock.count to UINT_MAX.

Also, looking at the error path for fsi_clk_prepare() which is called here:

sound/soc/renesas/fsi.c:fsi_clk_prepare() {
	...
	if (!IS_ERR_OR_NULL(spu) && fsi->master->spu_count == 0) {
		ret = clk_prepare(spu);
		if (ret)
			return ret;
	}
	...
err_spu:
	clk_unprepare(spu);

	return ret;
}

If fsi->master->spu_count is non-zero when entering the function,
clk_prepare(spu) is correctly skipped.

However, if a subsequent clock preparation fails, the error path unconditionally
calls clk_unprepare(spu). Does this prematurely decrement the shared SPU
clock's reference count, potentially disabling it while other instances are
still actively using it?

> +		if (ret)
> +			return ret;
> +	}
> +
>  	return 0;
>  }

[ ... ]

> @@ -1655,6 +1662,8 @@ static void fsi_dai_shutdown(struct snd_pcm_substream *substream,
>  {
>  	struct fsi_priv *fsi = fsi_get_priv(substream);
>  
> +	if (fsi->clock.count == 0)
> +		fsi_clk_unprepare(fsi);
>  	fsi_clk_invalid(fsi);
>  }

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