Re: [PATCH v6 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 4 potential issue(s) to consider:

New issues:
- [High] Double disable of the SPU clock on the error path.

Pre-existing issues:
- [High] `fsi_clk_enable()` unconditionally fails subsequent streams (breaks full-duplex).
- [High] Sleeping in atomic context during trigger.
- [High] Concurrent triggers race on shared hardware registers.
--

commit f5cfbced3eb2033737642bc08abc86f7381ec9a6
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(). This ensures the clock is active while the driver
accesses hardware registers.

> diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
> index e26f39dfe0598..b72396b5de7dc 100644
> --- 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)
> +		return ret;

[Severity: High]
This is a pre-existing issue, but does this code have a race condition when
triggered concurrently?

Playback and capture streams use the same fsi_priv instance and share
hardware registers like CKG1. Since they are separate substreams with
separate ALSA locks, starting or stopping them concurrently on different
CPUs could cause fsi_hw_startup() and fsi_hw_shutdown() to run in parallel.

This could lead to unprotected read-modify-write operations on shared
registers:

    fsi_reg_mask_set(fsi, CKG1, (DIMD | DOMD), data);

and on the shared clock count:

    fsi_clk_enable() {
        ...
        clock->count++;
    }

Could this corrupt the clock reference count and hardware register states?

> [ ... ]
> @@ -1605,8 +1610,13 @@ static int fsi_hw_startup(struct fsi_priv *fsi,
>  	fsi_fifo_init(fsi, io, dev);
>  
>  	/* start master clock */
> -	if (fsi_is_clk_master(fsi))
> -		return fsi_clk_enable(dev, fsi);
> +	if (fsi_is_clk_master(fsi)) {
> +		ret = fsi_clk_enable(dev, fsi);

[Severity: High]
This is a pre-existing issue, but does fsi_clk_enable() break full-duplex
operation?

If a stream is already running, clock->count is 1. When a second stream
starts, the condition if (0 == clock->count) is false, but the function
does not set ret = 0.

    fsi_clk_enable() {
        int ret = -EINVAL;
        if (!fsi_clk_is_valid(fsi))
            return ret;
            
        if (0 == clock->count) {
            ...
        }
        return ret;
    }

Won't this unconditionally return -EINVAL for the second stream, causing
fsi_hw_startup() to fail and preventing simultaneous playback and capture?

[Severity: High]
This is also a pre-existing issue, but could calling fsi_clk_enable() here
cause a sleep in atomic context?

The ALSA trigger callback runs in atomic context with the PCM substream
spinlock held. fsi_clk_enable() calls clock->set_rate(), which routes to
fsi_clk_set_rate_cpg():

    fsi_clk_set_rate_cpg() {
        ...
        ret = clk_set_rate(ick, best_cout);
    }

Since the Common Clock Framework's clk_set_rate() acquires a mutex and
sleeps, won't this cause a scheduling while atomic panic?

> +		if (ret) {
> +			clk_disable(fsi->master->clk_spu);
> +			return ret;
> +		}
> +	}
>  
>  	return 0;
>  }
> @@ -1614,9 +1624,15 @@ static int fsi_hw_startup(struct fsi_priv *fsi,
>  static int fsi_hw_shutdown(struct fsi_priv *fsi,
>  			    struct device *dev)
>  {
> +	int ret;
>  	/* stop master clock */
> -	if (fsi_is_clk_master(fsi))
> -		return fsi_clk_disable(dev, fsi);
> +	if (fsi_is_clk_master(fsi)) {
> +		ret = fsi_clk_disable(dev, fsi);
> +		if (ret)
> +			return ret;
> +	}
> +	/* stop spu bus bridge clock */
> +	clk_disable(fsi->master->clk_spu);
>  
>  	return 0;
>  }

[Severity: High]
Is there a risk of double-disabling the SPU clock if fsi_clk_enable() fails?

If fsi_clk_enable() fails during startup, it disables the SPU clock before
returning the error:

    fsi_hw_startup() {
        ...
        if (ret) {
            clk_disable(fsi->master->clk_spu);
            return ret;
        }
    }

ALSA intercepts this error in snd_pcm_action_single() and invokes
snd_pcm_undo_start(), which calls the driver's trigger callback with
SNDRV_PCM_TRIGGER_STOP.

This triggers fsi_hw_shutdown(), which unconditionally calls clk_disable()
on the SPU clock again, since fsi_clk_disable() returns 0 for a zero clock
count.

Could this result in an unbalanced clock disable, potentially triggering
warnings and preventing the clock from being re-enabled later?

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