Re: Do we need asynchronous pm_runtime_get()? (was: Re: bisected regression ...)

From: "Rafael J. Wysocki" <rjw@sisk.pl>
To: Alan Stern <stern@rowland.harvard.edu>
Cc: linux-pci@vger.kernel.org, USB list <linux-usb@vger.kernel.org>,
	Linux PM list <linux-pm@vger.kernel.org>
Subject: Re: Do we need asynchronous pm_runtime_get()? (was: Re: bisected regression ...)
Date: Tue, 31 Jul 2012 23:49:04 +0200	[thread overview]
Message-ID: <201207312349.04365.rjw@sisk.pl> (raw)
In-Reply-To: <201207312334.47173.rjw@sisk.pl>

On Tuesday, July 31, 2012, Rafael J. Wysocki wrote:
> On Tuesday, July 31, 2012, Alan Stern wrote:
> > [CC: list trimmed]
> > 
> > On Tue, 31 Jul 2012, Rafael J. Wysocki wrote:
> > 
> > > Now it occured to me that perhaps we don't need the current asynchronous
> > > pm_runtime_get() at all.  The usefulness of it is quite questionable, because
> > > either we want to resume the device immediately, for which pm_runtime_get_sync()
> > > should be used, or we just want to bump up the usage counter, in which cases
> > > pm_runtime_get_noresume() should always be sufficient.  I fail to see any
> > > particularly compelling use case for pm_runtime_get() doing an asynchronous
> > > resume at the moment, but perhaps that's just me.
> > 
> > There are indeed valid uses for pm_runtime_get().  We are forced to use
> > it in non-sleepable contexts when we want to resume the device as
> > quickly as possible.  Example: a driver receives an I/O request from an
> > interrupt handler.
> 
> Is it actually suitable to be used in such contexts?  It doesn't give any
> guarantee that the device will be active when it returns, so the caller can't
> really rely on it.  The caller always has to check if the device has been
> resumed before accessing it anyway, so it may as well do a _get_noresume(),
> do the check and do pm_request_resume() if needed directly.
> 
> Now, as far as interrupt handlers are concerned, there are two cases: either
> the device has to be active to generate an interrupt (like PCI), or the
> interrupt is generated by something else on behalf of it.  We don't seem to
> handle the first case very well right now, because in that case the interrupt
> handler will always know that the device is active, so it should do a
> _get_noresume() and then change the device's status to "active" without
> calling any kind of "resume", but we don't provide a helper for that.

Unless, of course, this is a shared interrupt, in which case the handler may
be invoked, because _another_ device has generated an interrupt, so the
"active" check will have to be done anyway.

> In the second case calling pm_runtime_get() doesn't really help either.
> I think it's better to do _get_noresume(), check the status and if
> "suspended", set a flag for the resume callback to do something specific
> before returning successfully, then call pm_request_resume() and return.

I realize that this may be somewhat racy, so perhaps we need something like
pm_request_resume_and_call(dev, func) that will execute the given function once
the device has been resumed (or just happens to be "active" when the resume
work item is run for it).

> > > However, I receive reports of people using pm_runtime_get() where they really
> > > should use pm_runtime_get_sync(), so I wonder if we can simply rename
> > > pm_runtime_get_sync() as pm_runtime_get() and drop the asynchronous version
> > > altogether?
> > 
> > Well, IMO the naming should have been the other way around from the
> > start.  That is, we should have made pm_runtime_get be the synchronous
> > routine and pm_runtime_get_async be the asynchronous one.  But it's too
> > late to change now.
> 
> I'm not sure it is too late.  If we first change all the instances of
> pm_runtime_get() to pm_runtime_get_async() and then all of the instances of
> pm_runtime_get_sync() to pm_runtime_get(), it should be technically possible.
> Of course, it would be confusing, but that's a different matter. :-)
> 
> > And no, we can't get rid of the async version.
> 
> I'm still not sure of that.

Thanks,
Rafael