| Message ID | 20250606111749.3142348-1-claudiu.beznea.uj@bp.renesas.com |
|---|---|
| Headers | show |
| Series | PM: domains: add devm_pm_domain_attach() | expand |
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!
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(-)