Message-ID: <CAPDyKFqMB7XutXba73YHx1X4rm6uc3Fz6yMZ8yM=wgduEmgUDg@mail.gmail.com>
Date: Tue, 20 May 2025 14:09:36 +0200
From: Ulf Hansson <ulf.hansson@...aro.org>
To: Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc: Claudiu Beznea <claudiu.beznea@...on.dev>, Jonathan Cameron <jic23@...nel.org>, 
	"Rafael J. Wysocki" <rafael@...nel.org>, Daniel Lezcano <daniel.lezcano@...aro.org>, dakr@...nel.org, 
	linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org, 
	linux-renesas-soc@...r.kernel.org, geert@...ux-m68k.org, 
	Claudiu Beznea <claudiu.beznea.uj@...renesas.com>, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, linux-iio@...r.kernel.org, bhelgaas@...gle.com
Subject: Re: [PATCH] driver core: platform: Use devres group to free driver
 probe resources

[...]

> > > >>>>>
> > > >>>>
> > > >>>> Rafael,
> > > >>>>
> > > >>>> Greg suggested we ask for your input on the right option:
> > > >>>>
> > > >>>> https://lore.kernel.org/all/2025032703-genre-excitable-9473@gregkh/
> > > >>>> (that thread has the other option).
> > > >>>
> > > >>> Can you please let us know your opinion on this?
> > > >> Can you please let us know if you have any suggestions for this?
> > > >
> > > > It's been a while since I looked at this. Although as I understand it,
> > > > the main issue comes from using devm_pm_runtime_enable().
> > >
> > > Yes, it comes from the usage of devm_pm_runtime_enable() in drivers and the
> > > dev_pm_domain_detach() call in platform_remove() right after calling
> > > driver's remove function.
> >
> > Okay.
>
> This is not the root of the problem though. There is nothing really
> special about power domain and runtime power management. The root of the
> problem is that current code violates the order of releasing resources
> by mixing devm- and normal resource management together. Usually it is
> individual driver's fault, but in this case it is bus code that uses the
> manual release (dev_om_domain_detach) that violates the "release in
> opposite order to acquisition" rule.

As I said before, runtime PM is not a regular resource, but a
behaviour that we turn on/off for a device. Enabling and disabling
runtime PM needs to be managed more carefully in my opinion.

For example, even if the order is made correctly, suppose a driver's
->remove() callback completes by turning off the resources for its
device and leaves runtime PM enabled, as it relies on devres to do it
some point later. Beyond this point, nothing would prevent userspace
for runtime resuming/suspending the device via sysfs. I would be quite
worried if that happens as it certainly would lead to undefined
behaviour.

>
> >
> > >
> > > On the platform I experienced issues with, the dev_pm_domain_detach() drops
> > > the clocks from the device power domain and any subsequent PM runtime
> > > resume calls (that may happen in the devres cleanup phase) have no effect
> > > on enabling the clocks. If driver has functions registered (e.g. through
> > > devm_add_action_or_reset()), or driver specific runtime PM functions that
> > > access directly registers in the devres cleanup phase this leads to system
> > > aborts.
> >
> > So if you move away from using devm_pm_runtime_enable() things would
> > be easier to manage and there is no additional new devres-management
> > needed.
>
> How exactly will it improve the situation? You still need to make sure
> that you are not disabling things out of the order. You simply moving
> the complexity to the driver, essentially forbidding it (and any other
> driver on platform bus) from using any devm APIs.

The driver can still use the devres APIs to "get" all resources and
then rely on devres to "put" them. There is nothing that prevents
that, right?

Or maybe I didn't understand the problem correctly?

>
> >
> > >
> > >
> > > >
> > > > As I have tried to argue before, I think devm_pm_runtime_enable()
> > > > should *not* be used. Not here, not at all. Runtime PM isn't like any
> > > > other resources that we fetch/release. Instead, it's a behaviour that
> > > > you turn on and off, which needs to be managed more carefully, rather
> > > > than relying on fetch/release ordering from devres.
>
> I disagree. It is a resource that you turn on and off, same as clocks,
> regulators, interrupts, etc. We manage those during lifetime of the
> device, disable them when going into low power mode/suspend, reenable
> them upon resume, may disable and reenable them for other reasons.
>
> PM is not any more special here. As long as you keep the proper order of
> operations it works as well.

How would you solve the issue I pointed out above?

>
> > > >
> > > > That said, I would convert the driver to use pm_runtime_enable() and
> > > > pm_runtime_disable() instead.
> > >
> > > I've tried this approach previously but it resulted in more complicated
> > > code and thus, Jonathan wasn't happy with it [1].
> >
> > I understand that you have been trying to move forward to address
> > people's opinions. It's not always easy to keep everybody happy. :-)
> >
> > That said, I still think this is the most viable option as it's how
> > the vast majority of drivers do it today. A few lines of additional
> > code shouldn't really be a big problem in my opinion.
>
> Have you tried making such change? Again, you will need to abandon use
> of most other devm APIs so that you keep the order of releasing
> resources. The only devm that you can still use is devm_k*alloc(), the
> rest has to be converted into unmanaged.

I guess I need to take a stab at this particular use case.

Looking closer, could it be that it's really the combination of
turning on/off resources using devres (not just get/put if them) like
clocks - and using devm_pm_runtime_enable()?

Kind regards
Uffe

Powered by blists - more mailing lists