[PATCH v2 0/2] PM: domains: add devm_pm_domain_attach()

[PATCH v2 0/2] PM: domains: add devm_pm_domain_attach()

[Claudiu]

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

Hi,

As a result of discussion at [1], series adds the devm_pm_domain_attach()
and uses it in platform bus probe.

Please provide your feedback.

Thank you,
Claudiu

[1] https://lore.kernel.org/all/20250215130849.227812-1-claudiu.beznea.uj@bp.renesas.com

Changes in v2:
- add devm_pm_domain_attach()
- drop the devres grup open/close approach and use the newly added
  devm_pm_domain_attach()

Claudiu Beznea (2):
  PM: domains: Add devres variant for dev_pm_domain_attach()
  driver core: platform: Use devm_pm_domain_attach()

 drivers/base/platform.c     |  8 ++---
 drivers/base/power/common.c | 59 +++++++++++++++++++++++++++++++++++++
 include/linux/pm_domain.h   |  8 +++++
 3 files changed, 69 insertions(+), 6 deletions(-)

-- 
2.43.0

[PATCH v2 1/2] PM: domains: Add devres variant for dev_pm_domain_attach()

[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 devm_pm_domain_attach(). When replacing the dev_pm_domain_attach() and
dev_pm_domain_detach() in bus probe and bus remove, it ensures that the
device is detached from its PM domain in device_unbind_cleanup(), only
after all driver's devres-managed resources have been release.

For flexibility, the implemented devm_pm_domain_attach() has 2 state
arguments, one for the domain state on attach, one for the domain state on
detach.

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

Changes in v2:
- none; this patch is new

 drivers/base/power/common.c | 59 +++++++++++++++++++++++++++++++++++++
 include/linux/pm_domain.h   |  8 +++++
 2 files changed, 67 insertions(+)

diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c
index 781968a128ff..6ef0924efe2e 100644
--- a/drivers/base/power/common.c
+++ b/drivers/base/power/common.c
@@ -115,6 +115,65 @@ int dev_pm_domain_attach(struct device *dev, bool power_on)
 }
 EXPORT_SYMBOL_GPL(dev_pm_domain_attach);
 
+/**
+ * devm_pm_domain_detach_off - devres action for devm_pm_domain_attach() to
+ * detach a device and power it off.
+ * @dev: device to detach.
+ *
+ * This function reverse the actions from devm_pm_domain_attach().
+ * It will be invoked during the remove phase from drivers implicitly.
+ */
+static void devm_pm_domain_detach_off(void *dev)
+{
+	dev_pm_domain_detach(dev, true);
+}
+
+/**
+ * devm_pm_domain_detach_on - devres action for devm_pm_domain_attach() to
+ * detach a device and power it on.
+ * @dev: device to detach.
+ *
+ * This function reverse the actions from devm_pm_domain_attach().
+ * It will be invoked during the remove phase from drivers implicitly.
+ */
+static void devm_pm_domain_detach_on(void *dev)
+{
+	dev_pm_domain_detach(dev, false);
+}
+
+/**
+ * devm_pm_domain_attach - devres-enabled version of dev_pm_domain_attach()
+ * @dev: Device to attach.
+ * @attach_power_on: Use to indicate whether we should power on the device
+ *                   when attaching (true indicates the device is powered on
+ *                   when attaching).
+ * @detach_power_off: Used to indicate whether we should power off the device
+ *                    when detaching (true indicates the device is powered off
+ *                    when detaching).
+ *
+ * NOTE: this will also handle calling dev_pm_domain_detach() for
+ * you during remove phase.
+ *
+ * Returns 0 on successfully attached PM domain, or a negative error code in
+ * case of a failure.
+ */
+int devm_pm_domain_attach(struct device *dev, bool attach_power_on,
+			  bool detach_power_off)
+{
+	int ret;
+
+	ret = dev_pm_domain_attach(dev, attach_power_on);
+	if (ret)
+		return ret;
+
+	if (detach_power_off)
+		return devm_add_action_or_reset(dev, devm_pm_domain_detach_off,
+						dev);
+
+	return devm_add_action_or_reset(dev, devm_pm_domain_detach_on, dev);
+}
+EXPORT_SYMBOL_GPL(devm_pm_domain_attach);
+
 /**
  * dev_pm_domain_attach_by_id - Associate a device with one of its PM domains.
  * @dev: The device used to lookup the PM domain.
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 0b18160901a2..ee798b090d17 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -509,6 +509,8 @@ struct device *dev_pm_domain_attach_by_name(struct device *dev,
 int dev_pm_domain_attach_list(struct device *dev,
 			      const struct dev_pm_domain_attach_data *data,
 			      struct dev_pm_domain_list **list);
+int devm_pm_domain_attach(struct device *dev, bool attach_power_on,
+			  bool detach_power_off);
 int devm_pm_domain_attach_list(struct device *dev,
 			       const struct dev_pm_domain_attach_data *data,
 			       struct dev_pm_domain_list **list);
@@ -539,6 +541,12 @@ static inline int dev_pm_domain_attach_list(struct device *dev,
 	return 0;
 }
 
+static int devm_pm_domain_attach(struct device *dev, bool attach_power_on,
+				 bool detach_power_off)
+{
+	return 0;
+}
+
 static inline int devm_pm_domain_attach_list(struct device *dev,
 					     const struct dev_pm_domain_attach_data *data,
 					     struct dev_pm_domain_list **list)
-- 
2.43.0

Re: [PATCH v2 1/2] PM: domains: Add devres variant for dev_pm_domain_attach()

[kernel test robot]

Hi Claudiu,

kernel test robot noticed the following build warnings:

[auto build test WARNING on rafael-pm/linux-next]
[also build test WARNING on rafael-pm/bleeding-edge driver-core/driver-core-testing driver-core/driver-core-next driver-core/driver-core-linus amd-pstate/linux-next amd-pstate/bleeding-edge linus/master v6.15 next-20250526]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Claudiu/PM-domains-Add-devres-variant-for-dev_pm_domain_attach/20250526-202318
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link:    https://lore.kernel.org/r/20250526122054.65532-2-claudiu.beznea.uj%40bp.renesas.com
patch subject: [PATCH v2 1/2] PM: domains: Add devres variant for dev_pm_domain_attach()
config: arm-randconfig-002-20250527 (https://download.01.org/0day-ci/archive/20250527/202505270434.ft8ekK9H-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 7.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250527/202505270434.ft8ekK9H-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202505270434.ft8ekK9H-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from drivers/amba/bus.c:15:0:
>> include/linux/pm_domain.h:534:12: warning: 'devm_pm_domain_attach' defined but not used [-Wunused-function]
    static int devm_pm_domain_attach(struct device *dev, bool attach_power_on,
               ^~~~~~~~~~~~~~~~~~~~~


vim +/devm_pm_domain_attach +534 include/linux/pm_domain.h

   533	
 > 534	static int devm_pm_domain_attach(struct device *dev, bool attach_power_on,
   535					 bool detach_power_off)
   536	{
   537		return 0;
   538	}
   539	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 1/2] PM: domains: Add devres variant for dev_pm_domain_attach()

[Dmitry Torokhov]

Hi Claudiu,

On Mon, May 26, 2025 at 03:20:53PM +0300, Claudiu 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.

I think you are concentrating too much on runtime PM aspect of this. As
you mentioned in the last sentence the same issue may happen in the
absence of runtime PM if the power domain code will shut down the device
while it is not fully cleaned up.

> 
> Add devm_pm_domain_attach(). When replacing the dev_pm_domain_attach() and
> dev_pm_domain_detach() in bus probe and bus remove, it ensures that the
> device is detached from its PM domain in device_unbind_cleanup(), only
> after all driver's devres-managed resources have been release.
> 
> For flexibility, the implemented devm_pm_domain_attach() has 2 state
> arguments, one for the domain state on attach, one for the domain state on
> detach.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
> 
> Changes in v2:
> - none; this patch is new
> 
>  drivers/base/power/common.c | 59 +++++++++++++++++++++++++++++++++++++
>  include/linux/pm_domain.h   |  8 +++++
>  2 files changed, 67 insertions(+)
> 
> diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c
> index 781968a128ff..6ef0924efe2e 100644
> --- a/drivers/base/power/common.c
> +++ b/drivers/base/power/common.c
> @@ -115,6 +115,65 @@ int dev_pm_domain_attach(struct device *dev, bool power_on)
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_domain_attach);
>  
> +/**
> + * devm_pm_domain_detach_off - devres action for devm_pm_domain_attach() to
> + * detach a device and power it off.
> + * @dev: device to detach.
> + *
> + * This function reverse the actions from devm_pm_domain_attach().
> + * It will be invoked during the remove phase from drivers implicitly.
> + */
> +static void devm_pm_domain_detach_off(void *dev)
> +{
> +	dev_pm_domain_detach(dev, true);
> +}
> +
> +/**
> + * devm_pm_domain_detach_on - devres action for devm_pm_domain_attach() to
> + * detach a device and power it on.
> + * @dev: device to detach.
> + *
> + * This function reverse the actions from devm_pm_domain_attach().
> + * It will be invoked during the remove phase from drivers implicitly.
> + */
> +static void devm_pm_domain_detach_on(void *dev)
> +{
> +	dev_pm_domain_detach(dev, false);
> +}
> +
> +/**
> + * devm_pm_domain_attach - devres-enabled version of dev_pm_domain_attach()
> + * @dev: Device to attach.
> + * @attach_power_on: Use to indicate whether we should power on the device
> + *                   when attaching (true indicates the device is powered on
> + *                   when attaching).
> + * @detach_power_off: Used to indicate whether we should power off the device
> + *                    when detaching (true indicates the device is powered off
> + *                    when detaching).
> + *
> + * NOTE: this will also handle calling dev_pm_domain_detach() for
> + * you during remove phase.
> + *
> + * Returns 0 on successfully attached PM domain, or a negative error code in
> + * case of a failure.
> + */
> +int devm_pm_domain_attach(struct device *dev, bool attach_power_on,
> +			  bool detach_power_off)

Do we have examples where we power on a device and leave it powered on
(or do not power on device on attach but power off it on detach)? I
believe devm release should strictly mirror the acquisition, so separate
flag is not needed.


> +{
> +	int ret;
> +
> +	ret = dev_pm_domain_attach(dev, attach_power_on);
> +	if (ret)
> +		return ret;
> +
> +	if (detach_power_off)
> +		return devm_add_action_or_reset(dev, devm_pm_domain_detach_off,
> +						dev);
> +
> +	return devm_add_action_or_reset(dev, devm_pm_domain_detach_on, dev);

Instead of 2 separate cleanup methods maybe define dedicated devres:

struct dev_pm_domain_devres {
	struct device *dev;
	bool power_off;
}

?

> +}
> +EXPORT_SYMBOL_GPL(devm_pm_domain_attach);
> +
>  /**
>   * dev_pm_domain_attach_by_id - Associate a device with one of its PM domains.
>   * @dev: The device used to lookup the PM domain.
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index 0b18160901a2..ee798b090d17 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -509,6 +509,8 @@ struct device *dev_pm_domain_attach_by_name(struct device *dev,
>  int dev_pm_domain_attach_list(struct device *dev,
>  			      const struct dev_pm_domain_attach_data *data,
>  			      struct dev_pm_domain_list **list);
> +int devm_pm_domain_attach(struct device *dev, bool attach_power_on,
> +			  bool detach_power_off);
>  int devm_pm_domain_attach_list(struct device *dev,
>  			       const struct dev_pm_domain_attach_data *data,
>  			       struct dev_pm_domain_list **list);
> @@ -539,6 +541,12 @@ static inline int dev_pm_domain_attach_list(struct device *dev,
>  	return 0;
>  }
>  
> +static int devm_pm_domain_attach(struct device *dev, bool attach_power_on,

Needs to be marked "inline".

> +				 bool detach_power_off)
> +{
> +	return 0;
> +}
> +
>  static inline int devm_pm_domain_attach_list(struct device *dev,
>  					     const struct dev_pm_domain_attach_data *data,
>  					     struct dev_pm_domain_list **list)

Thanks.

-- 
Dmitry

Re: [PATCH v2 1/2] PM: domains: Add devres variant for dev_pm_domain_attach()

[Ulf Hansson]

On Tue, 27 May 2025 at 23:27, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
>
> Hi Claudiu,
>
> On Mon, May 26, 2025 at 03:20:53PM +0300, Claudiu 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.
>
> I think you are concentrating too much on runtime PM aspect of this. As
> you mentioned in the last sentence the same issue may happen in the
> absence of runtime PM if the power domain code will shut down the device
> while it is not fully cleaned up.
>
> >
> > Add devm_pm_domain_attach(). When replacing the dev_pm_domain_attach() and
> > dev_pm_domain_detach() in bus probe and bus remove, it ensures that the
> > device is detached from its PM domain in device_unbind_cleanup(), only
> > after all driver's devres-managed resources have been release.
> >
> > For flexibility, the implemented devm_pm_domain_attach() has 2 state
> > arguments, one for the domain state on attach, one for the domain state on
> > detach.
> >
> > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> > ---
> >
> > Changes in v2:
> > - none; this patch is new
> >
> >  drivers/base/power/common.c | 59 +++++++++++++++++++++++++++++++++++++
> >  include/linux/pm_domain.h   |  8 +++++
> >  2 files changed, 67 insertions(+)
> >
> > diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c
> > index 781968a128ff..6ef0924efe2e 100644
> > --- a/drivers/base/power/common.c
> > +++ b/drivers/base/power/common.c
> > @@ -115,6 +115,65 @@ int dev_pm_domain_attach(struct device *dev, bool power_on)
> >  }
> >  EXPORT_SYMBOL_GPL(dev_pm_domain_attach);
> >
> > +/**
> > + * devm_pm_domain_detach_off - devres action for devm_pm_domain_attach() to
> > + * detach a device and power it off.
> > + * @dev: device to detach.
> > + *
> > + * This function reverse the actions from devm_pm_domain_attach().
> > + * It will be invoked during the remove phase from drivers implicitly.
> > + */
> > +static void devm_pm_domain_detach_off(void *dev)
> > +{
> > +     dev_pm_domain_detach(dev, true);
> > +}
> > +
> > +/**
> > + * devm_pm_domain_detach_on - devres action for devm_pm_domain_attach() to
> > + * detach a device and power it on.
> > + * @dev: device to detach.
> > + *
> > + * This function reverse the actions from devm_pm_domain_attach().
> > + * It will be invoked during the remove phase from drivers implicitly.
> > + */
> > +static void devm_pm_domain_detach_on(void *dev)
> > +{
> > +     dev_pm_domain_detach(dev, false);
> > +}
> > +
> > +/**
> > + * devm_pm_domain_attach - devres-enabled version of dev_pm_domain_attach()
> > + * @dev: Device to attach.
> > + * @attach_power_on: Use to indicate whether we should power on the device
> > + *                   when attaching (true indicates the device is powered on
> > + *                   when attaching).
> > + * @detach_power_off: Used to indicate whether we should power off the device
> > + *                    when detaching (true indicates the device is powered off
> > + *                    when detaching).
> > + *
> > + * NOTE: this will also handle calling dev_pm_domain_detach() for
> > + * you during remove phase.
> > + *
> > + * Returns 0 on successfully attached PM domain, or a negative error code in
> > + * case of a failure.
> > + */
> > +int devm_pm_domain_attach(struct device *dev, bool attach_power_on,
> > +                       bool detach_power_off)
>
> Do we have examples where we power on a device and leave it powered on
> (or do not power on device on attach but power off it on detach)? I
> believe devm release should strictly mirror the acquisition, so separate
> flag is not needed.

This sounds reasonable for me too.

Note that, in most of the *special* cases for where
dev_pm_domain_attach|detach() is used today, the corresponding PM
domain is managed by genpd through a DT based configuration. And genpd
via genpd_dev_pm_attach|detach() doesn't even take this as an
in-parameter.

So this is solely for the behaviour for the acpi PM domain, just to
make sure that's clear.

[...]

Kind regards
Uffe

Re: [PATCH v2 1/2] PM: domains: Add devres variant for dev_pm_domain_attach()

[Claudiu Beznea]

Hi, Ulf,

On 28.05.2025 12:31, Ulf Hansson wrote:
>>> +
>>> +/**
>>> + * devm_pm_domain_attach - devres-enabled version of dev_pm_domain_attach()
>>> + * @dev: Device to attach.
>>> + * @attach_power_on: Use to indicate whether we should power on the device
>>> + *                   when attaching (true indicates the device is powered on
>>> + *                   when attaching).
>>> + * @detach_power_off: Used to indicate whether we should power off the device
>>> + *                    when detaching (true indicates the device is powered off
>>> + *                    when detaching).
>>> + *
>>> + * NOTE: this will also handle calling dev_pm_domain_detach() for
>>> + * you during remove phase.
>>> + *
>>> + * Returns 0 on successfully attached PM domain, or a negative error code in
>>> + * case of a failure.
>>> + */
>>> +int devm_pm_domain_attach(struct device *dev, bool attach_power_on,
>>> +                       bool detach_power_off)
>> Do we have examples where we power on a device and leave it powered on
>> (or do not power on device on attach but power off it on detach)? I
>> believe devm release should strictly mirror the acquisition, so separate
>> flag is not needed.
> This sounds reasonable for me too.

Then I'll drop the detach_power_off in the next version.

Thank you for your review,
Claudiu

> 
> Note that, in most of the *special* cases for where
> dev_pm_domain_attach|detach() is used today, the corresponding PM
> domain is managed by genpd through a DT based configuration. And genpd
> via genpd_dev_pm_attach|detach() doesn't even take this as an
> in-parameter.
> 
> So this is solely for the behaviour for the acpi PM domain, just to
> make sure that's clear.
> 
> [...]
> 
> Kind regards
> Uffe

Re: [PATCH v2 1/2] PM: domains: Add devres variant for dev_pm_domain_attach()

[Claudiu Beznea]

Hi, Dmitry,

On 28.05.2025 00:27, Dmitry Torokhov wrote:
> Hi Claudiu,
> 
> On Mon, May 26, 2025 at 03:20:53PM +0300, Claudiu 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.
> 
> I think you are concentrating too much on runtime PM aspect of this. As
> you mentioned in the last sentence the same issue may happen in the
> absence of runtime PM if the power domain code will shut down the device
> while it is not fully cleaned up.

I see your point. Just wanted to describe the scenario that leads to this
patch.

> 
>>
>> Add devm_pm_domain_attach(). When replacing the dev_pm_domain_attach() and
>> dev_pm_domain_detach() in bus probe and bus remove, it ensures that the
>> device is detached from its PM domain in device_unbind_cleanup(), only
>> after all driver's devres-managed resources have been release.
>>
>> For flexibility, the implemented devm_pm_domain_attach() has 2 state
>> arguments, one for the domain state on attach, one for the domain state on
>> detach.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> ---
>>
>> Changes in v2:
>> - none; this patch is new
>>
>>  drivers/base/power/common.c | 59 +++++++++++++++++++++++++++++++++++++
>>  include/linux/pm_domain.h   |  8 +++++
>>  2 files changed, 67 insertions(+)
>>
>> diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c
>> index 781968a128ff..6ef0924efe2e 100644
>> --- a/drivers/base/power/common.c
>> +++ b/drivers/base/power/common.c
>> @@ -115,6 +115,65 @@ int dev_pm_domain_attach(struct device *dev, bool power_on)
>>  }
>>  EXPORT_SYMBOL_GPL(dev_pm_domain_attach);
>>  
>> +/**
>> + * devm_pm_domain_detach_off - devres action for devm_pm_domain_attach() to
>> + * detach a device and power it off.
>> + * @dev: device to detach.
>> + *
>> + * This function reverse the actions from devm_pm_domain_attach().
>> + * It will be invoked during the remove phase from drivers implicitly.
>> + */
>> +static void devm_pm_domain_detach_off(void *dev)
>> +{
>> +	dev_pm_domain_detach(dev, true);
>> +}
>> +
>> +/**
>> + * devm_pm_domain_detach_on - devres action for devm_pm_domain_attach() to
>> + * detach a device and power it on.
>> + * @dev: device to detach.
>> + *
>> + * This function reverse the actions from devm_pm_domain_attach().
>> + * It will be invoked during the remove phase from drivers implicitly.
>> + */
>> +static void devm_pm_domain_detach_on(void *dev)
>> +{
>> +	dev_pm_domain_detach(dev, false);
>> +}
>> +
>> +/**
>> + * devm_pm_domain_attach - devres-enabled version of dev_pm_domain_attach()
>> + * @dev: Device to attach.
>> + * @attach_power_on: Use to indicate whether we should power on the device
>> + *                   when attaching (true indicates the device is powered on
>> + *                   when attaching).
>> + * @detach_power_off: Used to indicate whether we should power off the device
>> + *                    when detaching (true indicates the device is powered off
>> + *                    when detaching).
>> + *
>> + * NOTE: this will also handle calling dev_pm_domain_detach() for
>> + * you during remove phase.
>> + *
>> + * Returns 0 on successfully attached PM domain, or a negative error code in
>> + * case of a failure.
>> + */
>> +int devm_pm_domain_attach(struct device *dev, bool attach_power_on,
>> +			  bool detach_power_off)
> 
> Do we have examples where we power on a device and leave it powered on
> (or do not power on device on attach but power off it on detach)? I

I haven't found one yet.

> believe devm release should strictly mirror the acquisition, so separate
> flag is not needed.

I was in the middle whether I should do it with 2 flags or only to revert
the acquisition.

> 
> 
>> +{
>> +	int ret;
>> +
>> +	ret = dev_pm_domain_attach(dev, attach_power_on);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (detach_power_off)
>> +		return devm_add_action_or_reset(dev, devm_pm_domain_detach_off,
>> +						dev);
>> +
>> +	return devm_add_action_or_reset(dev, devm_pm_domain_detach_on, dev);
> 
> Instead of 2 separate cleanup methods maybe define dedicated devres:
> 
> struct dev_pm_domain_devres {
> 	struct device *dev;
> 	bool power_off;
> }
> 
> ?

That was the other option I've thought about but I found the one with 2
cleanup methods to be simpler. What would you prefer here?

Ulf: could you please let me know what would you prefer here?

> 
>> +}
>> +EXPORT_SYMBOL_GPL(devm_pm_domain_attach);
>> +
>>  /**
>>   * dev_pm_domain_attach_by_id - Associate a device with one of its PM domains.
>>   * @dev: The device used to lookup the PM domain.
>> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
>> index 0b18160901a2..ee798b090d17 100644
>> --- a/include/linux/pm_domain.h
>> +++ b/include/linux/pm_domain.h
>> @@ -509,6 +509,8 @@ struct device *dev_pm_domain_attach_by_name(struct device *dev,
>>  int dev_pm_domain_attach_list(struct device *dev,
>>  			      const struct dev_pm_domain_attach_data *data,
>>  			      struct dev_pm_domain_list **list);
>> +int devm_pm_domain_attach(struct device *dev, bool attach_power_on,
>> +			  bool detach_power_off);
>>  int devm_pm_domain_attach_list(struct device *dev,
>>  			       const struct dev_pm_domain_attach_data *data,
>>  			       struct dev_pm_domain_list **list);
>> @@ -539,6 +541,12 @@ static inline int dev_pm_domain_attach_list(struct device *dev,
>>  	return 0;
>>  }
>>  
>> +static int devm_pm_domain_attach(struct device *dev, bool attach_power_on,
> 
> Needs to be marked "inline".

Will do!

Thank you for your review,
Claudiu

> 
>> +				 bool detach_power_off)
>> +{
>> +	return 0;
>> +}
>> +
>>  static inline int devm_pm_domain_attach_list(struct device *dev,
>>  					     const struct dev_pm_domain_attach_data *data,
>>  					     struct dev_pm_domain_list **list)
> 
> Thanks.
> 

Re: [PATCH v2 1/2] PM: domains: Add devres variant for dev_pm_domain_attach()

[Ulf Hansson]

[...]

> >> +/**
> >> + * devm_pm_domain_attach - devres-enabled version of dev_pm_domain_attach()
> >> + * @dev: Device to attach.
> >> + * @attach_power_on: Use to indicate whether we should power on the device
> >> + *                   when attaching (true indicates the device is powered on
> >> + *                   when attaching).
> >> + * @detach_power_off: Used to indicate whether we should power off the device
> >> + *                    when detaching (true indicates the device is powered off
> >> + *                    when detaching).
> >> + *
> >> + * NOTE: this will also handle calling dev_pm_domain_detach() for
> >> + * you during remove phase.
> >> + *
> >> + * Returns 0 on successfully attached PM domain, or a negative error code in
> >> + * case of a failure.
> >> + */
> >> +int devm_pm_domain_attach(struct device *dev, bool attach_power_on,
> >> +                      bool detach_power_off)
> >
> > Do we have examples where we power on a device and leave it powered on
> > (or do not power on device on attach but power off it on detach)? I
>
> I haven't found one yet.
>
> > believe devm release should strictly mirror the acquisition, so separate
> > flag is not needed.
>
> I was in the middle whether I should do it with 2 flags or only to revert
> the acquisition.
>
> >
> >
> >> +{
> >> +    int ret;
> >> +
> >> +    ret = dev_pm_domain_attach(dev, attach_power_on);
> >> +    if (ret)
> >> +            return ret;
> >> +
> >> +    if (detach_power_off)
> >> +            return devm_add_action_or_reset(dev, devm_pm_domain_detach_off,
> >> +                                            dev);
> >> +
> >> +    return devm_add_action_or_reset(dev, devm_pm_domain_detach_on, dev);
> >
> > Instead of 2 separate cleanup methods maybe define dedicated devres:
> >
> > struct dev_pm_domain_devres {
> >       struct device *dev;
> >       bool power_off;
> > }
> >
> > ?
>
> That was the other option I've thought about but I found the one with 2
> cleanup methods to be simpler. What would you prefer here?
>
> Ulf: could you please let me know what would you prefer here?

As it looks like we agreed to use one cleanup method, the struct
dev_pm_domain_devres seems superfluous to me.

[...]

Kind regards
Uffe

Re: [PATCH v2 1/2] PM: domains: Add devres variant for dev_pm_domain_attach()

[Dmitry Torokhov]

On Wed, May 28, 2025 at 06:04:45PM +0200, Ulf Hansson wrote:
> [...]
> 
> > >> +/**
> > >> + * devm_pm_domain_attach - devres-enabled version of dev_pm_domain_attach()
> > >> + * @dev: Device to attach.
> > >> + * @attach_power_on: Use to indicate whether we should power on the device
> > >> + *                   when attaching (true indicates the device is powered on
> > >> + *                   when attaching).
> > >> + * @detach_power_off: Used to indicate whether we should power off the device
> > >> + *                    when detaching (true indicates the device is powered off
> > >> + *                    when detaching).
> > >> + *
> > >> + * NOTE: this will also handle calling dev_pm_domain_detach() for
> > >> + * you during remove phase.
> > >> + *
> > >> + * Returns 0 on successfully attached PM domain, or a negative error code in
> > >> + * case of a failure.
> > >> + */
> > >> +int devm_pm_domain_attach(struct device *dev, bool attach_power_on,
> > >> +                      bool detach_power_off)
> > >
> > > Do we have examples where we power on a device and leave it powered on
> > > (or do not power on device on attach but power off it on detach)? I
> >
> > I haven't found one yet.
> >
> > > believe devm release should strictly mirror the acquisition, so separate
> > > flag is not needed.
> >
> > I was in the middle whether I should do it with 2 flags or only to revert
> > the acquisition.
> >
> > >
> > >
> > >> +{
> > >> +    int ret;
> > >> +
> > >> +    ret = dev_pm_domain_attach(dev, attach_power_on);
> > >> +    if (ret)
> > >> +            return ret;
> > >> +
> > >> +    if (detach_power_off)
> > >> +            return devm_add_action_or_reset(dev, devm_pm_domain_detach_off,
> > >> +                                            dev);
> > >> +
> > >> +    return devm_add_action_or_reset(dev, devm_pm_domain_detach_on, dev);
> > >
> > > Instead of 2 separate cleanup methods maybe define dedicated devres:
> > >
> > > struct dev_pm_domain_devres {
> > >       struct device *dev;
> > >       bool power_off;
> > > }
> > >
> > > ?
> >
> > That was the other option I've thought about but I found the one with 2
> > cleanup methods to be simpler. What would you prefer here?
> >
> > Ulf: could you please let me know what would you prefer here?
> 
> As it looks like we agreed to use one cleanup method, the struct
> dev_pm_domain_devres seems superfluous to me.

I think we agreed that cleanup should mirror the acquisition, that is
true. But since attaching to the domain has an option to either turn the
device on or not we still need 2 cleanup branches. They can either be
implemented with 2 cleanup callbacks or with 1 callback and dedicated
devres structure.

Thanks.

-- 
Dmitry

Re: [PATCH v2 1/2] PM: domains: Add devres variant for dev_pm_domain_attach()

[Ulf Hansson]

On Wed, 28 May 2025 at 18:09, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
>
> On Wed, May 28, 2025 at 06:04:45PM +0200, Ulf Hansson wrote:
> > [...]
> >
> > > >> +/**
> > > >> + * devm_pm_domain_attach - devres-enabled version of dev_pm_domain_attach()
> > > >> + * @dev: Device to attach.
> > > >> + * @attach_power_on: Use to indicate whether we should power on the device
> > > >> + *                   when attaching (true indicates the device is powered on
> > > >> + *                   when attaching).
> > > >> + * @detach_power_off: Used to indicate whether we should power off the device
> > > >> + *                    when detaching (true indicates the device is powered off
> > > >> + *                    when detaching).
> > > >> + *
> > > >> + * NOTE: this will also handle calling dev_pm_domain_detach() for
> > > >> + * you during remove phase.
> > > >> + *
> > > >> + * Returns 0 on successfully attached PM domain, or a negative error code in
> > > >> + * case of a failure.
> > > >> + */
> > > >> +int devm_pm_domain_attach(struct device *dev, bool attach_power_on,
> > > >> +                      bool detach_power_off)
> > > >
> > > > Do we have examples where we power on a device and leave it powered on
> > > > (or do not power on device on attach but power off it on detach)? I
> > >
> > > I haven't found one yet.
> > >
> > > > believe devm release should strictly mirror the acquisition, so separate
> > > > flag is not needed.
> > >
> > > I was in the middle whether I should do it with 2 flags or only to revert
> > > the acquisition.
> > >
> > > >
> > > >
> > > >> +{
> > > >> +    int ret;
> > > >> +
> > > >> +    ret = dev_pm_domain_attach(dev, attach_power_on);
> > > >> +    if (ret)
> > > >> +            return ret;
> > > >> +
> > > >> +    if (detach_power_off)
> > > >> +            return devm_add_action_or_reset(dev, devm_pm_domain_detach_off,
> > > >> +                                            dev);
> > > >> +
> > > >> +    return devm_add_action_or_reset(dev, devm_pm_domain_detach_on, dev);
> > > >
> > > > Instead of 2 separate cleanup methods maybe define dedicated devres:
> > > >
> > > > struct dev_pm_domain_devres {
> > > >       struct device *dev;
> > > >       bool power_off;
> > > > }
> > > >
> > > > ?
> > >
> > > That was the other option I've thought about but I found the one with 2
> > > cleanup methods to be simpler. What would you prefer here?
> > >
> > > Ulf: could you please let me know what would you prefer here?
> >
> > As it looks like we agreed to use one cleanup method, the struct
> > dev_pm_domain_devres seems superfluous to me.
>
> I think we agreed that cleanup should mirror the acquisition, that is
> true. But since attaching to the domain has an option to either turn the
> device on or not we still need 2 cleanup branches. They can either be
> implemented with 2 cleanup callbacks or with 1 callback and dedicated
> devres structure.

Yes, you are right. Better with one callback and using struct
dev_pm_domain_devres to manage the power_off parameter.

Kind regards
Uffe

[PATCH v2 2/2] driver core: platform: Use devm_pm_domain_attach()

[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.

Use devm_pm_domain_attach(). This ensures that driver-specific devm actions
or reset functions are executed in sequence with PM domain attach
action or reset and the driver will not end up runtime resuming the device
when it is not anymore managed by it's PM domain.

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

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 | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 075ec1d1b73a..0b2036d4bf4b 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -1396,15 +1396,12 @@ static int platform_probe(struct device *_dev)
 	if (ret < 0)
 		return ret;
 
-	ret = dev_pm_domain_attach(_dev, true);
+	ret = devm_pm_domain_attach(_dev, true, true);
 	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