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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but fsi_clk_enable() incorrectly handles reference counting and returns -EINVAL for subsequent streams, breaking concurrent playback and capture (full-duplex).
- [High] This problem wasn't introduced by this patch, but fsi_clk_enable() triggers sleepable mutex acquisitions (clk_set_rate) from inside an atomic trigger context, potentially causing a deadlock or kernel panic.
- [High] This problem wasn't introduced by this patch, but opening or closing an audio stream unconditionally invalidates the shared clock state, breaking any other active stream on the same DAI.
--

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

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

This commit adds fsi_clk_prepare() and fsi_clk_unprepare() helpers and calls
them from the startup and shutdown routines. This ensures clocks are prepared
from sleepable contexts.

> diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
> index 716ecf0401fe3..e26f39dfe0598 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)
> +{
[ ... ]
> +	clk_unprepare(spu);
> +}

[Severity: High]
This isn't a bug introduced by this patch, but does fsi_clk_enable() correctly
handle reference counting for subsequent streams?

Looking at fsi_clk_enable() in the same file:

static int fsi_clk_enable(struct device *dev, struct fsi_priv *fsi)
{
        struct fsi_clk *clock = &fsi->clock;
        int ret = -EINVAL;
        ...
        if (0 == clock->count) {
                ...
                clock->count++;
        }

        return ret;
}

If clock->count is greater than 0 (such as during a second stream start for
concurrent playback and capture), the if block is skipped and the function
directly returns the initialized ret value of -EINVAL. Does this break
full-duplex audio by failing the ALSA trigger operation for the second stream?

Additionally, if the second stream fails to start but later triggers an
unbalanced shutdown during cleanup, fsi_clk_disable() underflows clock->count
via the post-decrement:

static int fsi_clk_disable(struct device *dev, struct fsi_priv *fsi)
{
        ...
        if (1 == clock->count--) {
                clk_disable(clock->xck);
                ...
        }
        ...
}

Could this underflow cause further clock enablement to fail?

[Severity: High]
This is also a pre-existing issue, but does fsi_dai_trigger() call sleepable
functions while in an atomic context?

The ALSA ASoC trigger callback runs in an atomic context under a spinlock
since the DAI and PCM do not have the nonatomic flag set. However, the
call chain leads to clk_set_rate() which acquires the global CCF prepare_lock
mutex and can sleep:

fsi_dai_trigger()
  fsi_hw_startup()
    fsi_clk_enable()
      clock->set_rate()
        clk_set_rate()

Could this attempt to acquire a mutex while holding a spinlock result in a
scheduling while atomic panic or deadlock?

>  static int fsi_clk_enable(struct device *dev,
>  			  struct fsi_priv *fsi)
>  {
[ ... ]
> @@ -1580,7 +1628,7 @@ static int fsi_dai_startup(struct snd_pcm_substream *substream,
>  
>  	fsi_clk_invalid(fsi);
>  
> -	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);
>  	fsi_clk_invalid(fsi);

[Severity: High]
This is a pre-existing issue, but does calling fsi_clk_invalid()
unconditionally here and in fsi_dai_startup() break other active streams
on the same DAI?

Because playback and capture streams share a single fsi_clk structure,
if one stream is already running and a second stream is opened or closed,
fsi_clk_invalid() zeroes out fsi->clock.rate.

Later, when the running stream is closed via fsi_dai_trigger(),
fsi_hw_shutdown() will call fsi_clk_disable(). Because the rate was wiped,
fsi_clk_is_valid() evaluates to false:

static int fsi_clk_disable(struct device *dev, struct fsi_priv *fsi)
{
        if (!fsi_clk_is_valid(fsi))
                return -EINVAL;
        ...
}

This causes fsi_clk_disable() to return early without decrementing
clock->count or disabling the hardware clocks. Does this leak the clocks
indefinitely and break the reference count for future stream startups?

>  }

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