Re: [PATCH v5 6/9] irqchip/imx-irqsteer: Let devres own the clock and runtime PM

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing pm_runtime_put_noidle() in remove() causes a usage counter leak, permanently breaking autosuspend upon driver rebind.
- [Medium] Ignoring the return value of pm_runtime_resume_and_get() in remove() can lead to a clock enable count underflow during devres cleanup.
--

--- Patch [6]: [PATCH v5 6/9] irqchip/imx-irqsteer: Let devres own the clock and runtime PM ---
commit 57b65e0ae0ccb7952253c0bbf59f0e1f037ea934
Author: Zhipeng Wang <[email protected]>

irqchip/imx-irqsteer: Let devres own the clock and runtime PM

In preparation for making the driver unbindable/reloadable, let the
driver core own the clock and runtime PM lifetime so that the probe()
error path and remove() do not have to hand-balance them.

> diff --git a/drivers/irqchip/irq-imx-irqsteer.c b/drivers/irqchip/irq-imx-irqsteer.c
> index b63bf957ab884..c2f58787f9a86 100644
> --- a/drivers/irqchip/irq-imx-irqsteer.c
> +++ b/drivers/irqchip/irq-imx-irqsteer.c

[ ... ]

> @@ -289,6 +292,14 @@ static void imx_irqsteer_remove(struct platform_device *pdev)
>  	struct irqsteer_data *irqsteer_data = platform_get_drvdata(pdev);
>  	int i;
>  
> +	/*
> +	 * 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.
> +	 */
> +	pm_runtime_resume_and_get(&pdev->dev);

[Severity: Medium]
Is it safe to ignore the return value of pm_runtime_resume_and_get() here?

If it fails (for example, if clk_enable() fails in imx_irqsteer_resume()), 
the device will remain suspended and the clock disabled. When devres 
subsequently runs clk_disable_unprepare() during cleanup, wouldn't this 
cause a clock enable count underflow?

[Severity: High]
Does this leak the runtime PM usage counter on driver unbind?

pm_runtime_resume_and_get() increments dev->power.usage_count on success, 
but there doesn't appear to be a matching pm_runtime_put_noidle() or similar 
put call before imx_irqsteer_remove() returns.

Since the devres action registered by devm_pm_runtime_set_active_enabled() 
only calls pm_runtime_disable() (which does not drop the usage count), 
wouldn't this permanently break autosuspend when the driver is rebound?

> +
>  	for (i = 0; i < irqsteer_data->irq_count; i++) {
>  		if (!irqsteer_data->irq[i])
>  			break;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6