On Wednesday, August 01, 2012, Alan Stern wrote: > 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. Except that I meant something different from what I wrote, sorry about that (must be too much heat). What I really thought about was to do _get_noresume(), then check if the device is active and if not, queue up a work item (or another delayed execution in process context) that will do pm_runtime_resume() and then access the hardware. Why do I think it's better than plain pm_runtime_get()? Because the code path calling pm_runtime_resume() will know that it is safe to access the hardware after it has returned (unless it returns an error code, but that's exceptional). In contrast, with pm_runtime_get() there is no way to predict when the device is going to be resumed, so if the device happens to be suspended when pm_runtime_get() returns, the driver kind of has to poll it until it becomes active, or use a wait queue woken up from the resume callback, or do all of the processing in the resume callback itself (like in the example you mentioned). I'm not sure if the expectation that all driver writers will be able to implement any of these options correctly is a realistic one. > 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, that shouldn't need the is_suspended flag at all, methinks, and the reason it does need it is because it uses pm_runtime_get(). Moreover, processing requests in the resume callback is not something I'd recommend to anyone, because that's going to happen when the status of the device is RPM_RESUMING, we're holding a reference on the parent (if there's one) etc. So, it looks like I don't really agree with the example. :-) > > > 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. Well, these are not as misleading as pm_runtime_get(), at least in principle. 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
