Message-ID: <20250611172307.37c9b725@jic23-huawei>
Date: Wed, 11 Jun 2025 17:23:07 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: "Rafael J. Wysocki" <rafael@...nel.org>
Cc: Dmitry Torokhov <dmitry.torokhov@...il.com>, Claudiu
 <claudiu.beznea@...on.dev>, gregkh@...uxfoundation.org, dakr@...nel.org,
 len.brown@...el.com, pavel@...nel.org, ulf.hansson@...aro.org,
 daniel.lezcano@...aro.org, linux-kernel@...r.kernel.org,
 linux-pm@...r.kernel.org, bhelgaas@...gle.com, geert@...ux-m68k.org,
 linux-iio@...r.kernel.org, linux-renesas-soc@...r.kernel.org,
 fabrizio.castro.jz@...esas.com, Claudiu Beznea
 <claudiu.beznea.uj@...renesas.com>
Subject: Re: [PATCH v3 1/2] PM: domains: Add devres variant for
 dev_pm_domain_attach()

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

> On Sat, Jun 7, 2025 at 3:06 PM Jonathan Cameron <jic23@...nel.org> wrote:
> >
> > On Fri, 6 Jun 2025 22:01:52 +0200
> > "Rafael J. Wysocki" <rafael@...nel.org> wrote:
> >
> > Hi Rafael,
> >  
> > > On Fri, Jun 6, 2025 at 8:55 PM Dmitry Torokhov
> > > <dmitry.torokhov@...il.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@...on.dev> 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.  
> > > > >
> > > > > 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!
> 

Powered by blists - more mailing lists