Message-ID: <20250127123250.00002784@huawei.com>
Date: Mon, 27 Jan 2025 12:32:50 +0000
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Ulf Hansson <ulf.hansson@...aro.org>
CC: Claudiu Beznea <claudiu.beznea@...on.dev>, Jonathan Cameron
	<jic23@...nel.org>, <prabhakar.mahadev-lad.rj@...renesas.com>,
	<lars@...afoo.de>, <linux-iio@...r.kernel.org>,
	<linux-renesas-soc@...r.kernel.org>, <linux-kernel@...r.kernel.org>, "Claudiu
 Beznea" <claudiu.beznea.uj@...renesas.com>, "Rafael J. Wysocki"
	<rafael@...nel.org>, <linux-pm@...r.kernel.org>, Greg Kroah-Hartman
	<gregkh@...uxfoundation.org>, Dmitry Torokhov <dmitry.torokhov@...il.com>
Subject: Re: [PATCH 1/2] iio: adc: rzg2l_adc: Drop devm_pm_runtime_enable()

On Mon, 27 Jan 2025 11:47:44 +0100
Ulf Hansson <ulf.hansson@...aro.org> wrote:

> [...]
> 
> > > > > > Do consider OK to change the order in pm_runtime_disable_action() to get
> > > > > > rid of these issues, e.g.:
> > > > > >
> > > > > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > > > > > index 2ee45841486b..f27d311d2619 100644
> > > > > > --- a/drivers/base/power/runtime.c
> > > > > > +++ b/drivers/base/power/runtime.c
> > > > > > @@ -1547,8 +1547,8 @@ EXPORT_SYMBOL_GPL(pm_runtime_enable);
> > > > > >
> > > > > >  static void pm_runtime_disable_action(void *data)
> > > > > >  {
> > > > > > -       pm_runtime_dont_use_autosuspend(data);
> > > > > >         pm_runtime_disable(data);
> > > > > > +       pm_runtime_dont_use_autosuspend(data);
> > > > > >  }
> > > > > >
> > > > > > though I see a rpm_resume() call is still possible though pm_runtime_disable().  
> > > > >
> > > > > I am still worried about keeping the device runtime enabled during a
> > > > > window when we have turned off all resources for the device. Typically
> > > > > we want to leave the device in a low power state after unbind.
> > > > >
> > > > > That said, I would rather just drop the devm_pm_runtime_enable() API
> > > > > altogether and convert all users of it into
> > > > > pm_runtime_enable|disable(), similar to what your patch does.  
> > > >
> > > > That is making a mess of a lot of automated cleanup for a strange
> > > > runtime pm related path.  This is pain a driver should not have
> > > > to deal with, though I'm not clear what the right solution is!
> > > >
> > > > Key is that drivers should not mix devm managed cleanup and not, so
> > > > that means that anything that happens after runtime pm is enabled
> > > > has to be torn down manually.  One solution to this might be to
> > > > always enable it late assuming that is safe to do so there is
> > > > never anything else done after it in the probe path of a driver.  
> > >
> > > The problem is that runtime PM isn't really comparable to other
> > > resources that we are managing through devm* functions.
> > >
> > > Enabling runtime PM for a device changes the behaviour for how
> > > power-mgmt is handled for the device. Enabling/disabling of runtime PM
> > > really needs to be explicitly controlled by the driver for the device.  
> >
> > I'm sorry to say I'm not yet convinced.  
> 
> Okay, let me try one more time. :-)

+CC Greg as the disagreement here is really a philosophy of what
devm cleanup is relative to remove.  Perhaps Greg or Rafael can
given some guidance on the intent there.

Mind you I think I found another subsystem working around this
and in a somewhat more elegant, general way (to my eyes anyway!)

https://elixir.bootlin.com/linux/v6.12.6/source/drivers/i2c/i2c-core-base.c#L630
https://lore.kernel.org/all/YFf1GFPephFxC0mC@google.com/

+CC Dmitry.

I2C creates an extra devres group and releases it before devm_pm_domain_detach()
As all devm calls from the driver end up in that group, they are released
before dev_pm_domain_detach()


> 
> >
> > Devm callbacks are explicitly registered by the driver so that they
> > are unwound in a specific order.  Many other parts of driver
> > registration rely on this ordering.  This does not seem different
> > for runtime PM than anything else.  
> 
> If you compare clocks, for example. It's the driver that is in full
> control of the clock gating/ungating. When the ->remove() callback
> runs, the driver typically makes sure that it leaves the clock gated.
> Then it doesn't really matter when the clock resource gets released.
> The point is, the driver is in full control of the resource.

Not a good example. devm_clk_get_enabled() does not gate the clock until
the devm cleanup. The assumption being that nothing that affects
it runs between the remove() and devm cleanup.  So pretty much identical
to the runtime pm case.  They being that you have to obey ordering so
that if you need to run something after the clock is disabled then
you register that callback before you call devm_clk_get_enabled()

> 
> If runtime PM would remain enabled beyond the call to the ->remove()
> callback, it would mean that the driver's runtime PM callbacks could
> be called too. For example, userspace via sysfs may at any point
> decide to runtime resume the device. In other words, we may end up
> calling the runtime PM callbacks in the driver, when they are not
> intended to be called. In the worst case, I guess we could even end up
> trying to control resources (like a clock) from the ->runtime
> _resume() callback, when the references to these resources may already
> have been released.

This is all about what we consider remove. To me, with devm_ manged cleanup
in place, both remove() and devm_ cleanup count as parts of that remove
process.  

One option I did wonder about was having a devm_pm_domain_attach()
A little cheeky but I think the call in platform_probe() is late enough
that we don't run into the checks on no devm_ calls before driver probe.

That would shuffle the dev_pm_domain_detach() to the end of the
devm_ cleanup.  Mind you the i2c approach above seems better.

Jonathan

> 
> >
> > Superficially the issue here looks to me to be that a non devm
> > cleanup is inserted by the bus->remove() callback into drivers
> > that are otherwise relying on explicit ordering provided
> > by managed cleanup.
> >
> > I appreciate there may be no trivial solution.
> >
> > Maybe we can minimize that impact by always doing runtime pm last
> > in any probe() function?  Can that work here?
> >
> >  
> > >
> > > If there are cleanups to be made for runtime PM, beyond disabling
> > > runtime PM, we could instead consider adding that code in
> > > pm_runtime_reinit().  
> >
> > I'm not familiar enough to comment on this option beyond it being
> > after devres_release_all() so, maybe?  It seems superficially
> > a better point in the sequence.
> >
> > Jonathan  
> > >
> > > [...]  
> 
> Kind regards
> Uffe

Powered by blists - more mailing lists