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

["Rafael J. Wysocki" <rafael@kernel.org>]

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!