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

[sashiko-bot@kernel.org]

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

New issues:
- [High] The `clk_spu` clock reference is permanently leaked due to an early return in the shutdown path and a missing error cleanup in the startup path.

Pre-existing issues:
- [High] Data race on the shared clock reference counter `clock->count` between concurrent playback and capture triggers.
- [High] Sleeping in atomic context caused by calling `clk_set_rate()` within the ALSA DAI trigger callback.
--

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

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

This patch enables and disables the SPU clock in fsi_hw_startup() and
fsi_hw_shutdown() to ensure the clock is active while the driver accesses
hardware registers.

> diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
> --- a/sound/soc/renesas/fsi.c
> +++ b/sound/soc/renesas/fsi.c
> @@ -1560,6 +1560,11 @@ static int fsi_hw_startup(struct fsi_priv *fsi,
>  			  struct device *dev)
>  {
>  	u32 data = 0;
> +	int ret;
> +	/* enable spu bus bridge clock */
> +	ret = clk_enable(fsi->master->clk_spu);
> +	if (ret < 0)
> +		return ret;
>  
>  	/* clock setting */

[Severity: High]
This is a pre-existing issue, but calling clk_enable() and interacting with
the clock API from the startup path seems to lead to sleeping in an atomic
context.

The ALSA core invokes the fsi_dai_trigger() callback with the substream
stream_lock spinlock held and hardware interrupts disabled. Inside this
atomic context, the call chain leads to sleepable clock setup functions:

fsi_dai_trigger()
   fsi_hw_startup()
      fsi_clk_enable()
         ret = clock->set_rate(dev, fsi);

For example, fsi_clk_set_rate_cpg() executes clk_set_rate(), which
acquires a mutex. Will taking a mutex while holding a spinlock with
interrupts disabled trigger a sleeping in atomic context warning or a
potential deadlock?

Furthermore, later in fsi_hw_startup(), the function ends with:

    if (fsi_is_clk_master(fsi))
        return fsi_clk_enable(dev, fsi);

If fsi_clk_enable() returns an error, the function exits without disabling
the successfully enabled clk_spu. Does this need an error handling path to
avoid leaking the clock reference?

[ ... ]

> @@ -1617,6 +1622,8 @@ static int fsi_hw_shutdown(struct fsi_priv *fsi,
>  	/* stop master clock */
>  	if (fsi_is_clk_master(fsi))
>  		return fsi_clk_disable(dev, fsi);
> +	/* stop spu bus bridge clock */
> +	clk_disable(fsi->master->clk_spu);
>  
>  	return 0;
>  }

[Severity: High]
If fsi_is_clk_master(fsi) is true, the function returns early and completely
bypasses the newly added clk_disable(fsi->master->clk_spu) call. Will this
cause the SPU clock reference to leak when stopping the audio stream?

[Severity: High]
This is a pre-existing issue, but I noticed a potential data race on the
shared clock reference counter clock->count when starting and stopping
the streams.

Because the ALSA core allows independent and concurrent triggering of
playback and capture substreams on the same DAI, fsi_hw_startup() and
fsi_hw_shutdown() can be executed in parallel by different CPUs.

In fsi_clk_enable(), clock->count is read and incremented locklessly:

    if (0 == clock->count) {
        ret = clock->set_rate(dev, fsi);
        ...
        clock->count++;
    }

Similarly, fsi_clk_disable() decrements it without synchronization:

    if (1 == clock->count--) {
        clk_disable(clock->xck);
        ...
    }

Could this race condition lead to unbalanced clock reference counts and fail
to properly enable or disable the hardware clocks?

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