Re: [PATCH v6 6/9] irqchip/imx-irqsteer: Let devres own the clock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] pm: clock underflow warning on devres cleanup if pm_runtime_resume_and_get() fails
--

commit 7ea8370844753a72ff02a8217ace864b04cf2175
Author: Zhipeng Wang <zhipeng.wang_1@nxp.com>

irqchip/imx-irqsteer: Let devres own the clock

This commit transitions the imx-irqsteer driver to use devm_clk_get_enabled()
for clock lifecycle management. It also limits runtime PM callbacks to
clk_enable() and clk_disable() since preparation is handled by devres.

> diff --git a/drivers/irqchip/irq-imx-irqsteer.c b/drivers/irqchip/irq-imx-irqsteer.c
> index fa233de9bc2b2..15ee50f65d54c 100644
> --- a/drivers/irqchip/irq-imx-irqsteer.c
> +++ b/drivers/irqchip/irq-imx-irqsteer.c
[ ... ]
> @@ -273,15 +266,23 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
>  	platform_set_drvdata(pdev, data);
>  
>  	return 0;
> -out:
> -	clk_disable_unprepare(data->ipg_clk);
> -	return ret;
>  }
>  
>  static void imx_irqsteer_remove(struct platform_device *pdev)
>  {
>  	struct irqsteer_data *irqsteer_data = platform_get_drvdata(pdev);
> -	int i;
> +	int i, ret;
> +
> +	/*
> +	 * The device may be runtime-suspended here, in which case the runtime
> +	 * suspend callback has already dropped the clock enable count. Resume
> +	 * it so the devres clk_disable_unprepare(), which runs after remove(),
> +	 * finds the clock enabled and stays balanced. On success drop the
> +	 * usage count again with pm_runtime_put_noidle(): it must not trigger a
> +	 * suspend (which would re-disable the clock) and must not leak to the
> +	 * next probe of this persistent device.
> +	 */
> +	ret = pm_runtime_resume_and_get(&pdev->dev);
>  
>  	for (i = 0; i < irqsteer_data->irq_count; i++) {
>  		if (!irqsteer_data->irq[i])
> @@ -294,8 +295,8 @@ static void imx_irqsteer_remove(struct platform_device *pdev)
>  
>  	irq_domain_remove(irqsteer_data->domain);
>  
> -	if (!pm_runtime_status_suspended(&pdev->dev))
> -		clk_disable_unprepare(irqsteer_data->ipg_clk);
> +	if (ret >= 0)
> +		pm_runtime_put_noidle(&pdev->dev);

[Severity: Medium]
If pm_runtime_resume_and_get() fails, the device remains suspended and its
clock remains disabled.

Since devres owns the clock via devm_clk_get_enabled(), won't devres
unconditionally call clk_disable_unprepare() after imx_irqsteer_remove()
returns?

Could this trigger a WARN(core->enable_count == 0) in the clock framework and
unbalance the clock state?

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008090231.796511-1-Zhipeng.wang_1@oss.nxp.com?part=6