Re: [PATCH 5/5] ASoC: fsl_asrc: Use devm for runtime PM cleanup

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] ASoC: fsl_asrc: premature suspend action registration in
  fsl_asrc_probe()
--

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

ASoC: fsl_asrc: Use devm for runtime PM cleanup

This commit replaces manual runtime PM cleanup in fsl_asrc_probe() and
fsl_asrc_remove() with devm_add_action() and devm_pm_runtime_enable().

> diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
> index c97f814d7358b..09e09164a1061 100644
> --- a/sound/soc/fsl/fsl_asrc.c
> +++ b/sound/soc/fsl/fsl_asrc.c
> @@ -1258,6 +1258,15 @@ static int fsl_asrc_m2m_pair_resume(struct fsl_asrc_pair *pair)
>  static int fsl_asrc_runtime_resume(struct device *dev);
>  static int fsl_asrc_runtime_suspend(struct device *dev);
>  
> +/* devm action: runtime suspend the device if it is not runtime suspended yet */
> +static void fsl_asrc_runtime_suspend_action(void *data)
> +{
> +	struct device *dev = data;
> +
> +	if (!pm_runtime_status_suspended(dev))
> +		fsl_asrc_runtime_suspend(dev);
> +}
> +
>  static int fsl_asrc_probe(struct platform_device *pdev)
>  {
>  	struct device_node *np = pdev->dev.of_node;
> @@ -1413,22 +1422,34 @@ static int fsl_asrc_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, asrc);
>  	spin_lock_init(&asrc->lock);
> -	pm_runtime_enable(&pdev->dev);
> +
> +	/*
> +	 * Ensure the device is suspended after the IRQ and component
> +	 * have been released.
> +	 */
> +	ret = devm_add_action(&pdev->dev, fsl_asrc_runtime_suspend_action,
> +			      &pdev->dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_pm_runtime_enable(&pdev->dev);
> +	if (ret)
> +		return ret;
>  	if (!pm_runtime_enabled(&pdev->dev)) {
>  		ret = fsl_asrc_runtime_resume(&pdev->dev);
>  		if (ret)
> -			goto err_pm_disable;
> +			return ret;
>  	}

[Severity: Medium]
If CONFIG_PM is disabled and fsl_asrc_runtime_resume() fails here, does
devres unwind end up calling fsl_asrc_runtime_suspend() while the clocks are
still disabled?

When CONFIG_PM is disabled, pm_runtime_enabled() and
pm_runtime_status_suspended() both return false.  If
fsl_asrc_runtime_resume() fails, it cleans up any clocks it enabled and
returns an error, which now triggers fsl_asrc_runtime_suspend_action().
fsl_asrc_runtime_suspend() then reads the volatile REG_ASRCFG register over
MMIO with all ASRC clocks gated (which can cause an external abort or bus
hang on i.MX) and calls clk_disable_unprepare() on clocks that were never
enabled.

Registering fsl_asrc_runtime_suspend_action() after the !pm_runtime_enabled()
block (or using devm_add_action_or_reset() after devm_pm_runtime_enable()
with the !pm_runtime_enabled() resume handled first) would avoid suspending
an unresumed device.

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