Message-ID: <gthb7h5e3lu2lbajsra22nzlkfafxhnn4d5unpx2soxlj6bpvc@p2y6qjb5lpqd>
Date: Tue, 20 May 2025 12:08:32 -0700
From: Dmitry Torokhov <dmitry.torokhov@...il.com>
To: Ulf Hansson <ulf.hansson@...aro.org>
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

On Tue, May 20, 2025 at 02:09:36PM +0200, Ulf Hansson wrote:
> [...]
> 
> > > > >>>>>
> > > > >>>>
> > > > >>>> 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.

You also need to be careful enabling and disabling interrupts, be
careful with power supplies to make sure the chip you want to talk to is
powered up, and has clocks running.

But I think you are concentrating too much on runtime PM aspect and are
missing the bigger picture: even in the absence of runtime PM we should
not detach the device from its power domain out of order compared to
releasing other resources.

We have:

static void platform_remove(struct device *_dev)
{
	struct platform_driver *drv = to_platform_driver(_dev->driver);
	struct platform_device *dev = to_platform_device(_dev);

	if (drv->remove)
		drv->remove(dev);
	dev_pm_domain_detach(_dev, true);
}

That "dev_pm_domain_detach(_dev, true)" means that device may get
powered off. If you look at for example ACPI power domain it will end up
calling acpi_dev_pm_low_power() which may place the device into D3Cold
state. At this point you can't talk to the device.

Now, if the driver used devm APIs, all the resources acquired will be
released *after* we detach from the domain. That means that interrupts
may still be firing, custom devm callbacks may still try to communicate
with the device, etc. This can cause multitude of errors, from
relatively benign logs in dmesg to system hangs. 

To solve this is simple: make sure we take devm into account and
release all devm resources acquired after the device was attached to a
power domain before we detach the device from that power domain.

If we do this we can also safely use devm_pm_runtime_enable() and it
will be disabled at the right time and everyone will be happy.

> 
> 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.

If you have this situation that means that you have logic error in the
driver. You should not run off resources too early. devm usually helps
with that. If you do:

devm_regulator_get_enaled()
devm_clk_get_enabled()
devm_request_threaded_irq()
devm_pm_runtime_enable()

then at remove time you should see:

runtime_pm_disable()
free_irq()
clk_disable()
regulator_disable/put()

in this particular order, which should be safe. But if you start mixing
things up and let's say use clk_get() (not devm variant) and explicitly
call clk_put() in remove() you will end up with clocks disabled but
interrupts still active and trouble will ensue.

The point of devm is so that you do not simply "leave runtime PM
enabled" with other resources shut off.

> 
> >
> > >
> > > >
> > > > 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?

*Order* is important. You need to release everything in reverse order.

> 
> Or maybe I didn't understand the problem correctly?

If you mix devm and regular APIs it is nearly impossible to maintain the
proper order of releasing the resources.

I hope I was able to explain the issue above.

> 
> >
> > >
> > > >
> > > >
> > > > >
> > > > > 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?

The proper order is to disable runtime PM at the right time (which is
typically early enough as it is typically enabled late in probe()) so
there is no concern with "leaving it enabled". And again, the issue is
not limited to runtime PM, even devices that do not use runtime PM
should not be detached from their power domain too early.

> 
> >
> > > > >
> > > > > 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()?

It is mixing devres and non-devres resources, yes. Not specifically
runtime PM.

Thanks.

-- 
Dmitry

Powered by blists - more mailing lists