Re: [PATCH v4 1/2] PM: domains: Detach on device_unbind_cleanup()

[Rafael J. Wysocki]

On Tue, Jun 17, 2025 at 4:41 PM Claudiu Beznea <claudiu.beznea@xxxxxxxxx> wrote:
>
> Hi, Rafael,
>
> On 16.06.2025 20:14, Rafael J. Wysocki wrote:
> > On Mon, Jun 16, 2025 at 3:54 PM Claudiu <claudiu.beznea@xxxxxxxxx> wrote:
> >>
> >> From: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
> >>
> >> 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@xxxxxxxxxxxxxx>
> >> ---
> >>
> >> 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.
>
> In v2 it has been discussed to just mirror the power_on acquisition.
>
> Double checking now, all the current users of dev_pm_domain_attach() follow
> this rule, except the i2c bus. i2c powers on the domain conditionally:
>
> https://elixir.bootlin.com/linux/v6.15.2/source/drivers/i2c/i2c-core-base.c#L575
>
> and powers it off unconditionally:
> https://elixir.bootlin.com/linux/v6.15.2/source/drivers/i2c/i2c-core-base.c#L638
>
> Should we take this into account ?

I think so.

It is still sufficient to use one device flag to represent the
information whether or not to remove power on detach, but I would
change the second argument of dev_pm_domain_attach() to a u8
representing a mask of bits:

PM_DOMAIN_POWER_ON    BIT(0)
PM_DOMAIN_POWER_OFF    BIT(1)

where PM_DOMAIN_POWER_ON will be set to indicate that the device
should be turned on right after attaching the PM domain and the value
of PM_DOMAIN_POWER_OFF will be stored in the new device flag.

The majority of users will set or clear both, but i2c will set
PM_DOMAIN_POWER_OFF and either set of clear PM_DOMAIN_POWER_ON
depending on the do_power_on value.