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]
Date:   Wed, 17 May 2023 10:28:01 +0200
From:   Johan Hovold <johan@...nel.org>
To:     Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
Cc:     "Rafael J. Wysocki" <rafael@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-kernel@...r.kernel.org, kernel@...gutronix.de
Subject: Re: [PATCH] driver core: Call pm_runtime_put_sync() only after
 device_remove()

On Fri, May 12, 2023 at 08:49:25PM +0200, Uwe Kleine-König wrote:
> On Fri, May 12, 2023 at 09:40:01AM +0200, Johan Hovold wrote:
> > On Thu, May 11, 2023 at 04:44:25PM +0200, Rafael J. Wysocki wrote:
> > > On Thu, May 11, 2023 at 1:48 PM Johan Hovold <johan@...nel.org> wrote:
> > 
> > > > No, this seems like very bad idea and even violates the documentation
> > > > which clearly states that the usage counter is balanced before calling
> > > > remove() so that drivers can use pm_runtime_suspend() to put devices
> > > > into suspended state.
> > > 
> > > I missed that, sorry.
> > > 
> > > > There's is really no good reason to even try to change as this is in no
> > > > way a fast path.
> > > 
> > > Still, I think that while the "put" part needs to be done before
> > > device_remove(), the actual state change can be carried out later.
> > > 
> > > So something like
> > > 
> > >     pm_runtime_put_noidle(dev);
> > > 
> > >     device_remove(dev);
> > > 
> > >     pm_runtime_suspend(dev);
> > > 
> > > would generally work, wouldn't it?
> > 
> > No, as drivers typically disable runtime pm in their remove callbacks,
> > that pm_runtime_suspend() would amount to a no-op (and calling the
> > driver pm ops post unbind and the driver having freed its data would
> > not work either).
> 
> However if a driver author also cares for the CONFIG_PM=n case, calling
> pm_runtime_suspend() doesn't have the intended effect and so it's
> unfortunately complicated to rely on runtime-pm to power down your
> device and you have to do it by hand anyhow (unless you let your driver
> depend on CONFIG_PM). So I'm not convinced that "A driver can call
> pm_runtime_suspend() to power down" is a useful thing to have.

Right, but we do have drivers that have CONFIG_PM as an explicit
dependency.

> In the end something like 72362dcdf654 ("can: mcp251xfd:
> mcp251xfd_unregister(): simplify runtime PM handling") might be an
> approach. But IMHO it's more complicated than it should be and honestly
> I'm not sure if it's safe and correct this way.

Yeah, unfortunately runtime PM is fairly underspecified so we end up
with this multitude of implementations, many of which are broken in
various ways. A smaller API with documented best-practices may have
helped, but that's not where we are right now.

Looks like 72362dcdf654 ("can: mcp251xfd: mcp251xfd_unregister():
simplify runtime PM handling") introduces yet another way to do things,
and which will break if anyone enables (or tries to use this pattern in
another driver with) autosuspend...

Johan

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ