On Friday, April 27, 2018 12:25:53 AM CEST Doug Berger wrote: > On 04/25/2018 11:30 PM, Rafael J. Wysocki wrote: > > On Thu, Apr 26, 2018 at 1:40 AM, Doug Berger <[email protected]> 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 <[email protected]> > >> --- > >> 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().
You're right, so this isn't a concern. > 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, You're welcome. I'll queue up the patch for 4.18, thanks!
