[PATCH] pmdomain: core: Fix detach procedure for virtual devices in genpd

[Ulf Hansson]

On Fri, 17 Apr 2026 at 20:36, Geert Uytterhoeven <geert at linux-m68k.org> wrote:
>
> Hi Ulf,
>
> On Fri, 17 Apr 2026 at 13:13, Ulf Hansson <ulf.hansson at linaro.org> wrote:
> > If a device is attached to a PM domain through genpd_dev_pm_attach_by_id(),
> > genpd calls pm_runtime_enable() for the corresponding virtual device that
> > it registers. While this avoids boilerplate code in drivers, there is no
> > corresponding call to pm_runtime_disable() in genpd_dev_pm_detach().
> >
> > This means these virtual devices are typically detached from its genpd,
> > while runtime PM remains enabled for them, which is not how things are
> > designed to work. In worst cases it may lead to critical errors, like a
> > NULL pointer dereference bug in genpd_runtime_suspend(), which was recently
> > reported. For another case, we may end up keeping an unnecessary vote for a
> > performance state for the device.
> >
> > To fix these problems, let's add this missing call to pm_runtime_disable()
> > in genpd_dev_pm_detach().
> >
> > Reported-by: Geert Uytterhoeven <geert at linux-m68k.org>
> > Fixes: 3c095f32a92b ("PM / Domains: Add support for multi PM domains per device to genpd")
> > Cc: stable at vger.kernel.org
> > Closes: https://lore.kernel.org/all/CAMuHMdWapT40hV3c+CSBqFOW05aWcV1a6v_NiJYgoYi0i9_PDQ@mail.gmail.com/
> > Signed-off-by: Ulf Hansson <ulf.hansson at linaro.org>
>
> Thanks for your patch!
>
> This survived more than 160000 bind/unbind attempts[1] on R-Car M3-W
> and M3-N, so
> Tested-by: Geert Uytterhoeven <geert+renesas at glider.be>

Thanks for testing! I have queued the patch for fixes.

>
> > --- a/drivers/pmdomain/core.c
> > +++ b/drivers/pmdomain/core.c
> > @@ -3089,6 +3089,7 @@ static const struct bus_type genpd_bus_type = {
> >  static void genpd_dev_pm_detach(struct device *dev, bool power_off)
> >  {
> >         struct generic_pm_domain *pd;
> > +       bool is_virt_dev;
> >         unsigned int i;
> >         int ret = 0;
> >
> > @@ -3098,6 +3099,13 @@ static void genpd_dev_pm_detach(struct device *dev, bool power_off)
> >
> >         dev_dbg(dev, "removing from PM domain %s\n", pd->name);
> >
> > +       /* Check if the device was created by genpd at attach. */
> > +       is_virt_dev = dev->bus == &genpd_bus_type;
> > +
> > +       /* Disable runtime PM if we enabled it at attach. */
> > +       if (is_virt_dev)
> > +               pm_runtime_disable(dev);
> > +
> >         /* Drop the default performance state */
> >         if (dev_gpd_data(dev)->default_pstate) {
> >                 dev_pm_genpd_set_performance_state(dev, 0);
> > @@ -3123,7 +3131,7 @@ static void genpd_dev_pm_detach(struct device *dev, bool power_off)
>
> Above, out of context, there is an error return.
> Should we call pm_runtime_enable() again, to keep the reference count
> balanced? Or can we just ignore this? It's probably futile anyway.

Good point. I considered it, but I think it's safer to keep runtime PM
disabled, if we encounter an error.

If we end up converting genpd_dev_pm_detach() to return an int instead
of void, then we could revisit this.

>
> >         genpd_queue_power_off_work(pd);
> >
> >         /* Unregister the device if it was created by genpd. */
> > -       if (dev->bus == &genpd_bus_type)
> > +       if (is_virt_dev)
> >                 device_unregister(dev);
> >  }
> >

Kind regards
Uffe