linux-kernel - Re: [PATCH 1/2] iio: adc: rzg2l_adc: Drop devm_pm_runtime

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAPDyKFrqDfYEQHk0RsRi2LnMw_HgGozMW9JP9xmkAq52O7eztg@mail.gmail.com>
Date: Mon, 27 Jan 2025 11:47:44 +0100
From: Ulf Hansson <ulf.hansson@...aro.org>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>
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
Subject: Re: [PATCH 1/2] iio: adc: rzg2l_adc: Drop devm_pm_runtime_enable()

[...]

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

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

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.

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