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

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

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Hi,

Series drops the dev_pm_domain_detach() from platform bus remove and
adds it in device_unbind_cleanup() to avoid runtime resumming the device
after it was detached from its PM domain.

Please provide your feedback.

Thank you,
Claudiu

Changes in v4:
- added a flag in dev_pm_info that is saved in dev_pm_domain_attach()
  and used in device_unbind_cleanup()

Changes in v3:
- add devm_pm_domain_attach()

Changes in v2:
- dropped the devres group open/close approach and use
  devm_pm_domain_attach()
- adjusted patch description to reflect the new approach

Claudiu Beznea (2):
  PM: domains: Add domain detach_power_off state
  driver core: platform: Drop dev_pm_domain_detach()

 drivers/base/dd.c           | 2 ++
 drivers/base/platform.c     | 6 +-----
 drivers/base/power/common.c | 3 +++
 include/linux/pm.h          | 1 +
 4 files changed, 7 insertions(+), 5 deletions(-)

-- 
2.43.0

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

[Claudiu]

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;
+
 	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;
 };
 
 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 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.

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

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()

[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

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

[Ulf Hansson]

On Mon, 16 Jun 2025 at 19:14, Rafael J. Wysocki <rafael@kernel.org> 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.

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

[PATCH v4 2/2] driver core: platform: Drop dev_pm_domain_detach() call

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

On the Renesas RZ/G3S (and other Renesas SoCs, e.g., RZ/G2{L, LC, UL}),
clocks are managed through PM domains. These PM domains, registered on
behalf of the clock controller driver, are configured with
GENPD_FLAG_PM_CLK. In most of the Renesas drivers used by RZ SoCs, the
clocks are enabled/disabled using runtime PM APIs. The power domains may
also have power_on/power_off support implemented. After the device PM
domain is powered off any CPU accesses to these domains leads to system
aborts.

During probe, devices are attached to the PM domain controlling their
clocks and power. Similarly, during removal, devices are detached from the
PM domain.

The detachment call stack is as follows:

device_driver_detach() ->
  device_release_driver_internal() ->
    __device_release_driver() ->
      device_remove() ->
        platform_remove() ->
	  dev_pm_domain_detach()

During driver unbind, after the device is detached from its PM domain,
the device_unbind_cleanup() function is called, which subsequently invokes
devres_release_all(). This function handles devres resource cleanup.

If runtime PM is enabled in driver probe via devm_pm_runtime_enable(), the
cleanup process triggers the action or reset function for disabling runtime
PM. This function is pm_runtime_disable_action(), which leads to the
following call stack of interest when called:

pm_runtime_disable_action() ->
  pm_runtime_dont_use_autosuspend() ->
    __pm_runtime_use_autosuspend() ->
      update_autosuspend() ->
        rpm_idle()

The rpm_idle() function attempts to resume the device at runtime. However,
at the point it is called, the device is no longer part of a PM domain
(which manages clocks and power states). If the driver implements its own
runtime PM APIs for specific functionalities - such as the rzg2l_adc
driver - while also relying on the power domain subsystem for power
management, rpm_idle() will invoke the driver's runtime PM API. However,
since the device is no longer part of a PM domain at this point, the PM
domain's runtime PM APIs will not be called. This leads to system aborts on
Renesas SoCs.

Another identified case is when a subsystem performs various cleanups
using device_unbind_cleanup(), calling driver-specific APIs in the process.
A known example is the thermal subsystem, which may call driver-specific
APIs to disable the thermal device. The relevant call stack in this case
is:

device_driver_detach() ->
  device_release_driver_internal() ->
    device_unbind_cleanup() ->
      devres_release_all() ->
        devm_thermal_of_zone_release() ->
	  thermal_zone_device_disable() ->
	    thermal_zone_device_set_mode() ->
	      struct thermal_zone_device_ops::change_mode()

At the moment the driver-specific change_mode() API is called, the device
is no longer part of its PM domain. Accessing its registers without proper
power management leads to system aborts.

Drop the call to dev_pm_domain_detach() from the platform bus remove
function and rely on the newly introduced call in device_unbind_cleanup().
This ensures the same effect, but the call now occurs after all
driver-specific devres resources have been freed.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v4:
- dropped devm_pm_domain_attach() approach
- adjusted patch description to reflect this

Changes in v3:
- adjusted the call to devm_pm_domain_attach() as it now gets
  2 parameters

Changes in v2:
- dropped the devres group open/close approach and use
  devm_pm_domain_attach()
- adjusted patch description to reflect the new approach

 drivers/base/platform.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 075ec1d1b73a..2459be6aecf4 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -1400,11 +1400,8 @@ static int platform_probe(struct device *_dev)
 	if (ret)
 		goto out;
 
-	if (drv->probe) {
+	if (drv->probe)
 		ret = drv->probe(dev);
-		if (ret)
-			dev_pm_domain_detach(_dev, true);
-	}
 
 out:
 	if (drv->prevent_deferred_probe && ret == -EPROBE_DEFER) {
@@ -1422,7 +1419,6 @@ static void platform_remove(struct device *_dev)
 
 	if (drv->remove)
 		drv->remove(dev);
-	dev_pm_domain_detach(_dev, true);
 }
 
 static void platform_shutdown(struct device *_dev)
-- 
2.43.0