linux-kernel - Re: [PATCH v4 1/2] PM: domains: Detach on device_unbind

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAPDyKFr_oqvtRsCeak62wxw=fqadY6nuwnr0vKxsHqybTA8gzw@mail.gmail.com>
Date: Thu, 19 Jun 2025 14:11:35 +0200
From: Ulf Hansson <ulf.hansson@...aro.org>
To: "Rafael J. Wysocki" <rafael@...nel.org>
Cc: Claudiu <claudiu.beznea@...on.dev>, gregkh@...uxfoundation.org, dakr@...nel.org, 
	len.brown@...el.com, pavel@...nel.org, jic23@...nel.org, 
	daniel.lezcano@...aro.org, dmitry.torokhov@...il.com, 
	linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org, bhelgaas@...gle.com, 
	geert@...ux-m68k.org, linux-iio@...r.kernel.org, 
	linux-renesas-soc@...r.kernel.org, fabrizio.castro.jz@...esas.com, 
	Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
Subject: Re: [PATCH v4 1/2] PM: domains: Detach on device_unbind_cleanup()

On Mon, 16 Jun 2025 at 19:14, Rafael J. Wysocki <rafael@...nel.org> wrote:
>
> On Mon, Jun 16, 2025 at 3:54 PM Claudiu <claudiu.beznea@...on.dev> wrote:
> >
> > From: Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
> >
> > The dev_pm_domain_attach() function is typically used in bus code alongside
> > dev_pm_domain_detach(), often following patterns like:
> >
> > static int bus_probe(struct device *_dev)
> > {
> >     struct bus_driver *drv = to_bus_driver(dev->driver);
> >     struct bus_device *dev = to_bus_device(_dev);
> >     int ret;
> >
> >     // ...
> >
> >     ret = dev_pm_domain_attach(_dev, true);
> >     if (ret)
> >         return ret;
> >
> >     if (drv->probe)
> >         ret = drv->probe(dev);
> >
> >     // ...
> > }
> >
> > static void bus_remove(struct device *_dev)
> > {
> >     struct bus_driver *drv = to_bus_driver(dev->driver);
> >     struct bus_device *dev = to_bus_device(_dev);
> >
> >     if (drv->remove)
> >         drv->remove(dev);
> >     dev_pm_domain_detach(_dev);
> > }
> >
> > When the driver's probe function uses devres-managed resources that depend
> > on the power domain state, those resources are released later during
> > device_unbind_cleanup().
> >
> > Releasing devres-managed resources that depend on the power domain state
> > after detaching the device from its PM domain can cause failures.
> >
> > For example, if the driver uses devm_pm_runtime_enable() in its probe
> > function, and the device's clocks are managed by the PM domain, then
> > during removal the runtime PM is disabled in device_unbind_cleanup() after
> > the clocks have been removed from the PM domain. It may happen that the
> > devm_pm_runtime_enable() action causes the device to be runtime-resumed.
> > If the driver specific runtime PM APIs access registers directly, this
> > will lead to accessing device registers without clocks being enabled.
> > Similar issues may occur with other devres actions that access device
> > registers.
> >
> > Add detach_power_off member to struct dev_pm_info, to be used later in
> > device_unbind_cleanup() as the power_off argument for
> > dev_pm_domain_detach(). This is a preparatory step toward removing
> > dev_pm_domain_detach() calls from bus remove functions. Since the current
> > PM domain detach functions (genpd_dev_pm_detach() and acpi_dev_pm_detach())
> > already set dev->pm_domain = NULL, there should be no issues with bus
> > drivers that still call dev_pm_domain_detach() in their remove functions.
> >
> > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
> > ---
> >
> > Changes in v4:
> > - save dev->power.detach_power_off in dev_pm_domain_attach() and use
> >   it in device_unbind_cleanup() when detaching
> > - adjusted patch description
> >
> > Changes in v3:
> > - dropped devm_pm_domain_detach_off(), devm_pm_domain_detach_on()
> >   and use a single function devm_pm_domain_detach()
> >
> > Changes in v2:
> > - none; this patch is new
> >
> >  drivers/base/dd.c           | 2 ++
> >  drivers/base/power/common.c | 3 +++
> >  include/linux/pm.h          | 1 +
> >  3 files changed, 6 insertions(+)
> >
> > diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> > index b526e0e0f52d..13ab98e033ea 100644
> > --- a/drivers/base/dd.c
> > +++ b/drivers/base/dd.c
> > @@ -25,6 +25,7 @@
> >  #include <linux/kthread.h>
> >  #include <linux/wait.h>
> >  #include <linux/async.h>
> > +#include <linux/pm_domain.h>
> >  #include <linux/pm_runtime.h>
> >  #include <linux/pinctrl/devinfo.h>
> >  #include <linux/slab.h>
> > @@ -552,6 +553,7 @@ static void device_unbind_cleanup(struct device *dev)
> >         dev->dma_range_map = NULL;
> >         device_set_driver(dev, NULL);
> >         dev_set_drvdata(dev, NULL);
> > +       dev_pm_domain_detach(dev, dev->power.detach_power_off);
> >         if (dev->pm_domain && dev->pm_domain->dismiss)
> >                 dev->pm_domain->dismiss(dev);
> >         pm_runtime_reinit(dev);
> > diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c
> > index 781968a128ff..a8f302ed27a5 100644
> > --- a/drivers/base/power/common.c
> > +++ b/drivers/base/power/common.c
> > @@ -111,6 +111,9 @@ int dev_pm_domain_attach(struct device *dev, bool power_on)
> >         if (!ret)
> >                 ret = genpd_dev_pm_attach(dev);
> >
> > +       if (dev->pm_domain)
> > +               dev->power.detach_power_off = power_on;
>
> I'm assuming that you have checked all of the users of
> dev_pm_domain_attach() and verified that the "power off" value is the
> same as the "power on" one for all of them.

Also note that the value only matters for the ACPI PM domain.

Genpd doesn't even take the value into account, which is the most
common usage of this.

[...]

Kind regards
Uffe