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

[Johan Hovold <johan@kernel.org>]

On Fri, May 12, 2023 at 04:04:59PM +0200, Rafael J. Wysocki wrote:
> On Fri, May 12, 2023 at 9:39 AM Johan Hovold <johan@kernel.org> 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@kernel.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,
> 
> What exactly do you mean by "typically"?  None of the PCI drivers
> should do that, for instance.

I had platform drivers in mind, but so do i2c drivers for example.

> > 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).
> 
> Well, not really.
> 
> There are drivers and there are bus types/PM domains.  Drivers need
> not disable PM-runtime in their "remove" callbacks if they know that
> the bus type/PM domain will take care of handling PM-runtime properly
> after the driver's remove callback has run and the bus type/PM domain
> may very well want its PM-runtime suspend callback to run then (for
> example, to remove power from the unused device).  Arguably it can
> invoke runtime_suspend() from its "remove" callback, so it's not like
> this is a big deal, but IMO it helps if the most general case is
> considered.

My point was that hundreds of drivers do and for these this call becomes
a no-op. Same for buses that disable runtime pm at remove.

> Anyway, the question here really is: Does it make sense to carry out a
> runtime suspend immediately before device_remove()?  Honestly, I'm not
> sure about that.

I'd say it doesn't really matter as driver unbind is not a common
operation and drivers using autosuspend would generally not be affected
either.

You can try to rework this, but clearly it needs more thought than
simply moving the put sync and some drivers may also be relying on the
current behaviour.

A quick grep reveals a few which would be left active if you change
pm_runtime_put_sync() to pm_runtime_put_noidle(), even if that could be
fixed driver by driver of course.

Johan