Re: [PATCH] driver core: Call pm_runtime_put_sync() only after device_remove()

[Johan Hovold <johan@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 1830 bytes --]

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@pengutronix.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@pengutronix.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@kernel.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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]