Re: [PATCH v4 08/10] 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] Missing cleanup for previously allocated FSIA DMA channels if FSIB clock initialization fails in probe.

Pre-existing issues:
- [High] The DAI trigger callback executes sleeping clock operations while holding a PCM spinlock.
--

commit 7a2af4d59dffa88627e517f84d651b199bf13ef8
Author: bui duc phuc <phucduc.bui@gmail.com>

ASoC: renesas: fsi: refactor clock initialization

This commit moves the fsi_clk_init() function call from fsi_dai_set_fmt()
into the fsi_probe() path. It aims to ensure that clock resources are
validated early during the driver probe rather than deferring the lookup
to hardware configuration.

> diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
> index a2d7d17dd2bb0..3f303e15e8351 100644
> --- a/sound/soc/renesas/fsi.c
> +++ b/sound/soc/renesas/fsi.c

[ ... ]

> @@ -898,6 +904,11 @@ static int fsi_clk_set_rate_cpg(struct device *dev,
>  	int ackmd, bpfmd;
>  	int ret = -EINVAL;
>  
> +	if (!ick || !div) {
> +		dev_err(dev, "ick clock or div clock is missing\n");
> +		return -EINVAL;
> +	}
> +

[Severity: High]
This isn't a bug introduced by this patch, but does this clock initialization
path ultimately lead to sleeping while atomic?

The ASoC framework invokes fsi_dai_trigger() in an atomic context with the
PCM stream spinlock held and IRQs disabled. For the start command, the call
chain executes:

fsi_dai_trigger()
  fsi_hw_startup()
    fsi_clk_enable()
      fsi_clk_set_rate_cpg()
        clk_set_rate()

clk_set_rate() attempts to acquire the global prepare_lock mutex in the common
clock framework, which will trigger a scheduling-while-atomic panic when
called from the atomic trigger path.

[ ... ]

> @@ -1985,6 +2000,9 @@ static int fsi_probe(struct platform_device *pdev)
>  	fsi->master	= master;
>  	fsi_port_info_init(fsi, &info.port_b);
>  	fsi_handler_init(fsi, &info.port_b);
> +	ret = fsi_clk_init(&pdev->dev, fsi);
> +	if (ret)
> +		return ret;
>  	ret = fsi_stream_probe(fsi, &pdev->dev);
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "FSIB stream probe failed\n");

[Severity: High]
If this fails, does the direct return bypass the cleanup for FSIA?

Earlier in fsi_probe(), FSIA is probed and successfully allocates
non-devres managed DMA channels. If fsi_clk_init() subsequently fails for
FSIB (for example, returning -EPROBE_DEFER for an unavailable clock), the
error path directly returns instead of executing the cleanup routine.

Would this lead to a permanent leak of the DMA channels and other stream
resources allocated for FSIA? Might a goto statement targeting the exit_fsia
label be needed here so that fsi_stream_remove() can safely clean up?

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