Re: [PATCH v4 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 4 potential issue(s) to consider:
- [High] The SPU clock is never disabled, and spu_count is leaked, when the FSI operates as a clock master due to an early return in fsi_hw_shutdown().
- [High] Missing rollback of SPU clock and spu_count on fsi_clk_enable() failure during fsi_hw_startup().
- [High] Overloading spu_count breaks the clock prepare/unprepare lifecycle logic, leading to clk_enable() on an unprepared clock.
- [Medium] Data race on fsi->master->spu_count in fsi_clk_prepare() and fsi_clk_unprepare().
--

commit ce2afa5bc93fefd1e8f98233a0baba78b987617d
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 within fsi_hw_startup() and
fsi_hw_shutdown() to keep the clock active during hardware register accesses.
It tracks the clock state using fsi->master->spu_count.

> diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
> index 6537321242c04..d7cc5a14a0998 100644
> --- a/sound/soc/renesas/fsi.c
> +++ b/sound/soc/renesas/fsi.c
> @@ -1585,6 +1585,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;
> +                       }
> +                               fsi->master->spu_count++;

[Severity: High]
Does tracking active stream starts with spu_count interfere with the existing
clock prepare logic?

Before this patch, fsi_clk_prepare() and fsi_clk_unprepare() checked if
spu_count == 0 before calling clk_prepare() and clk_unprepare().

By modifying spu_count during trigger start and stop, a second stream opening
while the first is running might observe spu_count > 0 and skip clk_prepare().

Could this lead to calling clk_enable() on an unprepared clock?

[Severity: Medium]
Since spu_count is now dynamically modified here under the fsi->master->lock,
is it safe that fsi_clk_prepare() and fsi_clk_unprepare() read spu_count
without acquiring this lock?

Could alsa pcm open and close operations running concurrently with trigger
operations result in a data race?

> +               }
> +       }

[ ... ]

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

[Severity: High]
If fsi_clk_enable() fails here, the function returns the error immediately.

Because the audio core does not execute a stop trigger rollback if the
start trigger fails, fsi_hw_shutdown() will not be called.

Does this regression leave the spu clock erroneously enabled and spu_count
incremented forever?

>  
>          return 0;
>  }
>  
>  static int fsi_hw_shutdown(struct fsi_priv *fsi,
>                             struct device *dev)
>  {
>          /* stop master clock */
>          if (fsi_is_clk_master(fsi))
>                  return fsi_clk_disable(dev, fsi);

[Severity: High]
If the fsi operates as a clock master, this early return bypasses the newly
added spu clock disable logic.

Does this regression permanently leak the spu_count and leave the spu clock
enabled whenever a clock master stream is stopped?

> +       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;
>  }

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