Hi,
There are a few synchronization problems spotted by Arve in the
wakeup sources core code and the following patches are supposed to fix
them.
[1/3] - Fix possible infinite loop during wakeup source destruction.
[2/3] - Fix race conditions related to wakeup source timer function.
[3/3] - Make __pm_stay_awake() delete wakeup source timers.
The details are in the changelogs.
The patches are on top of the pm-sleep branch of the linux-pm tree.
Please let me know if you see any problems with these patches.
Otherwise they are regarded as v3.4 material.
Thanks,
Rafael
From: Rafael J. Wysocki <[email protected]>
If __pm_wakeup_event() has been used (with a nonzero timeout) to
report a wakeup event and then __pm_relax() immediately followed by
__pm_stay_awake() is called or __pm_wakeup_event() is called once
again for the same wakeup source object before its timer expires, the
timer function pm_wakeup_timer_fn() may still be run as a result of
the previous __pm_wakeup_event() call. In either of those cases it
may mistakenly deactivate the wakeup source that has just been
activated.
To prevent that from happening, make wakeup_source_deactivate()
clear the wakeup source's timer_expires field and make
pm_wakeup_timer_fn() check if timer_expires is different from zero
and if it's not in future before calling wakeup_source_deactivate()
(if timer_expires is 0, it means that the timer has just been
deleted and if timer_expires is in future, it means that the timer
has just been rescheduled to a different time).
Reported-by: Arve Hjønnevåg <[email protected]>
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/base/power/wakeup.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
Index: linux/drivers/base/power/wakeup.c
===================================================================
--- linux.orig/drivers/base/power/wakeup.c
+++ linux/drivers/base/power/wakeup.c
@@ -433,6 +433,7 @@ static void wakeup_source_deactivate(str
ws->max_time = duration;
del_timer(&ws->timer);
+ ws->timer_expires = 0;
/*
* Increment the counter of registered wakeup events and decrement the
@@ -487,11 +488,22 @@ EXPORT_SYMBOL_GPL(pm_relax);
* pm_wakeup_timer_fn - Delayed finalization of a wakeup event.
* @data: Address of the wakeup source object associated with the event source.
*
- * Call __pm_relax() for the wakeup source whose address is stored in @data.
+ * Call wakeup_source_deactivate() for the wakeup source whose address is stored
+ * in @data if it is currently active and its timer has not been canceled and
+ * the expiration time of the timer is not in future.
*/
static void pm_wakeup_timer_fn(unsigned long data)
{
- __pm_relax((struct wakeup_source *)data);
+ struct wakeup_source *ws = (struct wakeup_source *)data;
+ unsigned long flags;
+
+ spin_lock_irqsave(&ws->lock, flags);
+
+ if (ws->active && ws->timer_expires
+ && time_after_eq(jiffies, ws->timer_expires))
+ wakeup_source_deactivate(ws);
+
+ spin_unlock_irqrestore(&ws->lock, flags);
}
/**
From: Rafael J. Wysocki <[email protected]>
If wakeup_source_destroy() is called for an active wakeup source that
is never deactivated, it will spin forever. To prevent that from
happening, make wakeup_source_destroy() call __pm_relax() for the
wakeup source object it is about to free instead of waiting until
it will be deactivated by someone else. However, for this to work
it also needs to make sure that the timer function will not be
executed after the final __pm_relax(), so make it run
del_timer_sync() on the wakeup source's timer beforehand.
Additionally, update the kerneldoc comment to document the
requirement that __pm_stay_awake() and __pm_wakeup_event() must not
be run in parallel with wakeup_source_destroy().
Reported-by: Arve Hjønnevåg <[email protected]>
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/base/power/wakeup.c | 15 +++++----------
1 file changed, 5 insertions(+), 10 deletions(-)
Index: linux/drivers/base/power/wakeup.c
===================================================================
--- linux.orig/drivers/base/power/wakeup.c
+++ linux/drivers/base/power/wakeup.c
@@ -74,22 +74,17 @@ EXPORT_SYMBOL_GPL(wakeup_source_create);
/**
* wakeup_source_destroy - Destroy a struct wakeup_source object.
* @ws: Wakeup source to destroy.
+ *
+ * Callers must ensure that __pm_stay_awake() or __pm_wakeup_event() will never
+ * be run in parallel with this function for the same wakeup source object.
*/
void wakeup_source_destroy(struct wakeup_source *ws)
{
if (!ws)
return;
- spin_lock_irq(&ws->lock);
- while (ws->active) {
- spin_unlock_irq(&ws->lock);
-
- schedule_timeout_interruptible(msecs_to_jiffies(TIMEOUT));
-
- spin_lock_irq(&ws->lock);
- }
- spin_unlock_irq(&ws->lock);
-
+ del_timer_sync(&ws->timer);
+ __pm_relax(ws);
kfree(ws->name);
kfree(ws);
}
From: Rafael J. Wysocki <[email protected]>
If __pm_stay_awake() is called after __pm_wakeup_event() for the same
wakep source object before its timer expires, it won't cancel the
timer, so the wakeup source will be deactivated from the timer
function as scheduled by __pm_wakeup_event(). In that case
__pm_stay_awake() doesn't have any effect beyond incrementing
the wakeup source's event_count field, although it should cancel
the timer and make the wakeup source stay active until __pm_relax()
is called for it.
Conversely, if __pm_wakeup_event() is called for a wakeup source
that has been activated by __pm_stay_awake() before, it will set up
the timer to deactivate the wakeup source, although it should leave
it active until __pm_relax() is called for it.
To fix the first of these problems make __pm_stay_awake() delete the
wakeup source's timer and ensure that it won't be deactivated from
the timer funtion afterwards by clearing its timer_expires field.
To fix the second one, make __pm_wakeup_event() skip setting up the
timer if it sees that the wakeup source is active and its
timer_expires field is zero, which means that it has been activated
by __pm_stay_awake().
Reported-by: Arve Hjønnevåg <[email protected]>
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/base/power/wakeup.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
Index: linux/drivers/base/power/wakeup.c
===================================================================
--- linux.orig/drivers/base/power/wakeup.c
+++ linux/drivers/base/power/wakeup.c
@@ -365,9 +365,15 @@ void __pm_stay_awake(struct wakeup_sourc
return;
spin_lock_irqsave(&ws->lock, flags);
+
+ del_timer(&ws->timer);
+ ws->timer_expires = 0;
+
ws->event_count++;
+
if (!ws->active)
wakeup_source_activate(ws);
+
spin_unlock_irqrestore(&ws->lock, flags);
}
EXPORT_SYMBOL_GPL(__pm_stay_awake);
@@ -529,8 +535,12 @@ void __pm_wakeup_event(struct wakeup_sou
spin_lock_irqsave(&ws->lock, flags);
ws->event_count++;
- if (!ws->active)
+ if (ws->active) {
+ if (!ws->timer_expires)
+ goto unlock;
+ } else {
wakeup_source_activate(ws);
+ }
if (!msec) {
wakeup_source_deactivate(ws);
2012/2/15 Rafael J. Wysocki <[email protected]>:
> From: Rafael J. Wysocki <[email protected]>
>
> If __pm_stay_awake() is called after __pm_wakeup_event() for the same
> wakep source object before its timer expires, it won't cancel the
> timer, so the wakeup source will be deactivated from the timer
> function as scheduled by __pm_wakeup_event(). ?In that case
> __pm_stay_awake() doesn't have any effect beyond incrementing
> the wakeup source's event_count field, although it should cancel
> the timer and make the wakeup source stay active until __pm_relax()
> is called for it.
>
> Conversely, if __pm_wakeup_event() is called for a wakeup source
> that has been activated by __pm_stay_awake() before, it will set up
> the timer to deactivate the wakeup source, although it should leave
> it active until __pm_relax() is called for it.
We have many drivers that call wake_lock_timeout instead of
wake_unlock to cancel a previous wake_lock call. These drivers will
need to use two wakeup sources if __pm_wakeup_event does not always
set the timeout. I think it is better to have the state of the wakeup
source only depend on the last function you called, instead of that
last function being a noop in some cases.
--
Arve Hj?nnev?g
2012/2/15 Rafael J. Wysocki <[email protected]>:
...
> --- linux.orig/drivers/base/power/wakeup.c
> +++ linux/drivers/base/power/wakeup.c
> @@ -365,9 +365,15 @@ void __pm_stay_awake(struct wakeup_sourc
> ? ? ? ? ? ? ? ?return;
>
> ? ? ? ?spin_lock_irqsave(&ws->lock, flags);
> +
> + ? ? ? del_timer(&ws->timer);
> + ? ? ? ws->timer_expires = 0;
timer_expires gets overwritten in wakeup_source_activate, so
__pm_relax followed by __pm_stay_awake is still not safe.
...
> @@ -529,8 +535,12 @@ void __pm_wakeup_event(struct wakeup_sou
> ? ? ? ?spin_lock_irqsave(&ws->lock, flags);
>
> ? ? ? ?ws->event_count++;
> - ? ? ? if (!ws->active)
> + ? ? ? if (ws->active) {
> + ? ? ? ? ? ? ? if (!ws->timer_expires)
> + ? ? ? ? ? ? ? ? ? ? ? goto unlock;
> + ? ? ? } else {
> ? ? ? ? ? ? ? ?wakeup_source_activate(ws);
> + ? ? ? }
>
> ? ? ? ?if (!msec) {
> ? ? ? ? ? ? ? ?wakeup_source_deactivate(ws);
>
I suggest dropping this and adding:
- if (time_after(expires, ws->timer_expires)) {
+ if (!ws->timer_expires || time_after(expires, ws->timer_expires)) {
--
Arve Hj?nnev?g
On Thursday, February 16, 2012, Arve Hj?nnev?g wrote:
> 2012/2/15 Rafael J. Wysocki <[email protected]>:
> > From: Rafael J. Wysocki <[email protected]>
> >
> > If __pm_stay_awake() is called after __pm_wakeup_event() for the same
> > wakep source object before its timer expires, it won't cancel the
> > timer, so the wakeup source will be deactivated from the timer
> > function as scheduled by __pm_wakeup_event(). In that case
> > __pm_stay_awake() doesn't have any effect beyond incrementing
> > the wakeup source's event_count field, although it should cancel
> > the timer and make the wakeup source stay active until __pm_relax()
> > is called for it.
> >
> > Conversely, if __pm_wakeup_event() is called for a wakeup source
> > that has been activated by __pm_stay_awake() before, it will set up
> > the timer to deactivate the wakeup source, although it should leave
> > it active until __pm_relax() is called for it.
>
> We have many drivers that call wake_lock_timeout instead of
> wake_unlock to cancel a previous wake_lock call. These drivers will
> need to use two wakeup sources if __pm_wakeup_event does not always
> set the timeout. I think it is better to have the state of the wakeup
> source only depend on the last function you called, instead of that
> last function being a noop in some cases.
OK
Rafael
On Thursday, February 16, 2012, Arve Hj?nnev?g wrote:
> 2012/2/15 Rafael J. Wysocki <[email protected]>:
> ...
> > --- linux.orig/drivers/base/power/wakeup.c
> > +++ linux/drivers/base/power/wakeup.c
> > @@ -365,9 +365,15 @@ void __pm_stay_awake(struct wakeup_sourc
> > return;
> >
> > spin_lock_irqsave(&ws->lock, flags);
> > +
> > + del_timer(&ws->timer);
> > + ws->timer_expires = 0;
>
> timer_expires gets overwritten in wakeup_source_activate,
I actually don't remember why it does that and with the $subject changes
it's just pointless.
> so __pm_relax followed by __pm_stay_awake is still not safe.
Yes, I overlooked that timer_expires modification. Updated patch follows.
> ...
> > @@ -529,8 +535,12 @@ void __pm_wakeup_event(struct wakeup_sou
> > spin_lock_irqsave(&ws->lock, flags);
> >
> > ws->event_count++;
> > - if (!ws->active)
> > + if (ws->active) {
> > + if (!ws->timer_expires)
> > + goto unlock;
> > + } else {
> > wakeup_source_activate(ws);
> > + }
> >
> > if (!msec) {
> > wakeup_source_deactivate(ws);
> >
>
> I suggest dropping this and adding:
Well, what exactly would you like to drop? The above proposed changes I guess?
> - if (time_after(expires, ws->timer_expires)) {
> + if (!ws->timer_expires || time_after(expires, ws->timer_expires)) {
I've tried to follow your suggestion, so that __pm_wakeup_event() always
sets the timer, if msec is positive, or deactivates the wakeup source, if
msec is 0. Please let me know if that's what you wanted. :-)
Thanks,
Rafael
---
From: Rafael J. Wysocki <[email protected]>
Subject: PM / Sleep: Make __pm_stay_awake() delete wakeup source timers
If __pm_stay_awake() is called after __pm_wakeup_event() for the same
wakep source object before its timer expires, it won't cancel the
timer, so the wakeup source will be deactivated from the timer
function as scheduled by __pm_wakeup_event(). In that case
__pm_stay_awake() doesn't have any effect beyond incrementing
the wakeup source's event_count field, although it should cancel
the timer and make the wakeup source stay active until __pm_relax()
is called for it.
To fix this problem make __pm_stay_awake() delete the wakeup source's
timer and ensure that it won't be deactivated from the timer funtion
afterwards by clearing its timer_expires field.
Reported-by: Arve Hj?nnev?g <[email protected]>
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/base/power/wakeup.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
Index: linux/drivers/base/power/wakeup.c
===================================================================
--- linux.orig/drivers/base/power/wakeup.c
+++ linux/drivers/base/power/wakeup.c
@@ -344,7 +344,6 @@ static void wakeup_source_activate(struc
{
ws->active = true;
ws->active_count++;
- ws->timer_expires = jiffies;
ws->last_time = ktime_get();
/* Increment the counter of events in progress. */
@@ -365,9 +364,14 @@ void __pm_stay_awake(struct wakeup_sourc
return;
spin_lock_irqsave(&ws->lock, flags);
+
ws->event_count++;
if (!ws->active)
wakeup_source_activate(ws);
+
+ del_timer(&ws->timer);
+ ws->timer_expires = 0;
+
spin_unlock_irqrestore(&ws->lock, flags);
}
EXPORT_SYMBOL_GPL(__pm_stay_awake);
@@ -541,7 +545,7 @@ void __pm_wakeup_event(struct wakeup_sou
if (!expires)
expires = 1;
- if (time_after(expires, ws->timer_expires)) {
+ if (!ws->timer_expires || time_after(expires, ws->timer_expires)) {
mod_timer(&ws->timer, expires);
ws->timer_expires = expires;
}
2012/2/16 Rafael J. Wysocki <[email protected]>:
> On Thursday, February 16, 2012, Arve Hj?nnev?g wrote:
>> 2012/2/15 Rafael J. Wysocki <[email protected]>:
>> ...
>> > --- linux.orig/drivers/base/power/wakeup.c
>> > +++ linux/drivers/base/power/wakeup.c
>> > @@ -365,9 +365,15 @@ void __pm_stay_awake(struct wakeup_sourc
>> > ? ? ? ? ? ? ? ?return;
>> >
>> > ? ? ? ?spin_lock_irqsave(&ws->lock, flags);
>> > +
>> > + ? ? ? del_timer(&ws->timer);
>> > + ? ? ? ws->timer_expires = 0;
>>
>> timer_expires gets overwritten in wakeup_source_activate,
>
> I actually don't remember why it does that and with the $subject changes
> it's just pointless.
>
>> so __pm_relax followed by __pm_stay_awake is still not safe.
>
> Yes, I overlooked that timer_expires modification. ?Updated patch follows.
>
>> ...
>> > @@ -529,8 +535,12 @@ void __pm_wakeup_event(struct wakeup_sou
>> > ? ? ? ?spin_lock_irqsave(&ws->lock, flags);
>> >
>> > ? ? ? ?ws->event_count++;
>> > - ? ? ? if (!ws->active)
>> > + ? ? ? if (ws->active) {
>> > + ? ? ? ? ? ? ? if (!ws->timer_expires)
>> > + ? ? ? ? ? ? ? ? ? ? ? goto unlock;
>> > + ? ? ? } else {
>> > ? ? ? ? ? ? ? ?wakeup_source_activate(ws);
>> > + ? ? ? }
>> >
>> > ? ? ? ?if (!msec) {
>> > ? ? ? ? ? ? ? ?wakeup_source_deactivate(ws);
>> >
>>
>> I suggest dropping this and adding:
>
> Well, what exactly would you like to drop? ?The above proposed changes I guess?
Yes.
>
>> - ? ? ? if (time_after(expires, ws->timer_expires)) {
>> + ? ? ? if (!ws->timer_expires || time_after(expires, ws->timer_expires)) {
>
> I've tried to follow your suggestion, so that __pm_wakeup_event() always
> sets the timer, if msec is positive, or deactivates the wakeup source, if
> msec is 0. ?Please let me know if that's what you wanted. :-)
>
Yes. I should be able to replace a wake_lock with a single wakeup_source now.
--
Arve Hj?nnev?g
On Friday, February 17, 2012, Arve Hj?nnev?g wrote:
> 2012/2/16 Rafael J. Wysocki <[email protected]>:
> > On Thursday, February 16, 2012, Arve Hj?nnev?g wrote:
> >> 2012/2/15 Rafael J. Wysocki <[email protected]>:
> >> ...
> >> > --- linux.orig/drivers/base/power/wakeup.c
> >> > +++ linux/drivers/base/power/wakeup.c
> >> > @@ -365,9 +365,15 @@ void __pm_stay_awake(struct wakeup_sourc
> >> > return;
> >> >
> >> > spin_lock_irqsave(&ws->lock, flags);
> >> > +
> >> > + del_timer(&ws->timer);
> >> > + ws->timer_expires = 0;
> >>
> >> timer_expires gets overwritten in wakeup_source_activate,
> >
> > I actually don't remember why it does that and with the $subject changes
> > it's just pointless.
> >
> >> so __pm_relax followed by __pm_stay_awake is still not safe.
> >
> > Yes, I overlooked that timer_expires modification. Updated patch follows.
> >
> >> ...
> >> > @@ -529,8 +535,12 @@ void __pm_wakeup_event(struct wakeup_sou
> >> > spin_lock_irqsave(&ws->lock, flags);
> >> >
> >> > ws->event_count++;
> >> > - if (!ws->active)
> >> > + if (ws->active) {
> >> > + if (!ws->timer_expires)
> >> > + goto unlock;
> >> > + } else {
> >> > wakeup_source_activate(ws);
> >> > + }
> >> >
> >> > if (!msec) {
> >> > wakeup_source_deactivate(ws);
> >> >
> >>
> >> I suggest dropping this and adding:
> >
> > Well, what exactly would you like to drop? The above proposed changes I guess?
>
> Yes.
>
> >
> >> - if (time_after(expires, ws->timer_expires)) {
> >> + if (!ws->timer_expires || time_after(expires, ws->timer_expires)) {
> >
> > I've tried to follow your suggestion, so that __pm_wakeup_event() always
> > sets the timer, if msec is positive, or deactivates the wakeup source, if
> > msec is 0. Please let me know if that's what you wanted. :-)
> >
>
> Yes. I should be able to replace a wake_lock with a single wakeup_source now.
Good, thanks for the confirmation!
Rafael