On Saturday 07 February 2009, Arve Hjønnevåg wrote: > On Sat, Feb 7, 2009 at 10:56 AM, Rafael J. Wysocki <rjw@xxxxxxx> wrote: > > If my understanding is correct, a wakelock is a mechanism that, if held, will > > prevent the system from (automatically) entering a sleep state, but why do we > > need a number of such wakelocks instead of just one reference counter with the > > rule that (automatic) suspend can only happen if the counter is zero? > > Using wakelocks instead of a global reference count ensures that your > request cannot be cleared by someone else. Decreasing the refcount without increasing it would have been a bug, IMO. > It means that you can, without knowing the current state of the lock, safely > call wake_lock when you know that you need the wakelock, and > wake_unlock when you are done. You can do the same with a reference counter IMO. > It allows wakelocks with timeouts OK What for? > and detailed stats. Well, I'm not sure how this is useful in the long run. > > Then, code paths wanting to prevent the suspend from happening would only need > > to increase the counter and it shouldn't be difficult to provide a user space > > interface for that. > > No, but you would also need to provide way to decrement it, which > would allow user-space to override a request to stay awake in the > kernel or from another client. OK, that's a good reason. However, it might be better to use a refcount along with some mechanism allowing user space processes to increase it only once before decreasing. That would require a per-task flag, but I think you can reuse one of the freezer flags for that (the freezer is not going to run as long as a wakelock is held, right?). > >> +This works whether the wakelock is already held or not. It is useful if the > >> +driver woke up other parts of the system that do not use wakelocks but > >> +still need to run. Avoid this when possible, since it will waste power > >> +if the timeout is long or may fail to finish needed work if the timeout is > >> +short. > > > > And what's this call needed for? Please don't remove the context. This makes reading your reply difficult. > It is needed to give code that do not use wakelocks a chance to run. > For instance we have not added wakelocks to the network stack. We also > use it in some places when passing data to untrusted userspace code. Do you mean to take a wakelock with a timeout in one code path so that some other code path can run knowing that suspend will not happen? If this is correct, I don't like it, because it's inherently unreliable (you really never know how long it will take the other code path to run, so you can't choose the timeout 100% accurately). I really would use a refcount and make sure it's increased (and obviously decreased) by _every_ code path that needs to prevent suspend from happening, including networking and so on. > > Is there a mechanism allowing us to see what wakelocks have been created by > > the user land? Something like this would be useful for debugging. > > /proc/wakelocks shows all wakelocks. It does not currently indicate if > a wakelock is from userspace, but if you are looking for a specific > lock it is easy to find. I can add a prefix to the name of all user > space wakelocks of you want. Well, actually, do the wakelocks have to have the names? > >> +Do not use randomly generated wakelock names as there is no api to free > >> +a user-space wakelock. > > > > This appears to be very fragile. Please rethink it. > > I'm adding a /dev interface which removes this limitation. > > >> +/* wake_lock_active returns a non-zero value if the wake_lock is currently > >> + * locked. If the wake_lock has a timeout, it does not check the timeout, > >> + * but if the timeout had already expired when it was checked elsewhere > >> + * this function will return 0. > >> + */ > >> +int wake_lock_active(struct wake_lock *lock); > > > > What's the purpose of this function? > > It is used by our alarm driver to abort suspend. In what way, exactly? > >> +/* has_wake_lock returns 0 if no wake locks of the specified type are active, > >> + * and non-zero if one or more wake locks are held. Specifically it returns > >> + * -1 if one or more wake locks with no timeout are active or the > >> + * number of jiffies until all active wake locks time out. > >> + */ > >> +long has_wake_lock(int type); Well, it should be called max_wake_lock_timeout() or something like this. > > And this? > > It is used to abort suspend. Currently when freezing processes, but it > could also be called between each driver suspend call to improve > wakeup latency when a wakelock is locked while suspending. > > We also use it (with WAKE_LOCK_IDLE) to select the idle sleep mode. OK, but it is not supposed to be used by device drivers, right? Rafael _______________________________________________ linux-pm mailing list linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linux-foundation.org/mailman/listinfo/linux-pm
