On Tue, 31 Jul 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. Of course not. When you're in interrupt context, you can't wait around to see if the device will actually resume. (Unless you're using irq-safe runtime PM, but we can ignore that case.) > 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. This is exactly equivalent to doing pm_runtime_get() followed by a check, except that it's longer. Yes, I agree that pm_runtime_get(dev) is nothing more than a shorthand way of doing pm_runtime_get_noresume(dev) followed by pm_request_resume(dev). That doesn't mean it should be eliminated. Shorthands are convenient. As another example, pm_runtime_get_sync(dev) is nothing more than a shorthand for pm_runtime_get_noresume(dev) followed by pm_runtime_resume(dev). IMO, having these joint functions that do a get/put combined with a suspend/resume/idle is very useful. > 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. I disagree. Even if the device has to be active to generate an IRQ, it's possible for the interrupt handler to be delayed until after the device is suspended (unless the suspend routine explicitly calls synchronize_irq(), which would be pretty foolish). Hence the handler can't make any assumptions about the device's state. > 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. This is exactly equivalent to: pm_runtime_get(), check the status, if "suspended" then set a flag, otherwise do the work. Except that again, it's longer. Check out the sample driver code in section 9 of Documentation/power/runtime_pm.txt, especially the foo_read_or_write() routine. > > 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. :-) If you're willing to risk a certain amount of confusion, count me in. :-) While you're changing names around, consider also adding a "_runtime" somewhere to: pm_children_suspended, pm_schedule_suspend, pm_request_idle, pm_request_resume, pm_request_autosuspend. For example, we could have pm_runtime_idle_async instead of pm_request_idle. Alan Stern -- 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
