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@xxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx>");
> 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);