Re: [PATCH v2] irqchip: imx-irqsteer: Convert to devm_pm_runtime_enable()

On Tue, 2026-08-04 at 08:09 -0300, Fabio Estevam wrote:
> On Sat, Aug 1, 2026 at 3:54 PM Radu Rendec <[email protected]> wrote:
> 
> > I take that back. At a closer look, I think both issues identified by
> > Sashiko are valid. While the second issue is marked as "pre-existing",
> > the proposed patch introduces a new path where the issue can occur
> > (leaking the irq domain).
> 
> Would the first version of the patch be acceptable?

It certainly avoids the problem of the extra error path that
devm_pm_runtime_enable() introduces.

But Sashiko had a comment on the first version too, and I think at
least the first issue - about the double-disable of the clock (if the
device is suspended when it's removed) - is real.

In my (very limited) understanding of the runtime_pm framework, there
are two ways to handle this cleanly:
 * Manage the activation and deactivation of the clock indirectly
   through suspend/resume requests. The driver already enables the
   clock in its resume callback and disables the clock in its suspend
   callback. Instead of enabling/disabling the clock explicitly in the
   probe/remove function, use pm_runtime_get_sync() and
   pm_runtime_put(), which indirectly resume and respectively suspend
   the device. This is what drivers/irqchip/irq-renesas-intc-irqpin.c
   does (except it doesn't manage a clock).
 * Call pm_runtime_get_sync() at the beginning of the remove function
   (which requires a matching pm_runtime_put() in the probe function).
   That will indirectly resume the device first, then it's safe to
   disable the clock.

At a high level, the second option above would like this:

static int imx_irqsteer_probe(struct platform_device *pdev)
{
 /* Initial setup / clock enablement */

 pm_runtime_set_active(&pdev->dev);
 pm_runtime_enable(&pdev->dev);
 /* Mark device idle so it can autosuspend when unused */
 pm_runtime_put_autosuspend(&pdev->dev);

 return 0;
}

static void imx_irqsteer_remove(struct platform_device *pdev)
{
    pm_runtime_get_sync(&pdev->dev);
    pm_runtime_disable(&pdev->dev);

    /* Final clean up and disable clock */
}

That's closer to what the driver already does. But the part that I
don't like is that the pm_runtime_put_autosuspend() call may suspend
the device immediately, and I don't fully understand the implications
of that.