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@xxxxxxxxx> wrote:
> >
> > From: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
> >
> > 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