Re: [PATCH] ASoC: atmel-i2s: Use devm_clk_get_enabled() for pclk

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] ASoC: atmel-i2s: wrong IRQ and pclk ordering in
  atmel_i2s_probe()
--

commit 722d5e2f3add0fa840d1f0e68c8baa8331615c85
Author: bui duc phuc <phucduc.bui@gmail.com>

ASoC: atmel-i2s: Use devm_clk_get_enabled() for pclk

This commit replaces devm_clk_get() and clk_prepare_enable() for the
peripheral clock with devm_clk_get_enabled(), removing the manual clock
cleanup paths and the driver remove callback.

> diff --git a/sound/soc/atmel/atmel-i2s.c b/sound/soc/atmel/atmel-i2s.c
> index e1d8af0562341..a3aeb8e607f61 100644
> --- a/sound/soc/atmel/atmel-i2s.c
> +++ b/sound/soc/atmel/atmel-i2s.c
> @@ -660,8 +660,8 @@ static int atmel_i2s_probe(struct platform_device *pdev)
>  	if (err)
>  		return err;
>  
> -	/* Get the peripheral clock. */
> -	dev->pclk = devm_clk_get(&pdev->dev, "pclk");
> +	/* Get and enable the peripheral clock. */
> +	dev->pclk = devm_clk_get_enabled(&pdev->dev, "pclk");
>  	if (IS_ERR(dev->pclk))
>  		return dev_err_probe(&pdev->dev, PTR_ERR(dev->pclk),
>  				     "failed to get the peripheral clock\n");

[Severity: Medium]
Can enabling dev->pclk here cause atmel_i2s_interrupt() to dereference a
NULL dev->regmap or access registers while dev->pclk is disabled during
teardown?

atmel_i2s_probe() registers atmel_i2s_interrupt() via devm_request_irq()
just before this call, while dev->regmap and dev->dev are only assigned
after devm_clk_get_optional():

sound/soc/atmel/atmel-i2s.c:atmel_i2s_probe() {
	...
	err = devm_request_irq(&pdev->dev, irq, atmel_i2s_interrupt, 0,
			       dev_name(&pdev->dev), dev);
	if (err)
		return err;

	/* Get and enable the peripheral clock. */
	dev->pclk = devm_clk_get_enabled(&pdev->dev, "pclk");
	...
	dev->gclk = devm_clk_get_optional(&pdev->dev, "gclk");
	if (IS_ERR(dev->gclk))
		return PTR_ERR(dev->gclk);

	dev->dev = &pdev->dev;
	dev->regmap = regmap;
	...
}

Previously, clk_prepare_enable(dev->pclk) was called after dev->regmap
and dev->dev were initialized.  With devm_clk_get_enabled(), the
peripheral clock is enabled while dev->regmap is still NULL, so a pending
interrupt (for example across a driver rebind, as ATMEL_I2SC_IER is never
masked on unbind) will hit a NULL pointer dereference in
regmap_read(dev->regmap, ATMEL_I2SC_SR, &sr).

In addition, because devm_clk_get_enabled() is registered after
devm_request_irq(), devres teardown on probe failure or driver unbind now
disables dev->pclk before freeing the IRQ, leaving a window where
atmel_i2s_interrupt() can read I2S registers while the peripheral clock
is gated.  Initializing dev->dev and dev->regmap and calling
devm_clk_get_enabled() before devm_request_irq() would avoid both issues.

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