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

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

-- 
Best regards,
Radu