Re: [PATCH v5 08/11] ASoC: renesas: fsi: refactor clock initialization

[sashiko-bot@kernel.org]

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

New issues:
- [High] Unbalanced pm_runtime_disable() on the fsi_clk_init() error path before pm_runtime_enable() is called permanently breaks device PM on deferred probe.

Pre-existing issues:
- [High] Concurrent execution of playback and capture triggers causes lockless read-modify-write races on port registers and corrupts clock refcounting.
--

commit 3878a75703ac4c551d532bdff9ddca5b624ed448
Author: bui duc phuc <phucduc.bui@gmail.com>

ASoC: renesas: fsi: refactor clock initialization

Move fsi_clk_init() from set_fmt() to the probe path. This ensures that
clock resources are acquired only once during device initialization.

> diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
> index e29be2bcf952c..13988c7dc926e 100644
> --- a/sound/soc/renesas/fsi.c
> +++ b/sound/soc/renesas/fsi.c

[ ... ]

> @@ -1996,6 +1996,9 @@ static int fsi_probe(struct platform_device *pdev)
>  		dev_err(&pdev->dev, "FSIB stream probe failed\n");
>  		goto exit_fsia;
>  	}
> +	ret = fsi_clk_init(&pdev->dev, fsi);
> +	if (ret)
> +		goto exit_fsib;

[Severity: High]
If fsi_clk_init() fails here, we jump to exit_fsib before
pm_runtime_enable() is called.

At the exit_fsib label, pm_runtime_disable() is called unconditionally. Will
this cause an unbalanced pm_runtime_disable() call, incrementing the
device's disable_depth to 2?

If this happens during a probe deferral (e.g. -EPROBE_DEFER because a clock
is not yet available), the subsequent probe attempt's pm_runtime_enable()
will only decrement the depth to 1. Does this leave PM runtime permanently
disabled and break all future hardware initialization?

>  
>  	pm_runtime_enable(&pdev->dev);
>  	dev_set_drvdata(&pdev->dev, master);

[Severity: High]
This is a pre-existing issue, but I noticed a potential race condition with
concurrent execution of playback and capture triggers.

ALSA core holds individual substream locks, meaning ALSA trigger callbacks
can be invoked concurrently for playback and capture substreams on the same
DAI.

In fsi_dai_trigger(), both start operations call fsi_hw_startup(), which
performs lockless read-modify-write operations on shared port registers:

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

Could Thread A set ACKMD bits while Thread B concurrently sets DIMD bits on
the same register, causing one of the updates to be lost?

Additionally, fsi_clk_enable() uses a shared clock->count without atomic
operations or locking:

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

Could a concurrent increment of clock->count lead to both streams calling
clk_enable() but clock->count only reaching 1? On shutdown, clk_disable()
would only be called once, potentially leaving unbalanced clock refcounts in
the common clock framework and permanently leaking the clock reference.

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