pmdomain: core: Fix detach procedure for virtual devices in genpd

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

Posted by Ulf Hansson 1 month, 4 weeks ago

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@linux-m68k.org>
Fixes: 3c095f32a92b ("PM / Domains: Add support for multi PM domains per device to genpd")
Cc: stable@vger.kernel.org
Closes: https://lore.kernel.org/all/CAMuHMdWapT40hV3c+CSBqFOW05aWcV1a6v_NiJYgoYi0i9_PDQ@mail.gmail.com/
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/pmdomain/core.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
index 4d32fc676aaf..71e930e80178 100644
--- 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)
 	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

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

Posted by Geert Uytterhoeven 1 month, 4 weeks ago

Hi Ulf,

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

> --- 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@glider.be/

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

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

Posted by Ulf Hansson 1 month, 2 weeks ago

On Fri, 17 Apr 2026 at 20:36, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Ulf,
>
> On Fri, 17 Apr 2026 at 13:13, Ulf Hansson <ulf.hansson@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@linux-m68k.org>
> > Fixes: 3c095f32a92b ("PM / Domains: Add support for multi PM domains per device to genpd")
> > Cc: stable@vger.kernel.org
> > Closes: https://lore.kernel.org/all/CAMuHMdWapT40hV3c+CSBqFOW05aWcV1a6v_NiJYgoYi0i9_PDQ@mail.gmail.com/
> > Signed-off-by: Ulf Hansson <ulf.hansson@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@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