Re: [PATCH v4 09/10] 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:

New issues:
- [High] fsi_clk_prepare() and fsi_clk_unprepare() incorrectly use clock->count to skip operations, leading to leaked clock prepares and underflows.
- [High] The error rollback path in fsi_clk_prepare() unconditionally unprepares the shared SPU clock, and the conditional prepare relies on spu_count which tracks the incorrect state.

Pre-existing issues:
- [High] fsi_clk_enable() breaks full-duplex operation by returning -EINVAL for the second stream.
--

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

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

Add fsi_clk_prepare() and fsi_clk_unprepare() helpers and call them
from fsi_dai_startup() and fsi_dai_shutdown(). This ensures clk_prepare()
and clk_unprepare() are executed from sleepable contexts and keeps clocks
prepared only while audio streams are active.

Suggested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>

> diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
> index 3f303e15e8351..6537321242c04 100644
> --- a/sound/soc/renesas/fsi.c
> +++ b/sound/soc/renesas/fsi.c
> @@ -730,6 +731,78 @@ static int fsi_clk_is_valid(struct fsi_priv *fsi)
>  
> +static int fsi_clk_prepare(struct fsi_priv *fsi)
> +{
> +	struct fsi_clk *clock = &fsi->clock;
> +	struct clk *spu = fsi->master->clk_spu;
> +	struct clk *xck = clock->xck;
> +	struct clk *ick = clock->ick;
> +	struct clk *div = clock->div;
> +	int ret;
> +
> +	if (clock->count != 0)
> +		return 0;

[Severity: High]
Since clock->count tracks the hardware enablement state (modified during ALSA
trigger START/STOP via fsi_clk_enable/disable), could using it here during
the ALSA startup/shutdown (open/close) phase lead to unbalanced clock
prepares?

If Stream A opens, clock->count is 0, and the clocks are prepared.
If Stream A then starts, clock->count becomes 1.

If Stream B opens while Stream A is running, clock->count is 1, so we return
early and skip clk_prepare().

When Stream A stops, clock->count becomes 0.
When Stream A closes, clock->count is 0, so the clocks are unprepared.
When Stream B closes, clock->count is 0, so the clocks are unprepared again,
potentially causing a CCF prepare count underflow.

> +
> +	if (!IS_ERR_OR_NULL(spu) && fsi->master->spu_count == 0) {
> +		ret = clk_prepare(spu);
> +		if (ret)
> +			return ret;
> +	}

[ ... ]

> +err_ick:
> +	clk_unprepare(ick);
> +err_xck:
> +	clk_unprepare(xck);
> +err_spu:
> +	clk_unprepare(spu);
> +
> +	return ret;
> +}

[Severity: High]
If fsi->master->spu_count > 0, we skip preparing the spu clock above.
However, if a subsequent clock (like xck) fails to prepare, we jump to
err_spu and unconditionally call clk_unprepare(spu).

Will this forcefully unprepare the shared spu clock that the first active
stream is currently relying on?

Also, since spu_count tracks START/STOP instead of OPEN/CLOSE, does it
suffer from the same unbalanced prepare/unprepare regression as clock->count?

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but while looking at clock->count
usage, I noticed fsi_clk_enable() appears to prevent full-duplex operation
by returning -EINVAL for the second stream:

sound/soc/renesas/fsi.c:fsi_clk_enable() {
        int ret = -EINVAL;

        if (!fsi_clk_is_valid(fsi))
                return ret;

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

        return ret;
}

If a second stream attempts to start while the first is active
(clock->count == 1), the function skips the block and returns the
initialized -EINVAL. Does this cause the ALSA trigger to fail and prevent
simultaneous playback and capture?

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