Re: [PATCH] PM / Sleep: only update last time for active wakeup sources

[Doug Berger]

On 04/25/2018 11:30 PM, Rafael J. Wysocki wrote:
> On Thu, Apr 26, 2018 at 1:40 AM, Doug Berger <opendmb@xxxxxxxxx> wrote:
>> When wakelock support was added, the wakeup_source_add() function
>> was updated to set the last_time value of the wakeup source. This
>> has the unintended side effect of producing confusing output from
>> pm_print_active_wakeup_sources() when a wakeup source is added
>> prior to a sleep that is blocked by a different wakeup source.
>>
>> The function pm_print_active_wakeup_sources() will search for the
>> most recently active wakeup source when no active source is found.
>> If a wakeup source is added after a different wakeup source blocks
>> the system from going to sleep it may have a later last_time value
>> than the blocking source and be output as the last active wakeup
>> source even if it has never actually been active.
>>
>> It looks to me like the change to wakeup_source_add() was made to
>> prevent the wakelock garbage collection from accidentally dropping
>> a wakelock during the narrow window between adding the wakelock to
>> the wakelock list in wakelock_lookup_add() and the activation of
>> the wakeup source in pm_wake_lock().
>>
>> This commit changes the behavior so that only the last_time of the
>> wakeup source used by a wakelock is initialized prior to adding it
>> to the wakeup source list. This preserves the meaning of the
>> last_time value as the last time the wakeup source was active and
>> allows a wakeup source that has never been active to have a
>> last_time value of 0.
>>
>> Fixes: b86ff982 ("PM / Sleep: Add user space interface for manipulating wakeup sources, v3")
>> Signed-off-by: Doug Berger <opendmb@xxxxxxxxx>
>> ---
>>  drivers/base/power/wakeup.c | 1 -
>>  kernel/power/wakelock.c     | 1 +
>>  2 files changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
>> index ea01621..230160e 100644
>> --- a/drivers/base/power/wakeup.c
>> +++ b/drivers/base/power/wakeup.c
>> @@ -183,7 +183,6 @@ void wakeup_source_add(struct wakeup_source *ws)
>>         spin_lock_init(&ws->lock);
>>         timer_setup(&ws->timer, pm_wakeup_timer_fn, 0);
>>         ws->active = false;
>> -       ws->last_time = ktime_get();
> 
> If it is not initialized here, max_time may not be updated correctly later on.
> 
> If you don't want to initialize it to ktime_get() (to avoid the issue
> you're trying to avoid), initialize it to something special and then
> check for that explicitly in wakeup_source_deactivate() when computing
> max_time.
> 

I'm a little confused by your meaning. If you are concerned that the
duration calculation in wakeup_source_deactivate() may be compromised by
not initializing last_time in wakeup_source_add() and that an incorrect
duration could find its way into the comparison and update of max_time
then I don't believe that is a realizable concern.

As far as I can see there are no execution paths to
wakeup_source_deactivate() that don't require a call to
wakeup_source_activate() earlier in the path. The call to
wakeup_source_activate() will set the last_time to its proper value for
use by wakeup_source_deactivate().

So it should be safe to leave last_time at its initial 0 value in
wakeup_source_add() without impacting wakeup_source_deactivate() or
print_wakeup_source_stats().

This is the behavior of your original implementation of wakeup sources.
It wasn't changed until the wakelock support was added and as I said it
only appears to be necessary to protect against the timing hazard with
the garbage collecting thread possibly finding the wakeup_source from
the wakelock list before the pm_wake_lock() function has the opportunity
to activate the associated wakeup source.

>>
>>         spin_lock_irqsave(&events_lock, flags);
>>         list_add_rcu(&ws->entry, &wakeup_sources);
>> diff --git a/kernel/power/wakelock.c b/kernel/power/wakelock.c
>> index dfba59b..4210152 100644
>> --- a/kernel/power/wakelock.c
>> +++ b/kernel/power/wakelock.c
>> @@ -188,6 +188,7 @@ static struct wakelock *wakelock_lookup_add(const char *name, size_t len,
>>                 return ERR_PTR(-ENOMEM);
>>         }
>>         wl->ws.name = wl->name;
>> +       wl->ws.last_time = ktime_get();

This proposed change forces an early initialization of the last_time for
wakelocks only to protect against accidental garbage collection between
wakelock_lookup_add() and the subsequent call of __pm_wakeup_event() or
__pm_stay_awake() where last_time will be initialized again.


>>         wakeup_source_add(&wl->ws);
>>         rb_link_node(&wl->node, parent, node);
>>         rb_insert_color(&wl->node, &wakelocks_tree);
>> --
>> 2.7.4
>>

Thank you for your timely review and consideration of this patch,
    Doug