Re: [PATCH v3 1/2] irqchip/imx-irqsteer: Convert to devm_pm_runtime_enable()

On Wed, 2026-08-05 at 16:27 -0300, Fabio Estevam wrote:
> From: Fabio Estevam <[email protected]>
> 
> imx_irqsteer_probe() enables runtime PM, but imx_irqsteer_remove() does
> not disable it. Consequently, runtime PM remains enabled after unbinding
> the device, and rebinding it triggers:
> 
> Unbalanced pm_runtime_enable!
> 
> Use devm_pm_runtime_enable() to automatically disable runtime PM when
> the device is removed. Set up runtime PM before creating the IRQ domain
> and registering chained handlers so that a failure cannot leave either
> resource pointing at freed driver data.
> 
> Fixes: 4730d2233311 ("irqchip/imx-irqsteer: Add runtime PM support")
> Signed-off-by: Fabio Estevam <[email protected]>
> ---
> Changes since v1:
> - Move devm_pm_runtime_enable() prior to irq_domain_create_linear(). (Frank)
> 
>  drivers/irqchip/irq-imx-irqsteer.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-imx-irqsteer.c b/drivers/irqchip/irq-imx-irqsteer.c
> index 87b07f517be3..653e25115083 100644
> --- a/drivers/irqchip/irq-imx-irqsteer.c
> +++ b/drivers/irqchip/irq-imx-irqsteer.c
> @@ -236,6 +236,11 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
>  	if (irqsteer_has_chanctrl(data->devtype_data))
>  		writel_relaxed(BIT(data->channel), data->regs + CHANCTRL);
>  
> +	pm_runtime_set_active(&pdev->dev);
> +	ret = devm_pm_runtime_enable(&pdev->dev);
> +	if (ret)
> +		goto out;
> +
>  	data->domain = irq_domain_create_linear(dev_fwnode(&pdev->dev), data->reg_num * 32,
>  						&imx_irqsteer_domain_ops, data);
>  	if (!data->domain) {
> @@ -262,9 +267,6 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, data);
>  
> -	pm_runtime_set_active(&pdev->dev);
> -	pm_runtime_enable(&pdev->dev);
> -
>  	return 0;
>  out:
>  	clk_disable_unprepare(data->ipg_clk);

I still believe there is something off with the way pm_runtime is
handled, and that the double clock disable is possible.

Since (like I said) I have very limited understanding of the runtime_pm
framework, I decided to make a little experiment.

With the dummy module below, I see this:
[  548.253591] pm_dummy pm_dummy: pm_dummy_probe() executed
[  548.254209] pm_dummy pm_dummy: clock enabled; refcount: 1
[  548.255028] pm_dummy pm_dummy: pm_dummy_runtime_suspend() triggered
[  548.255845] pm_dummy pm_dummy: clock disabled; refcount: 0
[  554.261820] pm_dummy pm_dummy: pm_dummy_runtime_resume() triggered
[  554.263100] pm_dummy pm_dummy: clock enabled; refcount: 1
[  554.264006] pm_dummy pm_dummy: pm_dummy_runtime_suspend() triggered
[  554.264997] pm_dummy pm_dummy: clock disabled; refcount: 0
[  554.265819] pm_dummy pm_dummy: pm_dummy_remove() executed
[  554.266609] pm_dummy pm_dummy: clock disabled; refcount: -1
[  554.267468] pm_dummy pm_dummy: **************************************************
[  554.268554] pm_dummy pm_dummy: [BUG DETECTED] Clock disable count underflow! (-1)
[  554.269479] pm_dummy pm_dummy: **************************************************

What I find interesting is that the device is suspended immediately
during probe(), then it's automatically resumed and immediately
suspended again right before remove(). The latter is probably a side
effect of devm_pm_runtime_enable(). But in any case, the clock *is*
disabled twice, and that's even without any explicit suspend or resume,
it's just by loading and unloading the module.

#include <linux/module.h>
#include <linux/kernel.h>
#include <linux/init.h>
#include <linux/platform_device.h>
#include <linux/pm_runtime.h>

MODULE_LICENSE("GPL");
MODULE_AUTHOR("Radu Rendec <[email protected]>");
MODULE_DESCRIPTION("runtime_pm playground");

static int mock_clk_count = 0;

static int mock_clk_prepare_enable(struct device *dev)
{
	mock_clk_count++;
	dev_info(dev, "clock enabled; refcount: %d\n", mock_clk_count);
	return 0;
}

static void mock_clk_disable_unprepare(struct device *dev)
{
	mock_clk_count--;
	dev_info(dev, "clock disabled; refcount: %d\n", mock_clk_count);

	if (mock_clk_count < 0) {
		dev_err(dev, "**************************************************\n");
		dev_err(dev, "[BUG DETECTED] Clock disable count underflow! (%d)\n", mock_clk_count);
		dev_err(dev, "**************************************************\n");
	}
}

static int pm_dummy_runtime_suspend(struct device *dev)
{
	dev_info(dev, "%s() triggered\n", __func__);
	mock_clk_disable_unprepare(dev);
	return 0;
}

static int pm_dummy_runtime_resume(struct device *dev)
{
	dev_info(dev, "%s() triggered\n", __func__);
	return mock_clk_prepare_enable(dev);
}

static const struct dev_pm_ops pm_dummy_pm_ops = {
	SET_RUNTIME_PM_OPS(pm_dummy_runtime_suspend, pm_dummy_runtime_resume, NULL)
};

static int pm_dummy_probe(struct platform_device *pdev)
{
	dev_info(&pdev->dev, "%s() executed\n", __func__);

	mock_clk_prepare_enable(&pdev->dev);
	pm_runtime_set_active(&pdev->dev);
	devm_pm_runtime_enable(&pdev->dev);

	return 0;
}

static void pm_dummy_remove(struct platform_device *pdev)
{
	dev_info(&pdev->dev, "%s() executed\n", __func__);

	mock_clk_disable_unprepare(&pdev->dev);
}

static struct platform_driver pm_dummy = {
	.probe = pm_dummy_probe,
	.remove = pm_dummy_remove,
	.driver = {
		.name = "pm_dummy",
		.pm = &pm_dummy_pm_ops,
	},
};

static struct platform_device *pdev;

static int __init pm_demo_init(void)
{
	int ret;

	ret = platform_driver_register(&pm_dummy);
	if (ret)
		return ret;

	pdev = platform_device_register_simple("pm_dummy", -1, NULL, 0);
	if (IS_ERR(pdev)) {
		platform_driver_unregister(&pm_dummy);
		return PTR_ERR(pdev);
	}

	return 0;
}

static void __exit pm_demo_exit(void)
{
	platform_device_unregister(pdev);
	platform_driver_unregister(&pm_dummy);
}

module_init(pm_demo_init);
module_exit(pm_demo_exit);