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

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

[Fabio Estevam]

From: Fabio Estevam <festevam@nabladev.com>

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 <festevam@nabladev.com>
---
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);
-- 
2.43.0

[PATCH v3 2/2] irqchip/imx-irqsteer: Validate IRQ count before creating domain

[Fabio Estevam]

From: Fabio Estevam <festevam@nabladev.com>

The IRQ count is validated after creating the IRQ domain. If it is
invalid, probe returns without removing the domain, leaving its host
data pointing at devm-managed memory that is freed on probe failure.

Validate the count before allocating resources to avoid the leak and
dangling pointer.

Fixes: 28528fca4908 ("irqchip/imx-irqsteer: Add multi output interrupts support")
Signed-off-by: Fabio Estevam <festevam@nabladev.com>
---
Changes since v2:
- Newly introduced.

 drivers/irqchip/irq-imx-irqsteer.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/irqchip/irq-imx-irqsteer.c b/drivers/irqchip/irq-imx-irqsteer.c
index 653e25115083..55aec60dee40 100644
--- a/drivers/irqchip/irq-imx-irqsteer.c
+++ b/drivers/irqchip/irq-imx-irqsteer.c
@@ -217,6 +217,8 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
 	 */
 	data->irq_count = DIV_ROUND_UP(irqs_num, 64);
 	data->reg_num = irqs_num / 32;
+	if (!data->irq_count || data->irq_count > CHAN_MAX_OUTPUT_INT)
+		return -EINVAL;
 
 	if (IS_ENABLED(CONFIG_PM)) {
 		data->saved_reg = devm_kzalloc(&pdev->dev,
@@ -250,11 +252,6 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
 	}
 	irq_domain_set_pm_device(data->domain, &pdev->dev);
 
-	if (!data->irq_count || data->irq_count > CHAN_MAX_OUTPUT_INT) {
-		ret = -EINVAL;
-		goto out;
-	}
-
 	for (i = 0; i < data->irq_count; i++) {
 		data->irq[i] = irq_of_parse_and_map(np, i);
 		if (!data->irq[i])
-- 
2.43.0

Re: [PATCH v3 2/2] irqchip/imx-irqsteer: Validate IRQ count before creating domain

[Frank Li]

On Wed, Aug 05, 2026 at 04:27:43PM -0300, Fabio Estevam wrote:
> From: Fabio Estevam <festevam@nabladev.com>
>
> The IRQ count is validated after creating the IRQ domain. If it is
> invalid, probe returns without removing the domain, leaving its host
> data pointing at devm-managed memory that is freed on probe failure.
>
> Validate the count before allocating resources to avoid the leak and
> dangling pointer.
>
> Fixes: 28528fca4908 ("irqchip/imx-irqsteer: Add multi output interrupts support")
> Signed-off-by: Fabio Estevam <festevam@nabladev.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> Changes since v2:
> - Newly introduced.
>
>  drivers/irqchip/irq-imx-irqsteer.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/irqchip/irq-imx-irqsteer.c b/drivers/irqchip/irq-imx-irqsteer.c
> index 653e25115083..55aec60dee40 100644
> --- a/drivers/irqchip/irq-imx-irqsteer.c
> +++ b/drivers/irqchip/irq-imx-irqsteer.c
> @@ -217,6 +217,8 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
>  	 */
>  	data->irq_count = DIV_ROUND_UP(irqs_num, 64);
>  	data->reg_num = irqs_num / 32;
> +	if (!data->irq_count || data->irq_count > CHAN_MAX_OUTPUT_INT)
> +		return -EINVAL;
>
>  	if (IS_ENABLED(CONFIG_PM)) {
>  		data->saved_reg = devm_kzalloc(&pdev->dev,
> @@ -250,11 +252,6 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
>  	}
>  	irq_domain_set_pm_device(data->domain, &pdev->dev);
>
> -	if (!data->irq_count || data->irq_count > CHAN_MAX_OUTPUT_INT) {
> -		ret = -EINVAL;
> -		goto out;
> -	}
> -
>  	for (i = 0; i < data->irq_count; i++) {
>  		data->irq[i] = irq_of_parse_and_map(np, i);
>  		if (!data->irq[i])
> --
> 2.43.0
>
>

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

[Frank Li]

On Wed, Aug 05, 2026 at 04:27:42PM -0300, Fabio Estevam wrote:
> From: Fabio Estevam <festevam@nabladev.com>
>
> 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 <festevam@nabladev.com>
> ---
> Changes since v1:
> - Move devm_pm_runtime_enable() prior to irq_domain_create_linear(). (Frank)

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>
>  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);
> --
> 2.43.0
>
>

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

[Radu Rendec]

On Wed, 2026-08-05 at 16:27 -0300, Fabio Estevam wrote:
> From: Fabio Estevam <festevam@nabladev.com>
> 
> 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 <festevam@nabladev.com>
> ---
> 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 <radu@rendec.net>");
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);

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

[Frank Li]

On Thu, Aug 06, 2026 at 05:28:59PM -0400, Radu Rendec wrote:
> On Wed, 2026-08-05 at 16:27 -0300, Fabio Estevam wrote:
> > From: Fabio Estevam <festevam@nabladev.com>
> >
> > 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 <festevam@nabladev.com>
> > ---
> > 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

https://elixir.bootlin.com/linux/v7.2.2/source/drivers/base/dd.c#L827

after probe, pm_request_idle(dev), which trigger pm_dummy_runtime_suspend()

> [  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

You should set runtime_resume() at remove function to match probe's state.
if your driver remove() did disable clock.

It is trick between runtime pm and devm clock management.

The beneafit of keep runtime active in probe, driver can work when disable
CONFIG_PM, but complex at tear down.

keep inactive in probe, code will be simple, but it will not work if
disable CONFIG_PM because clock have not enabled

Frank

> [  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 <radu@rendec.net>");
> 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);

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

[Radu Rendec]

On Fri, 2026-09-11 at 15:32 -0400, Frank Li wrote:
> On Thu, Aug 06, 2026 at 05:28:59PM -0400, Radu Rendec wrote:
> > On Wed, 2026-08-05 at 16:27 -0300, Fabio Estevam wrote:
> > > From: Fabio Estevam <festevam@nabladev.com>
> > > 
> > > 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 <festevam@nabladev.com>
> > > ---
> > > 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
> 
> https://elixir.bootlin.com/linux/v7.2.2/source/drivers/base/dd.c#L827
> 
> after probe, pm_request_idle(dev), which trigger pm_dummy_runtime_suspend()
> 
> > [  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
> 
> You should set runtime_resume() at remove function to match probe's state.
> if your driver remove() did disable clock.
> 
> It is trick between runtime pm and devm clock management.
> 
> The beneafit of keep runtime active in probe, driver can work when disable
> CONFIG_PM, but complex at tear down.
> 
> keep inactive in probe, code will be simple, but it will not work if
> disable CONFIG_PM because clock have not enabled

Thanks, Frank! I appreciate you took the time to explain this in
detail.

The dummy driver followed the exact same logic/sequence as the proposed
patch, and the purpose was to observe the behavior of runtime_pm in
isolation and prove that something wasn't quite right about the clock
management.

I believe this is now handled correctly in Zhipeng's consolidated patch
series against imx-irqsteer.

> > [  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 <radu@rendec.net>");
> > 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);

-- 
Best regards,
Radu