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 -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
