Message-ID: <Z5ht3NrbAOazH7ze@google.com>
Date: Mon, 27 Jan 2025 21:40:44 -0800
From: Dmitry Torokhov <dmitry.torokhov@...il.com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>
Cc: Ulf Hansson <ulf.hansson@...aro.org>,
	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>
Subject: Re: [PATCH 1/2] iio: adc: rzg2l_adc: Drop devm_pm_runtime_enable()

On Mon, Jan 27, 2025 at 06:24:23PM +0000, Jonathan Cameron wrote:
> On Mon, 27 Jan 2025 16:02:32 +0100
> Ulf Hansson <ulf.hansson@...aro.org> wrote:
> 
> > On Mon, 27 Jan 2025 at 13:32, Jonathan Cameron
> > <Jonathan.Cameron@...wei.com> wrote:
> > >
> > > 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()

There is also a similar fix in HID core.

> > >  
> > 
> > How would that address the problem I pointed out with runtime PM
> > below? This problem isn't limited to attaching/detaching PM domains.
> 
> It's associated with anything that happens after a driver remove is done.
> We just disagree on when that remove is finished. There is nothing special about
> the remove() callback, that is just part of remove process.
> No magic transition of state that allows new things to happen follows
> the device driver remove finishing. Sure you can get the remove
> handling ordering wrong whether devm is in use or not.  The trick is
> almost always to never mix devm and not.  Once you need a single bit of
> manual unwinding stop with the devm and do everything beyond that point
> by hand (in probe order, before that point in remove order)

Right, this is a classic problem of mixing devm-managed resources and
ordinary ones. Every time we have a bus remove() method that is not
trivial we need to make sure the resources are released in the right
order, which is:

1. Driver-allocated resources
2. Bus-allocated resources
3. Driver-core allocated resources.

Establishing a devres group before calling into drivers' probe() methods
(and releasing it before doing the rest of the cleanup in remove()) is
one such way.

Thanks.

-- 
Dmitry

Powered by blists - more mailing lists