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

[PATCH v3 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 v3:
- dropped the detach_power_off argument of devm_pm_domain_attach()
- use a single cleanup function
- fixed build warning

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 | 50 +++++++++++++++++++++++++++++++++++++
 include/linux/pm_domain.h   |  6 +++++
 3 files changed, 58 insertions(+), 6 deletions(-)

-- 
2.43.0

[PATCH v3 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 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/power/common.c | 50 +++++++++++++++++++++++++++++++++++++
 include/linux/pm_domain.h   |  6 +++++
 2 files changed, 56 insertions(+)

diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c
index 781968a128ff..82ea20b343f5 100644
--- a/drivers/base/power/common.c
+++ b/drivers/base/power/common.c
@@ -115,6 +115,56 @@ int dev_pm_domain_attach(struct device *dev, bool power_on)
 }
 EXPORT_SYMBOL_GPL(dev_pm_domain_attach);
 
+/**
+ * devm_pm_domain_detach - devres action for devm_pm_domain_attach() to
+ * detach a device from its domain.
+ * @dev: device to detach.
+ * @res: indicate if the device should be powered off
+ *
+ * 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(struct device *dev, void *res)
+{
+	bool *power_off = res;
+
+	dev_pm_domain_detach(dev, *power_off);
+}
+
+/**
+ * devm_pm_domain_attach - devres-enabled version of dev_pm_domain_attach()
+ * @dev: Device to attach.
+ * @power_on: Use to indicate whether we should power on the device
+ *            when attaching.
+ *
+ * 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 power_on)
+{
+	bool *power_off;
+	int ret;
+
+	power_off = devres_alloc(devm_pm_domain_detach, sizeof(*power_off), GFP_KERNEL);
+	if (!power_off)
+		return -ENOMEM;
+
+	ret = dev_pm_domain_attach(dev, power_on);
+	if (ret) {
+		devres_free(power_off);
+		return ret;
+	}
+
+	*power_off = power_on;
+	devres_add(dev, power_off);
+
+	return ret;
+}
+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..f78b6b4dd734 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -509,6 +509,7 @@ 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 power_on);
 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 +540,11 @@ static inline int dev_pm_domain_attach_list(struct device *dev,
 	return 0;
 }
 
+static inline int devm_pm_domain_attach(struct device *dev, bool power_on)
+{
+	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 v3 1/2] PM: domains: Add devres variant for dev_pm_domain_attach()

[Rafael J. Wysocki]

On Fri, Jun 6, 2025 at 1:18 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.

Don't use devm_pm_runtime_enable() then.

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

dev_pm_domain_attach() is not part driver API and I'm not convinced at
all by the arguments above.

Thanks!

> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
>
> 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/power/common.c | 50 +++++++++++++++++++++++++++++++++++++
>  include/linux/pm_domain.h   |  6 +++++
>  2 files changed, 56 insertions(+)
>
> diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c
> index 781968a128ff..82ea20b343f5 100644
> --- a/drivers/base/power/common.c
> +++ b/drivers/base/power/common.c
> @@ -115,6 +115,56 @@ int dev_pm_domain_attach(struct device *dev, bool power_on)
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_domain_attach);
>
> +/**
> + * devm_pm_domain_detach - devres action for devm_pm_domain_attach() to
> + * detach a device from its domain.
> + * @dev: device to detach.
> + * @res: indicate if the device should be powered off
> + *
> + * 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(struct device *dev, void *res)
> +{
> +       bool *power_off = res;
> +
> +       dev_pm_domain_detach(dev, *power_off);
> +}
> +
> +/**
> + * devm_pm_domain_attach - devres-enabled version of dev_pm_domain_attach()
> + * @dev: Device to attach.
> + * @power_on: Use to indicate whether we should power on the device
> + *            when attaching.
> + *
> + * 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 power_on)
> +{
> +       bool *power_off;
> +       int ret;
> +
> +       power_off = devres_alloc(devm_pm_domain_detach, sizeof(*power_off), GFP_KERNEL);
> +       if (!power_off)
> +               return -ENOMEM;
> +
> +       ret = dev_pm_domain_attach(dev, power_on);
> +       if (ret) {
> +               devres_free(power_off);
> +               return ret;
> +       }
> +
> +       *power_off = power_on;
> +       devres_add(dev, power_off);
> +
> +       return ret;
> +}
> +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..f78b6b4dd734 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -509,6 +509,7 @@ 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 power_on);
>  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 +540,11 @@ static inline int dev_pm_domain_attach_list(struct device *dev,
>         return 0;
>  }
>
> +static inline int devm_pm_domain_attach(struct device *dev, bool power_on)
> +{
> +       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 v3 1/2] PM: domains: Add devres variant for dev_pm_domain_attach()

[Dmitry Torokhov]

On Fri, Jun 06, 2025 at 06:00:34PM +0200, Rafael J. Wysocki wrote:
> On Fri, Jun 6, 2025 at 1:18 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.
> 
> Don't use devm_pm_runtime_enable() then.

What about other devm_ APIs? Are you suggesting that platform drivers
should not be using devm_clk*(), devm_regulator_*(),
devm_request_*_irq() and devm_add_action_or_reset()? Because again,
dev_pm_domain_detach() that is called by platform bus_remove() may shut
off the device too early, before cleanup code has a chance to execute
proper cleanup.

The issue is not limited to runtime PM.

> 
> > 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.
> 
> dev_pm_domain_attach() is not part driver API and I'm not convinced at

Is the concern that devm_pm_domain_attach() will be [ab]used by drivers?
In that case we can go back to using devres group to enforce ordering,
but proper ordering is needed.

> all by the arguments above.

Please reconsider given the fact that issue is not limited to the
runtime PM.

Thanks.

-- 
Dmitry

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

[Rafael J. Wysocki]

On Fri, Jun 6, 2025 at 8:55 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Fri, Jun 06, 2025 at 06:00:34PM +0200, Rafael J. Wysocki wrote:
> > On Fri, Jun 6, 2025 at 1:18 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.
> >
> > Don't use devm_pm_runtime_enable() then.
>
> What about other devm_ APIs? Are you suggesting that platform drivers
> should not be using devm_clk*(), devm_regulator_*(),
> devm_request_*_irq() and devm_add_action_or_reset()? Because again,
> dev_pm_domain_detach() that is called by platform bus_remove() may shut
> off the device too early, before cleanup code has a chance to execute
> proper cleanup.
>
> The issue is not limited to runtime PM.
>
> >
> > > 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.
> >
> > dev_pm_domain_attach() is not part driver API and I'm not convinced at
>
> Is the concern that devm_pm_domain_attach() will be [ab]used by drivers?

Yes, among other things.

> In that case we can go back to using devres group to enforce ordering,
> but proper ordering is needed.

Sure.

> > all by the arguments above.
>
> Please reconsider given the fact that issue is not limited to the
> runtime PM.

PM domains are not resources, they are interfaces that are added to
devices by the bus types that need them and they also need to be
removed by those bus types.

A PM domain needs to go away at remove time because it may not make
sense to use PM domain callbacks without driver callbacks and if
enabled runtime PM is leaked beyond the point at which there are no
driver and bus type callbacks, this is exactly what may happen.

If you have ordering issues in drivers, that's where they are and
that's where they need to be addressed.

Thanks!

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

[Rafael J. Wysocki]

On Fri, Jun 6, 2025 at 10:01 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Fri, Jun 6, 2025 at 8:55 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > On Fri, Jun 06, 2025 at 06:00:34PM +0200, Rafael J. Wysocki wrote:
> > > On Fri, Jun 6, 2025 at 1:18 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.
> > >
> > > Don't use devm_pm_runtime_enable() then.
> >
> > What about other devm_ APIs? Are you suggesting that platform drivers
> > should not be using devm_clk*(), devm_regulator_*(),
> > devm_request_*_irq() and devm_add_action_or_reset()? Because again,
> > dev_pm_domain_detach() that is called by platform bus_remove() may shut
> > off the device too early, before cleanup code has a chance to execute
> > proper cleanup.
> >
> > The issue is not limited to runtime PM.
> >
> > >
> > > > 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.
> > >
> > > dev_pm_domain_attach() is not part driver API and I'm not convinced at
> >
> > Is the concern that devm_pm_domain_attach() will be [ab]used by drivers?
>
> Yes, among other things.

This would be much less objectionable to me if it were not devm_, but
also the current expectation is that the PM domain will be gone after
device_remove() has returned.

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

[Jonathan Cameron]

On Fri, 6 Jun 2025 22:01:52 +0200
"Rafael J. Wysocki" <rafael@kernel.org> wrote:

Hi Rafael,

> On Fri, Jun 6, 2025 at 8:55 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > On Fri, Jun 06, 2025 at 06:00:34PM +0200, Rafael J. Wysocki wrote:  
> > > On Fri, Jun 6, 2025 at 1:18 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.  
> > >
> > > Don't use devm_pm_runtime_enable() then.  
> >
> > What about other devm_ APIs? Are you suggesting that platform drivers
> > should not be using devm_clk*(), devm_regulator_*(),
> > devm_request_*_irq() and devm_add_action_or_reset()? Because again,
> > dev_pm_domain_detach() that is called by platform bus_remove() may shut
> > off the device too early, before cleanup code has a chance to execute
> > proper cleanup.
> >
> > The issue is not limited to runtime PM.
> >  
> > >  
> > > > 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.  
> > >
> > > dev_pm_domain_attach() is not part driver API and I'm not convinced at  
> >
> > Is the concern that devm_pm_domain_attach() will be [ab]used by drivers?  
> 
> Yes, among other things.

Maybe naming could make abuse at least obvious to spot? e.g.
pm_domain_attach_with_devm_release()

> 
> > In that case we can go back to using devres group to enforce ordering,
> > but proper ordering is needed.  
> 
> Sure.

Ok. Please take a look at:
https://lore.kernel.org/all/20250215130849.227812-1-claudiu.beznea.uj@bp.renesas.com/

> 
> > > all by the arguments above.  
> >
> > Please reconsider given the fact that issue is not limited to the
> > runtime PM.  
> 
> PM domains are not resources, they are interfaces that are added to
> devices by the bus types that need them and they also need to be
> removed by those bus types.
> 
> A PM domain needs to go away at remove time because it may not make
> sense to use PM domain callbacks without driver callbacks and if
> enabled runtime PM is leaked beyond the point at which there are no
> driver and bus type callbacks, this is exactly what may happen.

I'm fully in agreement with all that.  However, I'm not sure on relevance to this
discussion as (if the new function is used as intended) the pm domain will get
removed before any problems occur. The only call in
__device_release_driver() between the bus remove and device_unbind_cleanup()
which unrolls the devm stuff as the first thing it does is
dev->bus->dma_cleanup().

Maybe I'm missing another path?

> 
> If you have ordering issues in drivers, that's where they are and
> that's where they need to be addressed.
> 

To my viewpoint, the ordering issue is in the bus driver because devm
setup calls are in device driver probe() which comes after the
bus_type->probe() but unwound after the bus_type->remove() - they should
be before that if they are only for device driver usage.  There are
devm uses in bus probe functions though which is assume why the ordering
is as we have it.

Personally I preferred the devres group in the bus driver. It's a
model a couple of busses are already using.  The solution in this
patch also worked for me though as a good compromise between different view
points.

Claudiu has been working on different solutions to this problem for a long
time so lets work together to find a solution that finally resolves it.

Jonathan


> Thanks!

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

[Rafael J. Wysocki]

On Sat, Jun 7, 2025 at 3:06 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Fri, 6 Jun 2025 22:01:52 +0200
> "Rafael J. Wysocki" <rafael@kernel.org> wrote:
>
> Hi Rafael,
>
> > On Fri, Jun 6, 2025 at 8:55 PM Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> > >
> > > On Fri, Jun 06, 2025 at 06:00:34PM +0200, Rafael J. Wysocki wrote:
> > > > On Fri, Jun 6, 2025 at 1:18 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.
> > > >
> > > > Don't use devm_pm_runtime_enable() then.
> > >
> > > What about other devm_ APIs? Are you suggesting that platform drivers
> > > should not be using devm_clk*(), devm_regulator_*(),
> > > devm_request_*_irq() and devm_add_action_or_reset()? Because again,
> > > dev_pm_domain_detach() that is called by platform bus_remove() may shut
> > > off the device too early, before cleanup code has a chance to execute
> > > proper cleanup.
> > >
> > > The issue is not limited to runtime PM.
> > >
> > > >
> > > > > 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.
> > > >
> > > > dev_pm_domain_attach() is not part driver API and I'm not convinced at
> > >
> > > Is the concern that devm_pm_domain_attach() will be [ab]used by drivers?
> >
> > Yes, among other things.
>
> Maybe naming could make abuse at least obvious to spot? e.g.
> pm_domain_attach_with_devm_release()

If I'm not mistaken, it is not even necessary to use devres for this.

You might as well add a dev_pm_domain_detach() call to
device_unbind_cleanup() after devres_release_all().  There is a slight
complication related to the second argument of it, but I suppose that
this can be determined at the attach time and stored in a new device
PM flag, or similar.

Note that dev->pm_domain is expected to be cleared by ->detach(), so
this should not cause the domain to be detached twice in a row from
the same device, but that needs to be double-checked.

Thanks!

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

[Claudiu Beznea]

Hi, Rafael,

On 09.06.2025 22:59, Rafael J. Wysocki wrote:
> On Sat, Jun 7, 2025 at 3:06 PM Jonathan Cameron <jic23@kernel.org> wrote:
>>
>> On Fri, 6 Jun 2025 22:01:52 +0200
>> "Rafael J. Wysocki" <rafael@kernel.org> wrote:
>>
>> Hi Rafael,
>>
>>> On Fri, Jun 6, 2025 at 8:55 PM Dmitry Torokhov
>>> <dmitry.torokhov@gmail.com> wrote:
>>>>
>>>> On Fri, Jun 06, 2025 at 06:00:34PM +0200, Rafael J. Wysocki wrote:
>>>>> On Fri, Jun 6, 2025 at 1:18 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.
>>>>>
>>>>> Don't use devm_pm_runtime_enable() then.
>>>>
>>>> What about other devm_ APIs? Are you suggesting that platform drivers
>>>> should not be using devm_clk*(), devm_regulator_*(),
>>>> devm_request_*_irq() and devm_add_action_or_reset()? Because again,
>>>> dev_pm_domain_detach() that is called by platform bus_remove() may shut
>>>> off the device too early, before cleanup code has a chance to execute
>>>> proper cleanup.
>>>>
>>>> The issue is not limited to runtime PM.
>>>>
>>>>>
>>>>>> 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.
>>>>>
>>>>> dev_pm_domain_attach() is not part driver API and I'm not convinced at
>>>>
>>>> Is the concern that devm_pm_domain_attach() will be [ab]used by drivers?
>>>
>>> Yes, among other things.
>>
>> Maybe naming could make abuse at least obvious to spot? e.g.
>> pm_domain_attach_with_devm_release()
> 
> If I'm not mistaken, it is not even necessary to use devres for this.
> 
> You might as well add a dev_pm_domain_detach() call to
> device_unbind_cleanup() after devres_release_all().  There is a slight
> complication related to the second argument of it, but I suppose that
> this can be determined at the attach time and stored in a new device
> PM flag, or similar.

I can try this as well.

Another option I see at the moment would be keep the code added in
drivers/base/power/common.c in drivers/base/platform.c, something like:

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 075ec1d1b73a..391d725cd4c7 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -1376,10 +1376,18 @@ static int platform_uevent(const struct device
*dev, struct kobj_uevent_env *env
        return 0;
 }

+static void platform_dev_pm_domain_detach(struct device *dev, void *res)
+{
+       bool *power_off = res;
+
+       dev_pm_domain_detach(dev, *power_off);
+}
+
 static int platform_probe(struct device *_dev)
 {
        struct platform_driver *drv = to_platform_driver(_dev->driver);
        struct platform_device *dev = to_platform_device(_dev);
+       bool *power_off;
        int ret;

        /*
@@ -1396,15 +1404,22 @@ static int platform_probe(struct device *_dev)
        if (ret < 0)
                return ret;

+       power_off = devres_alloc(platform_dev_pm_domain_detach,
sizeof(*power_off),
+                                GFP_KERNEL);
+       if (!power_off)
+               return -ENOMEM;
+
        ret = dev_pm_domain_attach(_dev, true);
-       if (ret)
+       if (ret) {
+               devres_free(power_off);
                goto out;
+       }

-       if (drv->probe) {
+       *power_off = true;
+       devres_add(_dev, power_off);
+
+       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 +1437,6 @@ static void platform_remove(struct device *_dev)

        if (drv->remove)
                drv->remove(dev);
-       dev_pm_domain_detach(_dev, true);
 }

but this would involve duplicating code, as, sooner or later, this would
have to be done for other busses as well.

Could you please let me know what option would you prefer so that I can go
forward with it?

Thank you for your review,
Claudiu

> 
> Note that dev->pm_domain is expected to be cleared by ->detach(), so
> this should not cause the domain to be detached twice in a row from
> the same device, but that needs to be double-checked.
> 
> Thanks!

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

[Jonathan Cameron]

On Wed, 11 Jun 2025 12:11:08 +0300
Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:

> Hi, Rafael,
> 
> On 09.06.2025 22:59, Rafael J. Wysocki wrote:
> > On Sat, Jun 7, 2025 at 3:06 PM Jonathan Cameron <jic23@kernel.org> wrote:  
> >>
> >> On Fri, 6 Jun 2025 22:01:52 +0200
> >> "Rafael J. Wysocki" <rafael@kernel.org> wrote:
> >>
> >> Hi Rafael,
> >>  
> >>> On Fri, Jun 6, 2025 at 8:55 PM Dmitry Torokhov
> >>> <dmitry.torokhov@gmail.com> wrote:  
> >>>>
> >>>> On Fri, Jun 06, 2025 at 06:00:34PM +0200, Rafael J. Wysocki wrote:  
> >>>>> On Fri, Jun 6, 2025 at 1:18 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.  
> >>>>>
> >>>>> Don't use devm_pm_runtime_enable() then.  
> >>>>
> >>>> What about other devm_ APIs? Are you suggesting that platform drivers
> >>>> should not be using devm_clk*(), devm_regulator_*(),
> >>>> devm_request_*_irq() and devm_add_action_or_reset()? Because again,
> >>>> dev_pm_domain_detach() that is called by platform bus_remove() may shut
> >>>> off the device too early, before cleanup code has a chance to execute
> >>>> proper cleanup.
> >>>>
> >>>> The issue is not limited to runtime PM.
> >>>>  
> >>>>>  
> >>>>>> 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.  
> >>>>>
> >>>>> dev_pm_domain_attach() is not part driver API and I'm not convinced at  
> >>>>
> >>>> Is the concern that devm_pm_domain_attach() will be [ab]used by drivers?  
> >>>
> >>> Yes, among other things.  
> >>
> >> Maybe naming could make abuse at least obvious to spot? e.g.
> >> pm_domain_attach_with_devm_release()  
> > 
> > If I'm not mistaken, it is not even necessary to use devres for this.
> > 
> > You might as well add a dev_pm_domain_detach() call to
> > device_unbind_cleanup() after devres_release_all().  There is a slight
> > complication related to the second argument of it, but I suppose that
> > this can be determined at the attach time and stored in a new device
> > PM flag, or similar.  
> 
> I can try this as well.
> 
> Another option I see at the moment would be keep the code added in
> drivers/base/power/common.c in drivers/base/platform.c, something like:

Whilst this avoids the exposure of a devm interface by just
pushing it down to each bus, we'll for ever be rejecting cleanups
that unify it.  So I'd rather explore Rafael's suggestion to just
handle this one in the driver core.

That only deals with the pm domain issue and not the other ones
Dmitry refers to where devres being handled after bus->remove()
bites us, but maybe that is the right way forwards for now at least.

Jonathan

> 
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 075ec1d1b73a..391d725cd4c7 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -1376,10 +1376,18 @@ static int platform_uevent(const struct device
> *dev, struct kobj_uevent_env *env
>         return 0;
>  }
> 
> +static void platform_dev_pm_domain_detach(struct device *dev, void *res)
> +{
> +       bool *power_off = res;
> +
> +       dev_pm_domain_detach(dev, *power_off);
> +}
> +
>  static int platform_probe(struct device *_dev)
>  {
>         struct platform_driver *drv = to_platform_driver(_dev->driver);
>         struct platform_device *dev = to_platform_device(_dev);
> +       bool *power_off;
>         int ret;
> 
>         /*
> @@ -1396,15 +1404,22 @@ static int platform_probe(struct device *_dev)
>         if (ret < 0)
>                 return ret;
> 
> +       power_off = devres_alloc(platform_dev_pm_domain_detach,
> sizeof(*power_off),
> +                                GFP_KERNEL);
> +       if (!power_off)
> +               return -ENOMEM;
> +
>         ret = dev_pm_domain_attach(_dev, true);
> -       if (ret)
> +       if (ret) {
> +               devres_free(power_off);
>                 goto out;
> +       }
> 
> -       if (drv->probe) {
> +       *power_off = true;
> +       devres_add(_dev, power_off);
> +
> +       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 +1437,6 @@ static void platform_remove(struct device *_dev)
> 
>         if (drv->remove)
>                 drv->remove(dev);
> -       dev_pm_domain_detach(_dev, true);
>  }
> 
> but this would involve duplicating code, as, sooner or later, this would
> have to be done for other busses as well.
> 
> Could you please let me know what option would you prefer so that I can go
> forward with it?
> 
> Thank you for your review,
> Claudiu
> 
> > 
> > Note that dev->pm_domain is expected to be cleared by ->detach(), so
> > this should not cause the domain to be detached twice in a row from
> > the same device, but that needs to be double-checked.
> > 
> > Thanks!  
> 

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

[Jonathan Cameron]

On Mon, 9 Jun 2025 21:59:57 +0200
"Rafael J. Wysocki" <rafael@kernel.org> wrote:

> On Sat, Jun 7, 2025 at 3:06 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Fri, 6 Jun 2025 22:01:52 +0200
> > "Rafael J. Wysocki" <rafael@kernel.org> wrote:
> >
> > Hi Rafael,
> >  
> > > On Fri, Jun 6, 2025 at 8:55 PM Dmitry Torokhov
> > > <dmitry.torokhov@gmail.com> wrote:  
> > > >
> > > > On Fri, Jun 06, 2025 at 06:00:34PM +0200, Rafael J. Wysocki wrote:  
> > > > > On Fri, Jun 6, 2025 at 1:18 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.  
> > > > >
> > > > > Don't use devm_pm_runtime_enable() then.  
> > > >
> > > > What about other devm_ APIs? Are you suggesting that platform drivers
> > > > should not be using devm_clk*(), devm_regulator_*(),
> > > > devm_request_*_irq() and devm_add_action_or_reset()? Because again,
> > > > dev_pm_domain_detach() that is called by platform bus_remove() may shut
> > > > off the device too early, before cleanup code has a chance to execute
> > > > proper cleanup.
> > > >
> > > > The issue is not limited to runtime PM.
> > > >  
> > > > >  
> > > > > > 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.  
> > > > >
> > > > > dev_pm_domain_attach() is not part driver API and I'm not convinced at  
> > > >
> > > > Is the concern that devm_pm_domain_attach() will be [ab]used by drivers?  
> > >
> > > Yes, among other things.  
> >
> > Maybe naming could make abuse at least obvious to spot? e.g.
> > pm_domain_attach_with_devm_release()  
> 
> If I'm not mistaken, it is not even necessary to use devres for this.
> 
> You might as well add a dev_pm_domain_detach() call to
> device_unbind_cleanup() after devres_release_all().  There is a slight
> complication related to the second argument of it, but I suppose that
> this can be determined at the attach time and stored in a new device
> PM flag, or similar.

That options sounds good to me.  I think this moves dev_pm_domain_detach()
call into the the driver core / perhaps device_unbind_cleanup().  It's a noop
if a domain was never attached so that should be fine.

Given that second parameter, I guess we can't move the dev_pm_domain_attach()
into the driver core as well so it is a little odd wrt to balance,
but with some documentation that is probably fine. I don't think we
really want a bus->remove_after_devres() callback for just this.
Ulf what do you think of this approach?

Jonathan

> 
> Note that dev->pm_domain is expected to be cleared by ->detach(), so
> this should not cause the domain to be detached twice in a row from
> the same device, but that needs to be double-checked.
> 
> Thanks!
> 

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

[Dmitry Torokhov]

On Wed, Jun 11, 2025 at 05:23:07PM +0100, Jonathan Cameron wrote:
> On Mon, 9 Jun 2025 21:59:57 +0200
> "Rafael J. Wysocki" <rafael@kernel.org> wrote:
> 
> > On Sat, Jun 7, 2025 at 3:06 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > >
> > > On Fri, 6 Jun 2025 22:01:52 +0200
> > > "Rafael J. Wysocki" <rafael@kernel.org> wrote:
> > >
> > > Hi Rafael,
> > >  
> > > > On Fri, Jun 6, 2025 at 8:55 PM Dmitry Torokhov
> > > > <dmitry.torokhov@gmail.com> wrote:  
> > > > >
> > > > > On Fri, Jun 06, 2025 at 06:00:34PM +0200, Rafael J. Wysocki wrote:  
> > > > > > On Fri, Jun 6, 2025 at 1:18 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.  
> > > > > >
> > > > > > Don't use devm_pm_runtime_enable() then.  
> > > > >
> > > > > What about other devm_ APIs? Are you suggesting that platform drivers
> > > > > should not be using devm_clk*(), devm_regulator_*(),
> > > > > devm_request_*_irq() and devm_add_action_or_reset()? Because again,
> > > > > dev_pm_domain_detach() that is called by platform bus_remove() may shut
> > > > > off the device too early, before cleanup code has a chance to execute
> > > > > proper cleanup.
> > > > >
> > > > > The issue is not limited to runtime PM.
> > > > >  
> > > > > >  
> > > > > > > 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.  
> > > > > >
> > > > > > dev_pm_domain_attach() is not part driver API and I'm not convinced at  
> > > > >
> > > > > Is the concern that devm_pm_domain_attach() will be [ab]used by drivers?  
> > > >
> > > > Yes, among other things.  
> > >
> > > Maybe naming could make abuse at least obvious to spot? e.g.
> > > pm_domain_attach_with_devm_release()  
> > 
> > If I'm not mistaken, it is not even necessary to use devres for this.
> > 
> > You might as well add a dev_pm_domain_detach() call to
> > device_unbind_cleanup() after devres_release_all().  There is a slight
> > complication related to the second argument of it, but I suppose that
> > this can be determined at the attach time and stored in a new device
> > PM flag, or similar.
> 
> That options sounds good to me.  I think this moves dev_pm_domain_detach()
> call into the the driver core / perhaps device_unbind_cleanup().  It's a noop
> if a domain was never attached so that should be fine.
> 
> Given that second parameter, I guess we can't move the dev_pm_domain_attach()
> into the driver core as well so it is a little odd wrt to balance,
> but with some documentation that is probably fine.

It is going to be confusing IMO and might lead to ordering issues again
if there are more resources allocated by bus probe() before domain
attach is called.

I know Rafael does not consider dev_pm_domain_attach() a "driver" API
(although I do not see much difference between driver and bus probe
code), but maybe we can solve this by using different name
(devres_controlled_pm_domain_attach() instead of
devm_pm_domain_attach()?).

OTOH we have devm_pm_domain_attach_list() already which is I guess
driver-level API...

Thanks.

-- 
Dmitry

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

[Claudiu Beznea]

Hi, Rafael,

On 09.06.2025 22:59, Rafael J. Wysocki wrote:
> On Sat, Jun 7, 2025 at 3:06 PM Jonathan Cameron <jic23@kernel.org> wrote:
>>
>> On Fri, 6 Jun 2025 22:01:52 +0200
>> "Rafael J. Wysocki" <rafael@kernel.org> wrote:
>>
>> Hi Rafael,
>>
>>> On Fri, Jun 6, 2025 at 8:55 PM Dmitry Torokhov
>>> <dmitry.torokhov@gmail.com> wrote:
>>>>
>>>> On Fri, Jun 06, 2025 at 06:00:34PM +0200, Rafael J. Wysocki wrote:
>>>>> On Fri, Jun 6, 2025 at 1:18 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.
>>>>>
>>>>> Don't use devm_pm_runtime_enable() then.
>>>>
>>>> What about other devm_ APIs? Are you suggesting that platform drivers
>>>> should not be using devm_clk*(), devm_regulator_*(),
>>>> devm_request_*_irq() and devm_add_action_or_reset()? Because again,
>>>> dev_pm_domain_detach() that is called by platform bus_remove() may shut
>>>> off the device too early, before cleanup code has a chance to execute
>>>> proper cleanup.
>>>>
>>>> The issue is not limited to runtime PM.
>>>>
>>>>>
>>>>>> 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.
>>>>>
>>>>> dev_pm_domain_attach() is not part driver API and I'm not convinced at
>>>>
>>>> Is the concern that devm_pm_domain_attach() will be [ab]used by drivers?
>>>
>>> Yes, among other things.
>>
>> Maybe naming could make abuse at least obvious to spot? e.g.
>> pm_domain_attach_with_devm_release()
> 
> If I'm not mistaken, it is not even necessary to use devres for this.
> 
> You might as well add a dev_pm_domain_detach() call to
> device_unbind_cleanup() after devres_release_all().  There is a slight
> complication related to the second argument of it, but I suppose that
> this can be determined at the attach time and stored in a new device
> PM flag, or similar.
> 

I looked into this solution. I've tested it for all my failure cases and
went good.

> Note that dev->pm_domain is expected to be cleared by ->detach(), so
> this should not cause the domain to be detached twice in a row from
> the same device, but that needs to be double-checked.

The genpd_dev_pm_detach() calls genpd_remove_device() ->
dev_pm_domain_set(dev, NULL) which sets the dev->pm_domain = NULL. I can't
find any other detach function in the current code base.

The code I've tested for this solution is this one:

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index b526e0e0f52d..5e9750d007b4 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,8 +553,11 @@ static void device_unbind_cleanup(struct device *dev)
        dev->dma_range_map = NULL;
        device_set_driver(dev, NULL);
        dev_set_drvdata(dev, NULL);
-       if (dev->pm_domain && dev->pm_domain->dismiss)
-               dev->pm_domain->dismiss(dev);
+       if (dev->pm_domain) {
+               if (dev->pm_domain->dismiss)
+                       dev->pm_domain->dismiss(dev);
+               dev_pm_domain_detach(dev, dev->pm_domain->detach_power_off);
+       }
        pm_runtime_reinit(dev);
        dev_pm_set_driver_flags(dev, 0);
 }
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)
diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c
index 781968a128ff..4bd1e3c7f401 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->pm_domain->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..12e97e09e85c 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -748,6 +748,7 @@ struct dev_pm_domain {
        void (*sync)(struct device *dev);
        void (*dismiss)(struct device *dev);
        int (*set_performance_state)(struct device *dev, unsigned int state);
+       bool detach_power_off;
 };

Rafael, Ulf, Dmitry, Jonathan, all,

Could you please let me know how do you consider this approach?

Thank you,
Claudiu

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

[Rafael J. Wysocki]

On Fri, Jun 13, 2025 at 9:39 AM Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
>
> Hi, Rafael,
>
> On 09.06.2025 22:59, Rafael J. Wysocki wrote:
> > On Sat, Jun 7, 2025 at 3:06 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >>
> >> On Fri, 6 Jun 2025 22:01:52 +0200
> >> "Rafael J. Wysocki" <rafael@kernel.org> wrote:
> >>
> >> Hi Rafael,
> >>
> >>> On Fri, Jun 6, 2025 at 8:55 PM Dmitry Torokhov
> >>> <dmitry.torokhov@gmail.com> wrote:
> >>>>
> >>>> On Fri, Jun 06, 2025 at 06:00:34PM +0200, Rafael J. Wysocki wrote:
> >>>>> On Fri, Jun 6, 2025 at 1:18 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.
> >>>>>
> >>>>> Don't use devm_pm_runtime_enable() then.
> >>>>
> >>>> What about other devm_ APIs? Are you suggesting that platform drivers
> >>>> should not be using devm_clk*(), devm_regulator_*(),
> >>>> devm_request_*_irq() and devm_add_action_or_reset()? Because again,
> >>>> dev_pm_domain_detach() that is called by platform bus_remove() may shut
> >>>> off the device too early, before cleanup code has a chance to execute
> >>>> proper cleanup.
> >>>>
> >>>> The issue is not limited to runtime PM.
> >>>>
> >>>>>
> >>>>>> 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.
> >>>>>
> >>>>> dev_pm_domain_attach() is not part driver API and I'm not convinced at
> >>>>
> >>>> Is the concern that devm_pm_domain_attach() will be [ab]used by drivers?
> >>>
> >>> Yes, among other things.
> >>
> >> Maybe naming could make abuse at least obvious to spot? e.g.
> >> pm_domain_attach_with_devm_release()
> >
> > If I'm not mistaken, it is not even necessary to use devres for this.
> >
> > You might as well add a dev_pm_domain_detach() call to
> > device_unbind_cleanup() after devres_release_all().  There is a slight
> > complication related to the second argument of it, but I suppose that
> > this can be determined at the attach time and stored in a new device
> > PM flag, or similar.
> >
>
> I looked into this solution. I've tested it for all my failure cases and
> went good.

OK

> > Note that dev->pm_domain is expected to be cleared by ->detach(), so
> > this should not cause the domain to be detached twice in a row from
> > the same device, but that needs to be double-checked.
>
> The genpd_dev_pm_detach() calls genpd_remove_device() ->
> dev_pm_domain_set(dev, NULL) which sets the dev->pm_domain = NULL. I can't
> find any other detach function in the current code base.

There is also acpi_dev_pm_detach() which can be somewhat hard to find,
but it calls dev_pm_domain_set(dev, NULL) either.

> The code I've tested for this solution is this one:
>
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index b526e0e0f52d..5e9750d007b4 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,8 +553,11 @@ static void device_unbind_cleanup(struct device *dev)
>         dev->dma_range_map = NULL;
>         device_set_driver(dev, NULL);
>         dev_set_drvdata(dev, NULL);
> -       if (dev->pm_domain && dev->pm_domain->dismiss)
> -               dev->pm_domain->dismiss(dev);
> +       if (dev->pm_domain) {
> +               if (dev->pm_domain->dismiss)
> +                       dev->pm_domain->dismiss(dev);
> +               dev_pm_domain_detach(dev, dev->pm_domain->detach_power_off);

I would do the "detach" before the "dismiss" to retain the current ordering.

Also it is interesting that you ended up calling them both in one
place.  It kind of indicates that the PM domains attached via
dev_pm_domain_attach() should be attached earlier and just use the
->activate() and ->dismiss() callbacks.

> +       }
>         pm_runtime_reinit(dev);
>         dev_pm_set_driver_flags(dev, 0);
>  }
> 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)
> diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c
> index 781968a128ff..4bd1e3c7f401 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->pm_domain->detach_power_off = power_on;

This will not work for acpi_general_pm_domain because it is shared by
all of its users.

It is likely to not work for shared PM domains in general.

I would put the new flag into struct dev_pm_info.

> +
>         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..12e97e09e85c 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -748,6 +748,7 @@ struct dev_pm_domain {
>         void (*sync)(struct device *dev);
>         void (*dismiss)(struct device *dev);
>         int (*set_performance_state)(struct device *dev, unsigned int state);
> +       bool detach_power_off;
>  };
>
> Rafael, Ulf, Dmitry, Jonathan, all,
>
> Could you please let me know how do you consider this approach?

Please see my comments above.

Thanks!

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

[Claudiu Beznea]

Hi, Rafael,

On 13.06.2025 13:02, Rafael J. Wysocki wrote:
> On Fri, Jun 13, 2025 at 9:39 AM Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
>>
>> Hi, Rafael,
>>
>> On 09.06.2025 22:59, Rafael J. Wysocki wrote:
>>> On Sat, Jun 7, 2025 at 3:06 PM Jonathan Cameron <jic23@kernel.org> wrote:
>>>>
>>>> On Fri, 6 Jun 2025 22:01:52 +0200
>>>> "Rafael J. Wysocki" <rafael@kernel.org> wrote:
>>>>
>>>> Hi Rafael,
>>>>
>>>>> On Fri, Jun 6, 2025 at 8:55 PM Dmitry Torokhov
>>>>> <dmitry.torokhov@gmail.com> wrote:
>>>>>>
>>>>>> On Fri, Jun 06, 2025 at 06:00:34PM +0200, Rafael J. Wysocki wrote:
>>>>>>> On Fri, Jun 6, 2025 at 1:18 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.
>>>>>>>
>>>>>>> Don't use devm_pm_runtime_enable() then.
>>>>>>
>>>>>> What about other devm_ APIs? Are you suggesting that platform drivers
>>>>>> should not be using devm_clk*(), devm_regulator_*(),
>>>>>> devm_request_*_irq() and devm_add_action_or_reset()? Because again,
>>>>>> dev_pm_domain_detach() that is called by platform bus_remove() may shut
>>>>>> off the device too early, before cleanup code has a chance to execute
>>>>>> proper cleanup.
>>>>>>
>>>>>> The issue is not limited to runtime PM.
>>>>>>
>>>>>>>
>>>>>>>> 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.
>>>>>>>
>>>>>>> dev_pm_domain_attach() is not part driver API and I'm not convinced at
>>>>>>
>>>>>> Is the concern that devm_pm_domain_attach() will be [ab]used by drivers?
>>>>>
>>>>> Yes, among other things.
>>>>
>>>> Maybe naming could make abuse at least obvious to spot? e.g.
>>>> pm_domain_attach_with_devm_release()
>>>
>>> If I'm not mistaken, it is not even necessary to use devres for this.
>>>
>>> You might as well add a dev_pm_domain_detach() call to
>>> device_unbind_cleanup() after devres_release_all().  There is a slight
>>> complication related to the second argument of it, but I suppose that
>>> this can be determined at the attach time and stored in a new device
>>> PM flag, or similar.
>>>
>>
>> I looked into this solution. I've tested it for all my failure cases and
>> went good.
> 
> OK
> 
>>> Note that dev->pm_domain is expected to be cleared by ->detach(), so
>>> this should not cause the domain to be detached twice in a row from
>>> the same device, but that needs to be double-checked.
>>
>> The genpd_dev_pm_detach() calls genpd_remove_device() ->
>> dev_pm_domain_set(dev, NULL) which sets the dev->pm_domain = NULL. I can't
>> find any other detach function in the current code base.
> 
> There is also acpi_dev_pm_detach() which can be somewhat hard to find,
> but it calls dev_pm_domain_set(dev, NULL) either.

Thank you for the pointer.

> 
>> The code I've tested for this solution is this one:
>>
>> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
>> index b526e0e0f52d..5e9750d007b4 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,8 +553,11 @@ static void device_unbind_cleanup(struct device *dev)
>>         dev->dma_range_map = NULL;
>>         device_set_driver(dev, NULL);
>>         dev_set_drvdata(dev, NULL);
>> -       if (dev->pm_domain && dev->pm_domain->dismiss)
>> -               dev->pm_domain->dismiss(dev);
>> +       if (dev->pm_domain) {
>> +               if (dev->pm_domain->dismiss)
>> +                       dev->pm_domain->dismiss(dev);
>> +               dev_pm_domain_detach(dev, dev->pm_domain->detach_power_off);
> 
> I would do the "detach" before the "dismiss" to retain the current ordering.

OK.

> 
> Also it is interesting that you ended up calling them both in one
> place.  It kind of indicates that the PM domains attached via
> dev_pm_domain_attach() should be attached earlier and just use the
> ->activate() and ->dismiss() callbacks.
> 
>> +       }
>>         pm_runtime_reinit(dev);
>>         dev_pm_set_driver_flags(dev, 0);
>>  }
>> 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)
>> diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c
>> index 781968a128ff..4bd1e3c7f401 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->pm_domain->detach_power_off = power_on;
> 
> This will not work for acpi_general_pm_domain because it is shared by
> all of its users.
> 
> It is likely to not work for shared PM domains in general.
> 
> I would put the new flag into struct dev_pm_info.

OK, I'll do it.

Thank you for your input,
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..12e97e09e85c 100644
>> --- a/include/linux/pm.h
>> +++ b/include/linux/pm.h
>> @@ -748,6 +748,7 @@ struct dev_pm_domain {
>>         void (*sync)(struct device *dev);
>>         void (*dismiss)(struct device *dev);
>>         int (*set_performance_state)(struct device *dev, unsigned int state);
>> +       bool detach_power_off;
>>  };
>>
>> Rafael, Ulf, Dmitry, Jonathan, all,
>>
>> Could you please let me know how do you consider this approach?
> 
> Please see my comments above.
> 
> Thanks!

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

[Claudiu Beznea]

Hi, Rafael,

On 13.06.2025 13:02, Rafael J. Wysocki wrote:
> On Fri, Jun 13, 2025 at 9:39 AM Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
>>
>> Hi, Rafael,
>>
>> On 09.06.2025 22:59, Rafael J. Wysocki wrote:
>>> On Sat, Jun 7, 2025 at 3:06 PM Jonathan Cameron <jic23@kernel.org> wrote:
>>>>
>>>> On Fri, 6 Jun 2025 22:01:52 +0200
>>>> "Rafael J. Wysocki" <rafael@kernel.org> wrote:
>>>>
>>>> Hi Rafael,
>>>>
>>>>> On Fri, Jun 6, 2025 at 8:55 PM Dmitry Torokhov
>>>>> <dmitry.torokhov@gmail.com> wrote:
>>>>>>
>>>>>> On Fri, Jun 06, 2025 at 06:00:34PM +0200, Rafael J. Wysocki wrote:
>>>>>>> On Fri, Jun 6, 2025 at 1:18 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.
>>>>>>>
>>>>>>> Don't use devm_pm_runtime_enable() then.
>>>>>>
>>>>>> What about other devm_ APIs? Are you suggesting that platform drivers
>>>>>> should not be using devm_clk*(), devm_regulator_*(),
>>>>>> devm_request_*_irq() and devm_add_action_or_reset()? Because again,
>>>>>> dev_pm_domain_detach() that is called by platform bus_remove() may shut
>>>>>> off the device too early, before cleanup code has a chance to execute
>>>>>> proper cleanup.
>>>>>>
>>>>>> The issue is not limited to runtime PM.
>>>>>>
>>>>>>>
>>>>>>>> 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.
>>>>>>>
>>>>>>> dev_pm_domain_attach() is not part driver API and I'm not convinced at
>>>>>>
>>>>>> Is the concern that devm_pm_domain_attach() will be [ab]used by drivers?
>>>>>
>>>>> Yes, among other things.
>>>>
>>>> Maybe naming could make abuse at least obvious to spot? e.g.
>>>> pm_domain_attach_with_devm_release()
>>>
>>> If I'm not mistaken, it is not even necessary to use devres for this.
>>>
>>> You might as well add a dev_pm_domain_detach() call to
>>> device_unbind_cleanup() after devres_release_all().  There is a slight
>>> complication related to the second argument of it, but I suppose that
>>> this can be determined at the attach time and stored in a new device
>>> PM flag, or similar.
>>>
>>
>> I looked into this solution. I've tested it for all my failure cases and
>> went good.
> 
> OK
> 
>>> Note that dev->pm_domain is expected to be cleared by ->detach(), so
>>> this should not cause the domain to be detached twice in a row from
>>> the same device, but that needs to be double-checked.
>>
>> The genpd_dev_pm_detach() calls genpd_remove_device() ->
>> dev_pm_domain_set(dev, NULL) which sets the dev->pm_domain = NULL. I can't
>> find any other detach function in the current code base.
> 
> There is also acpi_dev_pm_detach() which can be somewhat hard to find,
> but it calls dev_pm_domain_set(dev, NULL) either.
> 
>> The code I've tested for this solution is this one:
>>
>> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
>> index b526e0e0f52d..5e9750d007b4 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,8 +553,11 @@ static void device_unbind_cleanup(struct device *dev)
>>         dev->dma_range_map = NULL;
>>         device_set_driver(dev, NULL);
>>         dev_set_drvdata(dev, NULL);
>> -       if (dev->pm_domain && dev->pm_domain->dismiss)
>> -               dev->pm_domain->dismiss(dev);
>> +       if (dev->pm_domain) {
>> +               if (dev->pm_domain->dismiss)
>> +                       dev->pm_domain->dismiss(dev);
>> +               dev_pm_domain_detach(dev, dev->pm_domain->detach_power_off);
> 
> I would do the "detach" before the "dismiss" to retain the current ordering.

I applied on my local development branch all your suggestions except this
one because genpd_dev_pm_detach() as well as acpi_dev_pm_detach() set
dev->pm_domain = NULL.

Due to this I would call first ->dismiss() then ->detach(), as initially
proposed. Please let me know if you consider it otherwise.

Thank you,
Claudiu

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

[Rafael J. Wysocki]

On Mon, Jun 16, 2025 at 11:37 AM Claudiu Beznea
<claudiu.beznea@tuxon.dev> wrote:
>
> Hi, Rafael,
>
> On 13.06.2025 13:02, Rafael J. Wysocki wrote:
> > On Fri, Jun 13, 2025 at 9:39 AM Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> >>
> >> Hi, Rafael,
> >>
> >> On 09.06.2025 22:59, Rafael J. Wysocki wrote:
> >>> On Sat, Jun 7, 2025 at 3:06 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >>>>
> >>>> On Fri, 6 Jun 2025 22:01:52 +0200
> >>>> "Rafael J. Wysocki" <rafael@kernel.org> wrote:
> >>>>
> >>>> Hi Rafael,
> >>>>
> >>>>> On Fri, Jun 6, 2025 at 8:55 PM Dmitry Torokhov
> >>>>> <dmitry.torokhov@gmail.com> wrote:
> >>>>>>
> >>>>>> On Fri, Jun 06, 2025 at 06:00:34PM +0200, Rafael J. Wysocki wrote:
> >>>>>>> On Fri, Jun 6, 2025 at 1:18 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.
> >>>>>>>
> >>>>>>> Don't use devm_pm_runtime_enable() then.
> >>>>>>
> >>>>>> What about other devm_ APIs? Are you suggesting that platform drivers
> >>>>>> should not be using devm_clk*(), devm_regulator_*(),
> >>>>>> devm_request_*_irq() and devm_add_action_or_reset()? Because again,
> >>>>>> dev_pm_domain_detach() that is called by platform bus_remove() may shut
> >>>>>> off the device too early, before cleanup code has a chance to execute
> >>>>>> proper cleanup.
> >>>>>>
> >>>>>> The issue is not limited to runtime PM.
> >>>>>>
> >>>>>>>
> >>>>>>>> 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.
> >>>>>>>
> >>>>>>> dev_pm_domain_attach() is not part driver API and I'm not convinced at
> >>>>>>
> >>>>>> Is the concern that devm_pm_domain_attach() will be [ab]used by drivers?
> >>>>>
> >>>>> Yes, among other things.
> >>>>
> >>>> Maybe naming could make abuse at least obvious to spot? e.g.
> >>>> pm_domain_attach_with_devm_release()
> >>>
> >>> If I'm not mistaken, it is not even necessary to use devres for this.
> >>>
> >>> You might as well add a dev_pm_domain_detach() call to
> >>> device_unbind_cleanup() after devres_release_all().  There is a slight
> >>> complication related to the second argument of it, but I suppose that
> >>> this can be determined at the attach time and stored in a new device
> >>> PM flag, or similar.
> >>>
> >>
> >> I looked into this solution. I've tested it for all my failure cases and
> >> went good.
> >
> > OK
> >
> >>> Note that dev->pm_domain is expected to be cleared by ->detach(), so
> >>> this should not cause the domain to be detached twice in a row from
> >>> the same device, but that needs to be double-checked.
> >>
> >> The genpd_dev_pm_detach() calls genpd_remove_device() ->
> >> dev_pm_domain_set(dev, NULL) which sets the dev->pm_domain = NULL. I can't
> >> find any other detach function in the current code base.
> >
> > There is also acpi_dev_pm_detach() which can be somewhat hard to find,
> > but it calls dev_pm_domain_set(dev, NULL) either.
> >
> >> The code I've tested for this solution is this one:
> >>
> >> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> >> index b526e0e0f52d..5e9750d007b4 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,8 +553,11 @@ static void device_unbind_cleanup(struct device *dev)
> >>         dev->dma_range_map = NULL;
> >>         device_set_driver(dev, NULL);
> >>         dev_set_drvdata(dev, NULL);
> >> -       if (dev->pm_domain && dev->pm_domain->dismiss)
> >> -               dev->pm_domain->dismiss(dev);
> >> +       if (dev->pm_domain) {
> >> +               if (dev->pm_domain->dismiss)
> >> +                       dev->pm_domain->dismiss(dev);
> >> +               dev_pm_domain_detach(dev, dev->pm_domain->detach_power_off);
> >
> > I would do the "detach" before the "dismiss" to retain the current ordering.
>
> I applied on my local development branch all your suggestions except this
> one because genpd_dev_pm_detach() as well as acpi_dev_pm_detach() set
> dev->pm_domain = NULL.
>
> Due to this I would call first ->dismiss() then ->detach(), as initially
> proposed. Please let me know if you consider it otherwise.

This is a matter of adding one more dev->pm_domain check AFAICS, but OK.

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

[Claudiu Beznea]

On 16.06.2025 14:18, Rafael J. Wysocki wrote:
> On Mon, Jun 16, 2025 at 11:37 AM Claudiu Beznea
> <claudiu.beznea@tuxon.dev> wrote:
>>
>> Hi, Rafael,
>>
>> On 13.06.2025 13:02, Rafael J. Wysocki wrote:
>>> On Fri, Jun 13, 2025 at 9:39 AM Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
>>>>
>>>> Hi, Rafael,
>>>>
>>>> On 09.06.2025 22:59, Rafael J. Wysocki wrote:
>>>>> On Sat, Jun 7, 2025 at 3:06 PM Jonathan Cameron <jic23@kernel.org> wrote:
>>>>>>
>>>>>> On Fri, 6 Jun 2025 22:01:52 +0200
>>>>>> "Rafael J. Wysocki" <rafael@kernel.org> wrote:
>>>>>>
>>>>>> Hi Rafael,
>>>>>>
>>>>>>> On Fri, Jun 6, 2025 at 8:55 PM Dmitry Torokhov
>>>>>>> <dmitry.torokhov@gmail.com> wrote:
>>>>>>>>
>>>>>>>> On Fri, Jun 06, 2025 at 06:00:34PM +0200, Rafael J. Wysocki wrote:
>>>>>>>>> On Fri, Jun 6, 2025 at 1:18 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.
>>>>>>>>>
>>>>>>>>> Don't use devm_pm_runtime_enable() then.
>>>>>>>>
>>>>>>>> What about other devm_ APIs? Are you suggesting that platform drivers
>>>>>>>> should not be using devm_clk*(), devm_regulator_*(),
>>>>>>>> devm_request_*_irq() and devm_add_action_or_reset()? Because again,
>>>>>>>> dev_pm_domain_detach() that is called by platform bus_remove() may shut
>>>>>>>> off the device too early, before cleanup code has a chance to execute
>>>>>>>> proper cleanup.
>>>>>>>>
>>>>>>>> The issue is not limited to runtime PM.
>>>>>>>>
>>>>>>>>>
>>>>>>>>>> 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.
>>>>>>>>>
>>>>>>>>> dev_pm_domain_attach() is not part driver API and I'm not convinced at
>>>>>>>>
>>>>>>>> Is the concern that devm_pm_domain_attach() will be [ab]used by drivers?
>>>>>>>
>>>>>>> Yes, among other things.
>>>>>>
>>>>>> Maybe naming could make abuse at least obvious to spot? e.g.
>>>>>> pm_domain_attach_with_devm_release()
>>>>>
>>>>> If I'm not mistaken, it is not even necessary to use devres for this.
>>>>>
>>>>> You might as well add a dev_pm_domain_detach() call to
>>>>> device_unbind_cleanup() after devres_release_all().  There is a slight
>>>>> complication related to the second argument of it, but I suppose that
>>>>> this can be determined at the attach time and stored in a new device
>>>>> PM flag, or similar.
>>>>>
>>>>
>>>> I looked into this solution. I've tested it for all my failure cases and
>>>> went good.
>>>
>>> OK
>>>
>>>>> Note that dev->pm_domain is expected to be cleared by ->detach(), so
>>>>> this should not cause the domain to be detached twice in a row from
>>>>> the same device, but that needs to be double-checked.
>>>>
>>>> The genpd_dev_pm_detach() calls genpd_remove_device() ->
>>>> dev_pm_domain_set(dev, NULL) which sets the dev->pm_domain = NULL. I can't
>>>> find any other detach function in the current code base.
>>>
>>> There is also acpi_dev_pm_detach() which can be somewhat hard to find,
>>> but it calls dev_pm_domain_set(dev, NULL) either.
>>>
>>>> The code I've tested for this solution is this one:
>>>>
>>>> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
>>>> index b526e0e0f52d..5e9750d007b4 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,8 +553,11 @@ static void device_unbind_cleanup(struct device *dev)
>>>>         dev->dma_range_map = NULL;
>>>>         device_set_driver(dev, NULL);
>>>>         dev_set_drvdata(dev, NULL);
>>>> -       if (dev->pm_domain && dev->pm_domain->dismiss)
>>>> -               dev->pm_domain->dismiss(dev);
>>>> +       if (dev->pm_domain) {
>>>> +               if (dev->pm_domain->dismiss)
>>>> +                       dev->pm_domain->dismiss(dev);
>>>> +               dev_pm_domain_detach(dev, dev->pm_domain->detach_power_off);
>>>
>>> I would do the "detach" before the "dismiss" to retain the current ordering.
>>
>> I applied on my local development branch all your suggestions except this
>> one because genpd_dev_pm_detach() as well as acpi_dev_pm_detach() set
>> dev->pm_domain = NULL.
>>
>> Due to this I would call first ->dismiss() then ->detach(), as initially
>> proposed. Please let me know if you consider it otherwise.
> 
> This is a matter of adding one more dev->pm_domain check AFAICS, but OK.

I don't know all the subtleties around this, my concern with adding one
more check on dev->pm_domain was that the
dev->pm_domain->dismiss() will never be called if the ->detach() function
will be called before ->dismiss() and it will set dev->pm_domain = NULL (as
it does today (though genpd_dev_pm_detach() and acpi_dev_pm_detach())).

For platform drivers that used to call dev_pm_domain_detach() in platform
bus remove function, if I'm not wrong, the dev->pm_domain->dismiss() was
never called previously. If that is a valid scenario, the code proposed in
this thread will change the behavior for devices that have ->dismiss()
implemented.

Thank you,
Claudiu

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

[Rafael J. Wysocki]

On Mon, Jun 16, 2025 at 1:37 PM Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
>
>
>
> On 16.06.2025 14:18, Rafael J. Wysocki wrote:
> > On Mon, Jun 16, 2025 at 11:37 AM Claudiu Beznea
> > <claudiu.beznea@tuxon.dev> wrote:
> >>
> >> Hi, Rafael,
> >>
> >> On 13.06.2025 13:02, Rafael J. Wysocki wrote:
> >>> On Fri, Jun 13, 2025 at 9:39 AM Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> >>>>
> >>>> Hi, Rafael,
> >>>>
> >>>> On 09.06.2025 22:59, Rafael J. Wysocki wrote:
> >>>>> On Sat, Jun 7, 2025 at 3:06 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >>>>>>
> >>>>>> On Fri, 6 Jun 2025 22:01:52 +0200
> >>>>>> "Rafael J. Wysocki" <rafael@kernel.org> wrote:
> >>>>>>
> >>>>>> Hi Rafael,
> >>>>>>
> >>>>>>> On Fri, Jun 6, 2025 at 8:55 PM Dmitry Torokhov
> >>>>>>> <dmitry.torokhov@gmail.com> wrote:
> >>>>>>>>
> >>>>>>>> On Fri, Jun 06, 2025 at 06:00:34PM +0200, Rafael J. Wysocki wrote:
> >>>>>>>>> On Fri, Jun 6, 2025 at 1:18 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.
> >>>>>>>>>
> >>>>>>>>> Don't use devm_pm_runtime_enable() then.
> >>>>>>>>
> >>>>>>>> What about other devm_ APIs? Are you suggesting that platform drivers
> >>>>>>>> should not be using devm_clk*(), devm_regulator_*(),
> >>>>>>>> devm_request_*_irq() and devm_add_action_or_reset()? Because again,
> >>>>>>>> dev_pm_domain_detach() that is called by platform bus_remove() may shut
> >>>>>>>> off the device too early, before cleanup code has a chance to execute
> >>>>>>>> proper cleanup.
> >>>>>>>>
> >>>>>>>> The issue is not limited to runtime PM.
> >>>>>>>>
> >>>>>>>>>
> >>>>>>>>>> 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.
> >>>>>>>>>
> >>>>>>>>> dev_pm_domain_attach() is not part driver API and I'm not convinced at
> >>>>>>>>
> >>>>>>>> Is the concern that devm_pm_domain_attach() will be [ab]used by drivers?
> >>>>>>>
> >>>>>>> Yes, among other things.
> >>>>>>
> >>>>>> Maybe naming could make abuse at least obvious to spot? e.g.
> >>>>>> pm_domain_attach_with_devm_release()
> >>>>>
> >>>>> If I'm not mistaken, it is not even necessary to use devres for this.
> >>>>>
> >>>>> You might as well add a dev_pm_domain_detach() call to
> >>>>> device_unbind_cleanup() after devres_release_all().  There is a slight
> >>>>> complication related to the second argument of it, but I suppose that
> >>>>> this can be determined at the attach time and stored in a new device
> >>>>> PM flag, or similar.
> >>>>>
> >>>>
> >>>> I looked into this solution. I've tested it for all my failure cases and
> >>>> went good.
> >>>
> >>> OK
> >>>
> >>>>> Note that dev->pm_domain is expected to be cleared by ->detach(), so
> >>>>> this should not cause the domain to be detached twice in a row from
> >>>>> the same device, but that needs to be double-checked.
> >>>>
> >>>> The genpd_dev_pm_detach() calls genpd_remove_device() ->
> >>>> dev_pm_domain_set(dev, NULL) which sets the dev->pm_domain = NULL. I can't
> >>>> find any other detach function in the current code base.
> >>>
> >>> There is also acpi_dev_pm_detach() which can be somewhat hard to find,
> >>> but it calls dev_pm_domain_set(dev, NULL) either.
> >>>
> >>>> The code I've tested for this solution is this one:
> >>>>
> >>>> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> >>>> index b526e0e0f52d..5e9750d007b4 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,8 +553,11 @@ static void device_unbind_cleanup(struct device *dev)
> >>>>         dev->dma_range_map = NULL;
> >>>>         device_set_driver(dev, NULL);
> >>>>         dev_set_drvdata(dev, NULL);
> >>>> -       if (dev->pm_domain && dev->pm_domain->dismiss)
> >>>> -               dev->pm_domain->dismiss(dev);
> >>>> +       if (dev->pm_domain) {
> >>>> +               if (dev->pm_domain->dismiss)
> >>>> +                       dev->pm_domain->dismiss(dev);
> >>>> +               dev_pm_domain_detach(dev, dev->pm_domain->detach_power_off);
> >>>
> >>> I would do the "detach" before the "dismiss" to retain the current ordering.
> >>
> >> I applied on my local development branch all your suggestions except this
> >> one because genpd_dev_pm_detach() as well as acpi_dev_pm_detach() set
> >> dev->pm_domain = NULL.
> >>
> >> Due to this I would call first ->dismiss() then ->detach(), as initially
> >> proposed. Please let me know if you consider it otherwise.
> >
> > This is a matter of adding one more dev->pm_domain check AFAICS, but OK.
>
> I don't know all the subtleties around this, my concern with adding one
> more check on dev->pm_domain was that the
> dev->pm_domain->dismiss() will never be called if the ->detach() function
> will be called before ->dismiss() and it will set dev->pm_domain = NULL (as
> it does today (though genpd_dev_pm_detach() and acpi_dev_pm_detach())).
>
> For platform drivers that used to call dev_pm_domain_detach() in platform
> bus remove function, if I'm not wrong, the dev->pm_domain->dismiss() was
> never called previously. If that is a valid scenario, the code proposed in
> this thread will change the behavior for devices that have ->dismiss()
> implemented.

->dismiss() and ->detach() are supposed to be mutually exclusive, so
this should not be a problem either way and in practice so far the
only user of ->dismiss() has been acpi_lpss_pm_domain which doesn't do
->detach() at all.

So this is your call.

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

[Ulf Hansson]

On Mon, 16 Jun 2025 at 13:47, Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Mon, Jun 16, 2025 at 1:37 PM Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> >
> >
> >
> > On 16.06.2025 14:18, Rafael J. Wysocki wrote:
> > > On Mon, Jun 16, 2025 at 11:37 AM Claudiu Beznea
> > > <claudiu.beznea@tuxon.dev> wrote:
> > >>
> > >> Hi, Rafael,
> > >>
> > >> On 13.06.2025 13:02, Rafael J. Wysocki wrote:
> > >>> On Fri, Jun 13, 2025 at 9:39 AM Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> > >>>>
> > >>>> Hi, Rafael,
> > >>>>
> > >>>> On 09.06.2025 22:59, Rafael J. Wysocki wrote:
> > >>>>> On Sat, Jun 7, 2025 at 3:06 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > >>>>>>
> > >>>>>> On Fri, 6 Jun 2025 22:01:52 +0200
> > >>>>>> "Rafael J. Wysocki" <rafael@kernel.org> wrote:
> > >>>>>>
> > >>>>>> Hi Rafael,
> > >>>>>>
> > >>>>>>> On Fri, Jun 6, 2025 at 8:55 PM Dmitry Torokhov
> > >>>>>>> <dmitry.torokhov@gmail.com> wrote:
> > >>>>>>>>
> > >>>>>>>> On Fri, Jun 06, 2025 at 06:00:34PM +0200, Rafael J. Wysocki wrote:
> > >>>>>>>>> On Fri, Jun 6, 2025 at 1:18 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.
> > >>>>>>>>>
> > >>>>>>>>> Don't use devm_pm_runtime_enable() then.
> > >>>>>>>>
> > >>>>>>>> What about other devm_ APIs? Are you suggesting that platform drivers
> > >>>>>>>> should not be using devm_clk*(), devm_regulator_*(),
> > >>>>>>>> devm_request_*_irq() and devm_add_action_or_reset()? Because again,
> > >>>>>>>> dev_pm_domain_detach() that is called by platform bus_remove() may shut
> > >>>>>>>> off the device too early, before cleanup code has a chance to execute
> > >>>>>>>> proper cleanup.
> > >>>>>>>>
> > >>>>>>>> The issue is not limited to runtime PM.
> > >>>>>>>>
> > >>>>>>>>>
> > >>>>>>>>>> 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.
> > >>>>>>>>>
> > >>>>>>>>> dev_pm_domain_attach() is not part driver API and I'm not convinced at
> > >>>>>>>>
> > >>>>>>>> Is the concern that devm_pm_domain_attach() will be [ab]used by drivers?
> > >>>>>>>
> > >>>>>>> Yes, among other things.
> > >>>>>>
> > >>>>>> Maybe naming could make abuse at least obvious to spot? e.g.
> > >>>>>> pm_domain_attach_with_devm_release()
> > >>>>>
> > >>>>> If I'm not mistaken, it is not even necessary to use devres for this.
> > >>>>>
> > >>>>> You might as well add a dev_pm_domain_detach() call to
> > >>>>> device_unbind_cleanup() after devres_release_all().  There is a slight
> > >>>>> complication related to the second argument of it, but I suppose that
> > >>>>> this can be determined at the attach time and stored in a new device
> > >>>>> PM flag, or similar.
> > >>>>>
> > >>>>
> > >>>> I looked into this solution. I've tested it for all my failure cases and
> > >>>> went good.
> > >>>
> > >>> OK
> > >>>
> > >>>>> Note that dev->pm_domain is expected to be cleared by ->detach(), so
> > >>>>> this should not cause the domain to be detached twice in a row from
> > >>>>> the same device, but that needs to be double-checked.
> > >>>>
> > >>>> The genpd_dev_pm_detach() calls genpd_remove_device() ->
> > >>>> dev_pm_domain_set(dev, NULL) which sets the dev->pm_domain = NULL. I can't
> > >>>> find any other detach function in the current code base.
> > >>>
> > >>> There is also acpi_dev_pm_detach() which can be somewhat hard to find,
> > >>> but it calls dev_pm_domain_set(dev, NULL) either.
> > >>>
> > >>>> The code I've tested for this solution is this one:
> > >>>>
> > >>>> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> > >>>> index b526e0e0f52d..5e9750d007b4 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,8 +553,11 @@ static void device_unbind_cleanup(struct device *dev)
> > >>>>         dev->dma_range_map = NULL;
> > >>>>         device_set_driver(dev, NULL);
> > >>>>         dev_set_drvdata(dev, NULL);
> > >>>> -       if (dev->pm_domain && dev->pm_domain->dismiss)
> > >>>> -               dev->pm_domain->dismiss(dev);
> > >>>> +       if (dev->pm_domain) {
> > >>>> +               if (dev->pm_domain->dismiss)
> > >>>> +                       dev->pm_domain->dismiss(dev);
> > >>>> +               dev_pm_domain_detach(dev, dev->pm_domain->detach_power_off);
> > >>>
> > >>> I would do the "detach" before the "dismiss" to retain the current ordering.
> > >>
> > >> I applied on my local development branch all your suggestions except this
> > >> one because genpd_dev_pm_detach() as well as acpi_dev_pm_detach() set
> > >> dev->pm_domain = NULL.
> > >>
> > >> Due to this I would call first ->dismiss() then ->detach(), as initially
> > >> proposed. Please let me know if you consider it otherwise.
> > >
> > > This is a matter of adding one more dev->pm_domain check AFAICS, but OK.
> >
> > I don't know all the subtleties around this, my concern with adding one
> > more check on dev->pm_domain was that the
> > dev->pm_domain->dismiss() will never be called if the ->detach() function
> > will be called before ->dismiss() and it will set dev->pm_domain = NULL (as
> > it does today (though genpd_dev_pm_detach() and acpi_dev_pm_detach())).
> >
> > For platform drivers that used to call dev_pm_domain_detach() in platform
> > bus remove function, if I'm not wrong, the dev->pm_domain->dismiss() was
> > never called previously. If that is a valid scenario, the code proposed in
> > this thread will change the behavior for devices that have ->dismiss()
> > implemented.
>
> ->dismiss() and ->detach() are supposed to be mutually exclusive, so
> this should not be a problem either way and in practice so far the
> only user of ->dismiss() has been acpi_lpss_pm_domain which doesn't do
> ->detach() at all.

May I ask if you know if there remains any real good reasons to keep
the ->dismiss() callback around?

Can't acpi_lpss_pm_domain() convert to use the ->detach() callback
instead? Just thinking that it would be easier, but maybe it doesn't
work.

Kind regards
Uffe

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

[Rafael J. Wysocki]

On Thu, Jun 19, 2025 at 2:21 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Mon, 16 Jun 2025 at 13:47, Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Mon, Jun 16, 2025 at 1:37 PM Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> > >
> > >
> > >
> > > On 16.06.2025 14:18, Rafael J. Wysocki wrote:
> > > > On Mon, Jun 16, 2025 at 11:37 AM Claudiu Beznea
> > > > <claudiu.beznea@tuxon.dev> wrote:
> > > >>
> > > >> Hi, Rafael,
> > > >>
> > > >> On 13.06.2025 13:02, Rafael J. Wysocki wrote:
> > > >>> On Fri, Jun 13, 2025 at 9:39 AM Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> > > >>>>
> > > >>>> Hi, Rafael,
> > > >>>>
> > > >>>> On 09.06.2025 22:59, Rafael J. Wysocki wrote:
> > > >>>>> On Sat, Jun 7, 2025 at 3:06 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > > >>>>>>
> > > >>>>>> On Fri, 6 Jun 2025 22:01:52 +0200
> > > >>>>>> "Rafael J. Wysocki" <rafael@kernel.org> wrote:
> > > >>>>>>
> > > >>>>>> Hi Rafael,
> > > >>>>>>
> > > >>>>>>> On Fri, Jun 6, 2025 at 8:55 PM Dmitry Torokhov
> > > >>>>>>> <dmitry.torokhov@gmail.com> wrote:
> > > >>>>>>>>
> > > >>>>>>>> On Fri, Jun 06, 2025 at 06:00:34PM +0200, Rafael J. Wysocki wrote:
> > > >>>>>>>>> On Fri, Jun 6, 2025 at 1:18 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.
> > > >>>>>>>>>
> > > >>>>>>>>> Don't use devm_pm_runtime_enable() then.
> > > >>>>>>>>
> > > >>>>>>>> What about other devm_ APIs? Are you suggesting that platform drivers
> > > >>>>>>>> should not be using devm_clk*(), devm_regulator_*(),
> > > >>>>>>>> devm_request_*_irq() and devm_add_action_or_reset()? Because again,
> > > >>>>>>>> dev_pm_domain_detach() that is called by platform bus_remove() may shut
> > > >>>>>>>> off the device too early, before cleanup code has a chance to execute
> > > >>>>>>>> proper cleanup.
> > > >>>>>>>>
> > > >>>>>>>> The issue is not limited to runtime PM.
> > > >>>>>>>>
> > > >>>>>>>>>
> > > >>>>>>>>>> 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.
> > > >>>>>>>>>
> > > >>>>>>>>> dev_pm_domain_attach() is not part driver API and I'm not convinced at
> > > >>>>>>>>
> > > >>>>>>>> Is the concern that devm_pm_domain_attach() will be [ab]used by drivers?
> > > >>>>>>>
> > > >>>>>>> Yes, among other things.
> > > >>>>>>
> > > >>>>>> Maybe naming could make abuse at least obvious to spot? e.g.
> > > >>>>>> pm_domain_attach_with_devm_release()
> > > >>>>>
> > > >>>>> If I'm not mistaken, it is not even necessary to use devres for this.
> > > >>>>>
> > > >>>>> You might as well add a dev_pm_domain_detach() call to
> > > >>>>> device_unbind_cleanup() after devres_release_all().  There is a slight
> > > >>>>> complication related to the second argument of it, but I suppose that
> > > >>>>> this can be determined at the attach time and stored in a new device
> > > >>>>> PM flag, or similar.
> > > >>>>>
> > > >>>>
> > > >>>> I looked into this solution. I've tested it for all my failure cases and
> > > >>>> went good.
> > > >>>
> > > >>> OK
> > > >>>
> > > >>>>> Note that dev->pm_domain is expected to be cleared by ->detach(), so
> > > >>>>> this should not cause the domain to be detached twice in a row from
> > > >>>>> the same device, but that needs to be double-checked.
> > > >>>>
> > > >>>> The genpd_dev_pm_detach() calls genpd_remove_device() ->
> > > >>>> dev_pm_domain_set(dev, NULL) which sets the dev->pm_domain = NULL. I can't
> > > >>>> find any other detach function in the current code base.
> > > >>>
> > > >>> There is also acpi_dev_pm_detach() which can be somewhat hard to find,
> > > >>> but it calls dev_pm_domain_set(dev, NULL) either.
> > > >>>
> > > >>>> The code I've tested for this solution is this one:
> > > >>>>
> > > >>>> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> > > >>>> index b526e0e0f52d..5e9750d007b4 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,8 +553,11 @@ static void device_unbind_cleanup(struct device *dev)
> > > >>>>         dev->dma_range_map = NULL;
> > > >>>>         device_set_driver(dev, NULL);
> > > >>>>         dev_set_drvdata(dev, NULL);
> > > >>>> -       if (dev->pm_domain && dev->pm_domain->dismiss)
> > > >>>> -               dev->pm_domain->dismiss(dev);
> > > >>>> +       if (dev->pm_domain) {
> > > >>>> +               if (dev->pm_domain->dismiss)
> > > >>>> +                       dev->pm_domain->dismiss(dev);
> > > >>>> +               dev_pm_domain_detach(dev, dev->pm_domain->detach_power_off);
> > > >>>
> > > >>> I would do the "detach" before the "dismiss" to retain the current ordering.
> > > >>
> > > >> I applied on my local development branch all your suggestions except this
> > > >> one because genpd_dev_pm_detach() as well as acpi_dev_pm_detach() set
> > > >> dev->pm_domain = NULL.
> > > >>
> > > >> Due to this I would call first ->dismiss() then ->detach(), as initially
> > > >> proposed. Please let me know if you consider it otherwise.
> > > >
> > > > This is a matter of adding one more dev->pm_domain check AFAICS, but OK.
> > >
> > > I don't know all the subtleties around this, my concern with adding one
> > > more check on dev->pm_domain was that the
> > > dev->pm_domain->dismiss() will never be called if the ->detach() function
> > > will be called before ->dismiss() and it will set dev->pm_domain = NULL (as
> > > it does today (though genpd_dev_pm_detach() and acpi_dev_pm_detach())).
> > >
> > > For platform drivers that used to call dev_pm_domain_detach() in platform
> > > bus remove function, if I'm not wrong, the dev->pm_domain->dismiss() was
> > > never called previously. If that is a valid scenario, the code proposed in
> > > this thread will change the behavior for devices that have ->dismiss()
> > > implemented.
> >
> > ->dismiss() and ->detach() are supposed to be mutually exclusive, so
> > this should not be a problem either way and in practice so far the
> > only user of ->dismiss() has been acpi_lpss_pm_domain which doesn't do
> > ->detach() at all.
>
> May I ask if you know if there remains any real good reasons to keep
> the ->dismiss() callback around?
>
> Can't acpi_lpss_pm_domain() convert to use the ->detach() callback
> instead? Just thinking that it would be easier, but maybe it doesn't
> work.

It will, but let's just not make too many changes in one go.

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

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 075ec1d1b73a..c39d21fc1793 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);
 	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