linux-kernel - Re: [PATCH] driver core: Call pm_runtime_put_sync() only after device

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: <ZFzWCey825wSlr2v@hovoldconsulting.com>
Date:   Thu, 11 May 2023 13:48:25 +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 Thu, May 11, 2023 at 12:39:23PM +0200, Uwe Kleine-König wrote:
> On Thu, May 11, 2023 at 12:18:09PM +0200, Rafael J. Wysocki wrote:
> > On Thu, May 11, 2023 at 9:34 AM Uwe Kleine-König
> > <u.kleine-koenig@...gutronix.de> wrote:
> > >
> > > Many drivers that use runtime PM call pm_runtime_get_sync() or one of
> > > its variants in their remove callback. So calling pm_runtime_put_sync()
> > > directly before calling the remove callback results (under some
> > > conditions) in the driver's suspend routine being called just to resume
> > > it again afterwards.
> > >
> > > So delay the pm_runtime_put_sync() call until after device_remove().
> > >
> > > Confirmed on a stm32mp157a that doing
> > >
> > >         echo 4400e000.can > /sys/bus/platform/drivers/m_can_platform/unbind
> > >
> > > (starting with a runtime-pm suspended 4400e000.can) results in one call
> > > less of m_can_runtime_resume() and m_can_runtime_suspend() each after
> > > this change was applied.
> > >
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
> > 
> > I'm not against this change, although I kind of expect it to trigger
> > some fallout that will need to be addressed.  So caveat emtor.
> > 
> > Anyway
> > 
> > Reviewed-by: Rafael J. Wysocki <rafael@...nel.org>
> 
> Thanks for your review tag. I wondered if there will be some fallout,
> and don't know what to expect yet. Sounds like getting it into next soon
> is a good idea?!

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.

There's is really no good reason to even try to change as this is in no
way a fast path.

NAK.

Johan

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