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

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

[Claudiu Beznea]

Hi, Rafael,

On 16.06.2025 20:14, Rafael J. Wysocki wrote:
> On Mon, Jun 16, 2025 at 3:54 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
>>
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.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@bp.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.

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 ?

Thank you,
Claudiu


> 
>> +
>>         return ret < 0 ? ret : 0;
>>  }
>>  EXPORT_SYMBOL_GPL(dev_pm_domain_attach);
>> diff --git a/include/linux/pm.h b/include/linux/pm.h
>> index f0bd8fbae4f2..dcbe2c1ef59b 100644
>> --- a/include/linux/pm.h
>> +++ b/include/linux/pm.h
>> @@ -720,6 +720,7 @@ struct dev_pm_info {
>>         struct pm_subsys_data   *subsys_data;  /* Owned by the subsystem. */
>>         void (*set_latency_tolerance)(struct device *, s32);
>>         struct dev_pm_qos       *qos;
>> +       bool                    detach_power_off:1;
> 
> Please put the new flag under #ifdef CONFIG_PM after memalloc_noio and
> comment it as "Owned by the driver core".

OK!

Thank you for your review,
Claudiu

> 
> Otherwise LGTM.
> 
>>  };
>>
>>  extern int dev_pm_get_subsys_data(struct device *dev);
>> --
>> 2.43.0
>>

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@tuxon.dev> 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@tuxon.dev> wrote:
> >>
> >> From: Claudiu Beznea <claudiu.beznea.uj@bp.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@bp.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.
>
> 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.

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

[Ulf Hansson]

On Tue, 17 Jun 2025 at 20:54, Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Tue, Jun 17, 2025 at 4:41 PM Claudiu Beznea <claudiu.beznea@tuxon.dev> 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@tuxon.dev> wrote:
> > >>
> > >> From: Claudiu Beznea <claudiu.beznea.uj@bp.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@bp.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.
> >
> > 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.

I am not sure it's needed, unless it's especially targeted for the
ACPI PM domain, which I find hard to believe.

Also, I find it awkward why the i2c bus should be any different from
many other types of buses. It's probably just because of legacy and
that someone took a decision when we added it.

Wolfram, what's your thinking around this?

Kind regards
Uffe

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

[Dmitry Torokhov]

On Thu, Jun 19, 2025 at 02:16:37PM +0200, Ulf Hansson wrote:
> 
> I am not sure it's needed, unless it's especially targeted for the
> ACPI PM domain, which I find hard to believe.
> 
> Also, I find it awkward why the i2c bus should be any different from
> many other types of buses. It's probably just because of legacy and
> that someone took a decision when we added it.

It has nothing to do with I2C and everything to do with ACPI and the
fact that it brings devices into D0 when probing. On ACPI systems
(unlike DT ones) power sequencing is done in firmware so drivers are
unable to control this. And this causes annoying flashing of privacy
leds on webcams.

See details in 1e96078e0ae4 ("at24: Support probing while in non-zero ACPI D state")

If there was a SPI device sharing power rails with a camera we'd need
similar hack in SPI bus.

Thanks.

-- 
Dmitry