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

[Geert Uytterhoeven]

Hi Ulf,

On Fri, 17 Apr 2026 at 13:13, Ulf Hansson <ulf.hansson@xxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> Fixes: 3c095f32a92b ("PM / Domains: Add support for multi PM domains per device to genpd")
> Cc: stable@xxxxxxxxxxxxxxx
> Closes: https://lore.kernel.org/all/CAMuHMdWapT40hV3c+CSBqFOW05aWcV1a6v_NiJYgoYi0i9_PDQ@xxxxxxxxxxxxxx/
> Signed-off-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx>

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

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

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

[1] https://lore.kernel.org/15510cee649959281d9554965cacd0c06531c1f3.1773308898.git.geert+renesas@xxxxxxxxx/

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds