2010/8/19 Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>: > On Wed, 18 Aug 2010, Arve Hjønnevåg wrote: > >> >> By that argument, all in-kernel wakelocks should always have a timeout. >> >> >> I don't think that is the only conclusion. >> >> >> What happens if somebody puts down their phone and then puts something >> >> on top of it, in such a way that one of the keypad buttons is >> >> depressed? If the keypad driver's wakelock doesn't have a timeout then >> >> the battery would drain quickly. >> >> This is a real problem that might be worth solving, but just adding a >> wakelock timeout does not solve it since the keyboard driver would >> either renew the timeout after every scan, or it would leave the >> keyboard in a state where you could not use it to wake up the device >> anymore. > > Okay. Nevertheless, you maintain debugging info in case a driver bug > causes a wakelock not to be released. If the wakelocks all had > timeouts then a bug of that sort wouldn't be able to affect battery > life very much. So why not add the timeouts? > Because we can't. I just described how the keypad driver would break we just add a timeout to its wakelock. Other drivers, like the usb gadget driver may need to block suspend for an extended time period. > Or conversely, if you think the likelihood of such bugs is too low to > worry about, then why have the debugging info? > That statement assumes all the bugs can be fixed by adding timeouts, which I don't think is true. Also, you don't need to have bugs for the stats to be useful. The gps could for instance prevent suspend for a long time because the user ran a gps tracking app. >> >> The question is whether the kernel should automatically try to >> >> re-suspend following a wakeup for which no information was sent to >> >> userspace (e.g., WOL). The alternative, of course, is that the kernel >> >> does not try to re-suspend until a power-manager program tells it to. >> >> To me this seems more flexible and general (the PM program can always >> >> start a new suspend, whereas there's no way to prevent an automatic >> >> in-kernel re-suspend). >> > >> > I agree. >> > >> >> Having user space initiate every suspend operation is more flexible >> out of the box, but you can get the same flexibility with the other >> approach by adding an interface to report all wakeup events and failed >> suspend attempts. However, I have not seen a use case for this >> flexibility other than as a partial workaround for wakeup events that >> user space needs to know about but are not reported to user space any >> other way (it is only a partial workaround since it still has the same >> race condition we are fixing for other wakeup events). > > In any case, this is something that could easily be changed with a > small Android-specific kernel patch. At the moment it doesn't seem to > be a major issue. > Yes, that seems to be a likely outcome. >> >> > When multiple drivers use the same device, who creates and destroys the handle? >> >> >> >> The primary driver (i.e., the one bound to the device) would use the >> >> device's handle. Other drivers would create and use their own separate >> >> wakeup structures. >> >> >> >> But if all the calls are of the "nesting" sort then there's not really >> >> any need for multiple structures, other than to help identify which >> >> driver is misbehaving when a problem occurs. >> > >> > Still, IMO we can do that if it would help to narrow the gap between the >> > Android kernel and the mainline. >> > >> >> Some android drivers currently need the calls to be not nested so that >> they can mix timeouts and pm_relax. With only nesting apis we cannot >> get the same functionality. >> >> Other drivers rely on the non nesting property of the existing >> wakelock api but do not use timeouts. I expect these drivers can be >> converted have the same behavior with a nested api, but there could be >> difficult edge cases. >> >> Non-android driver may need a nested api so they can use a handle that >> they did not allocate. It is not clear to me if this is really needed. >> If it is not needed we can make all the calls non nested and require >> that the driver allocates a handle (or more commonly put the handle in >> the private data it already allocates). If we do need to keep the >> nested behavior, we a way to select which variant to use. We can have >> separate calls that nest and require drivers that use the non-nested >> variant have their own handle, or we could set a flag when creating a >> handle that makes calls on that handle not nest. > > I agree, there are several possible designs. It's hard to tell which > will be best without more experience. > > How much degradation do you think you would observe if in-kernel > wakelocks _always_ used timeouts alone and _never_ were cancelled? > I don't think that is an option. We have kernel wakelocks that need to be held while the device is in a specific state. >> >> You didn't answer my question about which devices have more than one >> >> associated wakelock in Android. Those would be the only cases where >> >> the device/driver combo matters. >> >> >> >> Wakelocks are not associated with devices at all right now, so I don't >> have a list for you. However, the input queues need more than one >> wakelock per device if they use timeouts. In this case there is only >> one driver though, but the driver allocates a separate queue for each >> client that opens a device. Our keypad/gpio-keys driver may also end >> up using multiple wakelocks. Many phones use different keys types in >> the same logical keyboard, so you could end up with one wakelock for >> the matrix scan and another one for debouncing keys connected directly >> to a gpio. The driver could be changed to share a nested handle, but I >> prefer to keep them separate. > > Okay. In addition, we've already seen that devices for which you > haven't yet implemented any wakelocks may end up needing more than one. > >> >> Also, why does Android's MMC core have its own wakelock instead of >> >> using a per-device wakelock? >> >> >> >> There is a global work queue. > > So you want to be able to cancel the wakelock whenever the queue is > empty, right? And not have to keep track of whether each MMC device > has any work items in the queue. > I have not reviewed that code, so I don't know the details of how it works or even if it is correct, but I see that it uses both timeouts and wake_unlock calls on the same wakelock. > In theory this is insufficient -- it doesn't handle the case where one > MMC device is enabled for wakeup and another isn't. I assume you don't > really care about this (especially since Android phones aren't likely > to have more than one MMC device capable of generating wakeup events). > I'm not sure what you mean by insufficient here. If it not optimal if you also have work that does not need to prevent suspend, but it should still work correctly. >> >> The driver's (or bus subsystem's) suspend routine calls >> >> device_may_wakeup(dev) to see whether it should enable dev's wakeup >> >> mechanism. The value returned by device_may_wakeup can change at >> >> almost any time, since it is set by userspace. >> >> >> >> It would be better to allocate the structure when user space changes >> the setting. If you wait until suspend before blocking suspend you may >> have passed the event on already. > > That will work for the structure directly associated with the device. > Additional structures allocated by the drivers have to be handled > differently, because the drivers aren't notified when the wakeup-enable > setting is changed. > I'm not convinced that is it sufficient to detect that a device is wakeup-enabled on suspend. However it is not a problem I have tried to solve since we have only used static wakeup sources. -- Arve Hjønnevåg _______________________________________________ linux-pm mailing list linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linux-foundation.org/mailman/listinfo/linux-pm
