On Sat, Feb 7, 2009 at 3:25 PM, Rafael J. Wysocki <rjw@xxxxxxx> wrote:
> 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.
Yes, but if all you have is a global reference count, you can't tell
where the bug is.
>> 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.
Not without some per lock state.
>> It allows wakelocks with timeouts
>
> OK
>
> What for?
So we do not have to change everything at once, and so we can allow
code that we do not really trust a chance to run.
>> and detailed stats.
>
> Well, I'm not sure how this is useful in the long run.
You may want to know which app drained your battery.
>> > 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?).
How would this be better?
>
>> >> +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 can also call wake_lock_timeout to release the wakelock after a delay:
+ wake_lock_timeout(&state->wakelock, HZ);
+
>
>> 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 agree, but without wakelock support you have two options. Do not
user suspend at all, or use a global timeout that you reset everywhere
we lock a wakelock. By using wakelocks with timeouts we can at least
make some events 100% reliable.
>
> 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?
If you want to debug the system or provide stats, yes.
>> >> +/* 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?
The alarm driver sets an rtc alarm on suspend. After grabbing its
state lock it, checks if the wakelock is active. Since the wakelock
implementation now prevents suspend in a suspend_late hook when
wakelocks are held, this may not be strictly necessary anymore, but
prevents the alarm driver from having to deal with the state of being
suspended while an alarm is pending.
>> >> +/* 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.
I called it has_wake_lock since most of the clients do not care about
the timeout. I think "if(has_wake_lock(" is easier to read than
"if(max_wake_lock_timeout(".
>
>> > 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?
No, it is only used by generic or machine specific power management code.
--
Arve Hjønnevåg
_______________________________________________
linux-pm mailing list
linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/linux-pm
