Message-ID: <CAPDyKFptNg5t6RehRNNfnnuCqpfiaQLaHBEdh4aRXfn7X6rYQQ@mail.gmail.com>
Date: Wed, 28 May 2025 11:31:03 +0200
From: Ulf Hansson <ulf.hansson@...aro.org>
To: Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc: Claudiu <claudiu.beznea@...on.dev>, gregkh@...uxfoundation.org, rafael@...nel.org, 
	dakr@...nel.org, len.brown@...el.com, pavel@...nel.org, jic23@...nel.org, 
	daniel.lezcano@...aro.org, linux-kernel@...r.kernel.org, 
	linux-pm@...r.kernel.org, linux-iio@...r.kernel.org, 
	linux-renesas-soc@...r.kernel.org, bhelgaas@...gle.com, geert@...ux-m68k.org, 
	Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
Subject: Re: [PATCH v2 1/2] PM: domains: Add devres variant for dev_pm_domain_attach()

On Tue, 27 May 2025 at 23:27, Dmitry Torokhov <dmitry.torokhov@...il.com> wrote:
>
> Hi Claudiu,
>
> On Mon, May 26, 2025 at 03:20:53PM +0300, Claudiu wrote:
> > From: Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
> >
> > The dev_pm_domain_attach() function is typically used in bus code alongside
> > dev_pm_domain_detach(), often following patterns like:
> >
> > static int bus_probe(struct device *_dev)
> > {
> >     struct bus_driver *drv = to_bus_driver(dev->driver);
> >     struct bus_device *dev = to_bus_device(_dev);
> >     int ret;
> >
> >     // ...
> >
> >     ret = dev_pm_domain_attach(_dev, true);
> >     if (ret)
> >         return ret;
> >
> >     if (drv->probe)
> >         ret = drv->probe(dev);
> >
> >     // ...
> > }
> >
> > static void bus_remove(struct device *_dev)
> > {
> >     struct bus_driver *drv = to_bus_driver(dev->driver);
> >     struct bus_device *dev = to_bus_device(_dev);
> >
> >     if (drv->remove)
> >         drv->remove(dev);
> >     dev_pm_domain_detach(_dev);
> > }
> >
> > When the driver's probe function uses devres-managed resources that depend
> > on the power domain state, those resources are released later during
> > device_unbind_cleanup().
> >
> > Releasing devres-managed resources that depend on the power domain state
> > after detaching the device from its PM domain can cause failures.
> >
> > For example, if the driver uses devm_pm_runtime_enable() in its probe
> > function, and the device's clocks are managed by the PM domain, then
> > during removal the runtime PM is disabled in device_unbind_cleanup() after
> > the clocks have been removed from the PM domain. It may happen that the
> > devm_pm_runtime_enable() action causes the device to be runtime-resumed.
> > If the driver specific runtime PM APIs access registers directly, this
> > will lead to accessing device registers without clocks being enabled.
> > Similar issues may occur with other devres actions that access device
> > registers.
>
> I think you are concentrating too much on runtime PM aspect of this. As
> you mentioned in the last sentence the same issue may happen in the
> absence of runtime PM if the power domain code will shut down the device
> while it is not fully cleaned up.
>
> >
> > Add devm_pm_domain_attach(). When replacing the dev_pm_domain_attach() and
> > dev_pm_domain_detach() in bus probe and bus remove, it ensures that the
> > device is detached from its PM domain in device_unbind_cleanup(), only
> > after all driver's devres-managed resources have been release.
> >
> > For flexibility, the implemented devm_pm_domain_attach() has 2 state
> > arguments, one for the domain state on attach, one for the domain state on
> > detach.
> >
> > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
> > ---
> >
> > Changes in v2:
> > - none; this patch is new
> >
> >  drivers/base/power/common.c | 59 +++++++++++++++++++++++++++++++++++++
> >  include/linux/pm_domain.h   |  8 +++++
> >  2 files changed, 67 insertions(+)
> >
> > diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c
> > index 781968a128ff..6ef0924efe2e 100644
> > --- a/drivers/base/power/common.c
> > +++ b/drivers/base/power/common.c
> > @@ -115,6 +115,65 @@ int dev_pm_domain_attach(struct device *dev, bool power_on)
> >  }
> >  EXPORT_SYMBOL_GPL(dev_pm_domain_attach);
> >
> > +/**
> > + * devm_pm_domain_detach_off - devres action for devm_pm_domain_attach() to
> > + * detach a device and power it off.
> > + * @dev: device to detach.
> > + *
> > + * This function reverse the actions from devm_pm_domain_attach().
> > + * It will be invoked during the remove phase from drivers implicitly.
> > + */
> > +static void devm_pm_domain_detach_off(void *dev)
> > +{
> > +     dev_pm_domain_detach(dev, true);
> > +}
> > +
> > +/**
> > + * devm_pm_domain_detach_on - devres action for devm_pm_domain_attach() to
> > + * detach a device and power it on.
> > + * @dev: device to detach.
> > + *
> > + * This function reverse the actions from devm_pm_domain_attach().
> > + * It will be invoked during the remove phase from drivers implicitly.
> > + */
> > +static void devm_pm_domain_detach_on(void *dev)
> > +{
> > +     dev_pm_domain_detach(dev, false);
> > +}
> > +
> > +/**
> > + * devm_pm_domain_attach - devres-enabled version of dev_pm_domain_attach()
> > + * @dev: Device to attach.
> > + * @attach_power_on: Use to indicate whether we should power on the device
> > + *                   when attaching (true indicates the device is powered on
> > + *                   when attaching).
> > + * @detach_power_off: Used to indicate whether we should power off the device
> > + *                    when detaching (true indicates the device is powered off
> > + *                    when detaching).
> > + *
> > + * NOTE: this will also handle calling dev_pm_domain_detach() for
> > + * you during remove phase.
> > + *
> > + * Returns 0 on successfully attached PM domain, or a negative error code in
> > + * case of a failure.
> > + */
> > +int devm_pm_domain_attach(struct device *dev, bool attach_power_on,
> > +                       bool detach_power_off)
>
> Do we have examples where we power on a device and leave it powered on
> (or do not power on device on attach but power off it on detach)? I
> believe devm release should strictly mirror the acquisition, so separate
> flag is not needed.

This sounds reasonable for me too.

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

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

[...]

Kind regards
Uffe

Powered by blists - more mailing lists