* [PATCH 1/2] PM / wakeup: Remove timer from wakeup_source_remove()
@ 2019-03-08 9:53 Viresh Kumar
2019-03-08 9:53 ` [PATCH 2/2] PM / wakeup: Clear timer.function in wakeup_source_remove() Viresh Kumar
2019-03-11 12:05 ` [PATCH 1/2] PM / wakeup: Remove timer from wakeup_source_remove() Rafael J. Wysocki
0 siblings, 2 replies; 7+ messages in thread
From: Viresh Kumar @ 2019-03-08 9:53 UTC (permalink / raw)
To: Rafael Wysocki, Pavel Machek, Len Brown
Cc: Viresh Kumar, linux-pm, Vincent Guittot, sunzhaosheng,
jean.xupeng, yuwei3, gengyanping, peter.panshilin, linux-kernel
wakeup_source_remove() is the counterpart of wakeup_source_add() helper
and must undo the initializations done by wakeup_source_add(). Currently
the timer is initialized by wakeup_source_add() but removed from
wakeup_source_drop(), which doesn't look logically correct. Also it
should be okay to call wakeup_source_add() right after calling
wakeup_source_remove(), and in that case we may end up calling
timer_setup() for a potentially scheduled timer which is surely
incorrect.
Move the timer removal part to wakeup_source_remove() instead.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/base/power/wakeup.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index f1fee72ed970..18333962e3da 100644
--- a/drivers/base/power/wakeup.c
+++ b/drivers/base/power/wakeup.c
@@ -118,7 +118,6 @@ void wakeup_source_drop(struct wakeup_source *ws)
if (!ws)
return;
- del_timer_sync(&ws->timer);
__pm_relax(ws);
}
EXPORT_SYMBOL_GPL(wakeup_source_drop);
@@ -205,6 +204,8 @@ void wakeup_source_remove(struct wakeup_source *ws)
list_del_rcu(&ws->entry);
raw_spin_unlock_irqrestore(&events_lock, flags);
synchronize_srcu(&wakeup_srcu);
+
+ del_timer_sync(&ws->timer);
}
EXPORT_SYMBOL_GPL(wakeup_source_remove);
--
2.21.0.rc0.269.g1a574e7a288b
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] PM / wakeup: Clear timer.function in wakeup_source_remove()
2019-03-08 9:53 [PATCH 1/2] PM / wakeup: Remove timer from wakeup_source_remove() Viresh Kumar
@ 2019-03-08 9:53 ` Viresh Kumar
2019-03-11 12:05 ` [PATCH 1/2] PM / wakeup: Remove timer from wakeup_source_remove() Rafael J. Wysocki
1 sibling, 0 replies; 7+ messages in thread
From: Viresh Kumar @ 2019-03-08 9:53 UTC (permalink / raw)
To: Rafael Wysocki, Pavel Machek, Len Brown
Cc: Viresh Kumar, linux-pm, Vincent Guittot, sunzhaosheng,
jean.xupeng, yuwei3, gengyanping, peter.panshilin, linux-kernel
wakeup_source_activate() performs a check to see if the wakeup source is
registered or not. It works fine for a newly added wakeup source which
may not have been registered but fails to catch the case where a wakeup
source is unregistered as the timer.function is still valid.
Fix it by setting the timer.function to NULL from
wakeup_source_remove().
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/base/power/wakeup.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index 18333962e3da..3699faca24af 100644
--- a/drivers/base/power/wakeup.c
+++ b/drivers/base/power/wakeup.c
@@ -206,6 +206,13 @@ void wakeup_source_remove(struct wakeup_source *ws)
synchronize_srcu(&wakeup_srcu);
del_timer_sync(&ws->timer);
+
+ /*
+ * Clear timer.function so that wakeup_source_not_registered() can
+ * detect an unregistered wakeup source.
+ */
+ ws->timer.function = NULL;
+
}
EXPORT_SYMBOL_GPL(wakeup_source_remove);
--
2.21.0.rc0.269.g1a574e7a288b
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] PM / wakeup: Remove timer from wakeup_source_remove()
2019-03-08 9:53 [PATCH 1/2] PM / wakeup: Remove timer from wakeup_source_remove() Viresh Kumar
2019-03-08 9:53 ` [PATCH 2/2] PM / wakeup: Clear timer.function in wakeup_source_remove() Viresh Kumar
@ 2019-03-11 12:05 ` Rafael J. Wysocki
2019-03-12 3:28 ` Viresh Kumar
1 sibling, 1 reply; 7+ messages in thread
From: Rafael J. Wysocki @ 2019-03-11 12:05 UTC (permalink / raw)
To: Viresh Kumar
Cc: Pavel Machek, Len Brown, linux-pm, Vincent Guittot, sunzhaosheng,
jean.xupeng, yuwei3, gengyanping, peter.panshilin, linux-kernel
On Friday, March 8, 2019 10:53:11 AM CET Viresh Kumar wrote:
> wakeup_source_remove() is the counterpart of wakeup_source_add() helper
> and must undo the initializations done by wakeup_source_add(). Currently
> the timer is initialized by wakeup_source_add() but removed from
> wakeup_source_drop(), which doesn't look logically correct. Also it
> should be okay to call wakeup_source_add() right after calling
> wakeup_source_remove(), and in that case we may end up calling
> timer_setup() for a potentially scheduled timer which is surely
> incorrect.
>
> Move the timer removal part to wakeup_source_remove() instead.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> drivers/base/power/wakeup.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> index f1fee72ed970..18333962e3da 100644
> --- a/drivers/base/power/wakeup.c
> +++ b/drivers/base/power/wakeup.c
> @@ -118,7 +118,6 @@ void wakeup_source_drop(struct wakeup_source *ws)
> if (!ws)
> return;
>
> - del_timer_sync(&ws->timer);
> __pm_relax(ws);
> }
> EXPORT_SYMBOL_GPL(wakeup_source_drop);
> @@ -205,6 +204,8 @@ void wakeup_source_remove(struct wakeup_source *ws)
> list_del_rcu(&ws->entry);
> raw_spin_unlock_irqrestore(&events_lock, flags);
> synchronize_srcu(&wakeup_srcu);
> +
> + del_timer_sync(&ws->timer);
> }
> EXPORT_SYMBOL_GPL(wakeup_source_remove);
>
>
I've merged it with the [2/2], rewritten the subject and changelog and
queued the result as commit d856f39ac1cc ("PM / wakeup: Rework wakeup
source timer cancellation").
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] PM / wakeup: Remove timer from wakeup_source_remove()
2019-03-11 12:05 ` [PATCH 1/2] PM / wakeup: Remove timer from wakeup_source_remove() Rafael J. Wysocki
@ 2019-03-12 3:28 ` Viresh Kumar
2019-03-12 9:03 ` Rafael J. Wysocki
2019-03-12 9:04 ` Pavel Machek
0 siblings, 2 replies; 7+ messages in thread
From: Viresh Kumar @ 2019-03-12 3:28 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Pavel Machek, Len Brown, linux-pm, Vincent Guittot, sunzhaosheng,
jean.xupeng, yuwei3, gengyanping, peter.panshilin, linux-kernel
On 11-03-19, 13:05, Rafael J. Wysocki wrote:
> On Friday, March 8, 2019 10:53:11 AM CET Viresh Kumar wrote:
> > wakeup_source_remove() is the counterpart of wakeup_source_add() helper
> > and must undo the initializations done by wakeup_source_add(). Currently
> > the timer is initialized by wakeup_source_add() but removed from
> > wakeup_source_drop(), which doesn't look logically correct. Also it
> > should be okay to call wakeup_source_add() right after calling
> > wakeup_source_remove(), and in that case we may end up calling
> > timer_setup() for a potentially scheduled timer which is surely
> > incorrect.
> >
> > Move the timer removal part to wakeup_source_remove() instead.
> >
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> > drivers/base/power/wakeup.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> > index f1fee72ed970..18333962e3da 100644
> > --- a/drivers/base/power/wakeup.c
> > +++ b/drivers/base/power/wakeup.c
> > @@ -118,7 +118,6 @@ void wakeup_source_drop(struct wakeup_source *ws)
> > if (!ws)
> > return;
> >
> > - del_timer_sync(&ws->timer);
> > __pm_relax(ws);
> > }
> > EXPORT_SYMBOL_GPL(wakeup_source_drop);
> > @@ -205,6 +204,8 @@ void wakeup_source_remove(struct wakeup_source *ws)
> > list_del_rcu(&ws->entry);
> > raw_spin_unlock_irqrestore(&events_lock, flags);
> > synchronize_srcu(&wakeup_srcu);
> > +
> > + del_timer_sync(&ws->timer);
> > }
> > EXPORT_SYMBOL_GPL(wakeup_source_remove);
> >
> >
>
> I've merged it with the [2/2], rewritten the subject and changelog and
> queued the result as commit d856f39ac1cc ("PM / wakeup: Rework wakeup
> source timer cancellation").
Okay, thanks. We (Android guys) want this to be backported into 4.4+
kernels via the stable tree. Can we mark this for stable in the commit
itself ? Else I would be required to send this separately for all the
kernels. I should have marked it for stable initially though, sorry
about forgetting then.
--
viresh
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] PM / wakeup: Remove timer from wakeup_source_remove()
2019-03-12 3:28 ` Viresh Kumar
@ 2019-03-12 9:03 ` Rafael J. Wysocki
2019-03-12 9:04 ` Pavel Machek
1 sibling, 0 replies; 7+ messages in thread
From: Rafael J. Wysocki @ 2019-03-12 9:03 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael J. Wysocki, Pavel Machek, Len Brown, Linux PM,
Vincent Guittot, Zhaosheng Sun, jean.xupeng, yuwei3, gengyanping,
peter.panshilin, Linux Kernel Mailing List
On Tue, Mar 12, 2019 at 4:28 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 11-03-19, 13:05, Rafael J. Wysocki wrote:
> > On Friday, March 8, 2019 10:53:11 AM CET Viresh Kumar wrote:
> > > wakeup_source_remove() is the counterpart of wakeup_source_add() helper
> > > and must undo the initializations done by wakeup_source_add(). Currently
> > > the timer is initialized by wakeup_source_add() but removed from
> > > wakeup_source_drop(), which doesn't look logically correct. Also it
> > > should be okay to call wakeup_source_add() right after calling
> > > wakeup_source_remove(), and in that case we may end up calling
> > > timer_setup() for a potentially scheduled timer which is surely
> > > incorrect.
> > >
> > > Move the timer removal part to wakeup_source_remove() instead.
> > >
> > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > > ---
> > > drivers/base/power/wakeup.c | 3 ++-
> > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> > > index f1fee72ed970..18333962e3da 100644
> > > --- a/drivers/base/power/wakeup.c
> > > +++ b/drivers/base/power/wakeup.c
> > > @@ -118,7 +118,6 @@ void wakeup_source_drop(struct wakeup_source *ws)
> > > if (!ws)
> > > return;
> > >
> > > - del_timer_sync(&ws->timer);
> > > __pm_relax(ws);
> > > }
> > > EXPORT_SYMBOL_GPL(wakeup_source_drop);
> > > @@ -205,6 +204,8 @@ void wakeup_source_remove(struct wakeup_source *ws)
> > > list_del_rcu(&ws->entry);
> > > raw_spin_unlock_irqrestore(&events_lock, flags);
> > > synchronize_srcu(&wakeup_srcu);
> > > +
> > > + del_timer_sync(&ws->timer);
> > > }
> > > EXPORT_SYMBOL_GPL(wakeup_source_remove);
> > >
> > >
> >
> > I've merged it with the [2/2], rewritten the subject and changelog and
> > queued the result as commit d856f39ac1cc ("PM / wakeup: Rework wakeup
> > source timer cancellation").
>
> Okay, thanks. We (Android guys) want this to be backported into 4.4+
> kernels via the stable tree. Can we mark this for stable in the commit
> itself ? Else I would be required to send this separately for all the
> kernels. I should have marked it for stable initially though, sorry
> about forgetting then.
Queued up with a CC-stable tag for 4.4 an later, thanks!
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] PM / wakeup: Remove timer from wakeup_source_remove()
2019-03-12 3:28 ` Viresh Kumar
2019-03-12 9:03 ` Rafael J. Wysocki
@ 2019-03-12 9:04 ` Pavel Machek
2019-03-12 11:41 ` Viresh Kumar
1 sibling, 1 reply; 7+ messages in thread
From: Pavel Machek @ 2019-03-12 9:04 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael J. Wysocki, Len Brown, linux-pm, Vincent Guittot,
sunzhaosheng, jean.xupeng, yuwei3, gengyanping, peter.panshilin,
linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1637 bytes --]
On Tue 2019-03-12 08:58:02, Viresh Kumar wrote:
> On 11-03-19, 13:05, Rafael J. Wysocki wrote:
> > On Friday, March 8, 2019 10:53:11 AM CET Viresh Kumar wrote:
> > > wakeup_source_remove() is the counterpart of wakeup_source_add() helper
> > > and must undo the initializations done by wakeup_source_add(). Currently
> > > the timer is initialized by wakeup_source_add() but removed from
> > > wakeup_source_drop(), which doesn't look logically correct. Also it
> > > should be okay to call wakeup_source_add() right after calling
> > > wakeup_source_remove(), and in that case we may end up calling
> > > timer_setup() for a potentially scheduled timer which is surely
> > > incorrect.
> > >
> > > Move the timer removal part to wakeup_source_remove() instead.
> >
> > I've merged it with the [2/2], rewritten the subject and changelog and
> > queued the result as commit d856f39ac1cc ("PM / wakeup: Rework wakeup
> > source timer cancellation").
>
> Okay, thanks. We (Android guys) want this to be backported into 4.4+
> kernels via the stable tree. Can we mark this for stable in the commit
> itself ? Else I would be required to send this separately for all the
> kernels. I should have marked it for stable initially though, sorry
> about forgetting then.
Greg is normally pretty agressive about backporting everything
remotely looking like a fix...
But better changelog would help. How is the bug actually affecting
users?
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] PM / wakeup: Remove timer from wakeup_source_remove()
2019-03-12 9:04 ` Pavel Machek
@ 2019-03-12 11:41 ` Viresh Kumar
0 siblings, 0 replies; 7+ messages in thread
From: Viresh Kumar @ 2019-03-12 11:41 UTC (permalink / raw)
To: Pavel Machek
Cc: Rafael J. Wysocki, Len Brown, linux-pm, Vincent Guittot,
sunzhaosheng, jean.xupeng, yuwei3, gengyanping, peter.panshilin,
linux-kernel
On 12-03-19, 10:04, Pavel Machek wrote:
> On Tue 2019-03-12 08:58:02, Viresh Kumar wrote:
> > On 11-03-19, 13:05, Rafael J. Wysocki wrote:
> > > On Friday, March 8, 2019 10:53:11 AM CET Viresh Kumar wrote:
> > > > wakeup_source_remove() is the counterpart of wakeup_source_add() helper
> > > > and must undo the initializations done by wakeup_source_add(). Currently
> > > > the timer is initialized by wakeup_source_add() but removed from
> > > > wakeup_source_drop(), which doesn't look logically correct. Also it
> > > > should be okay to call wakeup_source_add() right after calling
> > > > wakeup_source_remove(), and in that case we may end up calling
> > > > timer_setup() for a potentially scheduled timer which is surely
> > > > incorrect.
> > > >
> > > > Move the timer removal part to wakeup_source_remove() instead.
>
> > >
> > > I've merged it with the [2/2], rewritten the subject and changelog and
> > > queued the result as commit d856f39ac1cc ("PM / wakeup: Rework wakeup
> > > source timer cancellation").
> >
> > Okay, thanks. We (Android guys) want this to be backported into 4.4+
> > kernels via the stable tree. Can we mark this for stable in the commit
> > itself ? Else I would be required to send this separately for all the
> > kernels. I should have marked it for stable initially though, sorry
> > about forgetting then.
>
> Greg is normally pretty agressive about backporting everything
> remotely looking like a fix...
>
> But better changelog would help. How is the bug actually affecting
> users?
The Android wakelock infrastructure (not in mainline) is based on the
wakeup sources stuff and that is where we found the issue. A wakelock
was taken due to a bug in user driver and the board never attempts
going into suspend anymore.
If this patch doesn't go into the stable kernels, I would be required
to send this separately for Android kernels and the Android guys will
suggest getting it via stable tree.
And because this fixes a potential issue it was worth trying getting
it via the stable kernel :)
--
viresh
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-03-12 11:41 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-08 9:53 [PATCH 1/2] PM / wakeup: Remove timer from wakeup_source_remove() Viresh Kumar
2019-03-08 9:53 ` [PATCH 2/2] PM / wakeup: Clear timer.function in wakeup_source_remove() Viresh Kumar
2019-03-11 12:05 ` [PATCH 1/2] PM / wakeup: Remove timer from wakeup_source_remove() Rafael J. Wysocki
2019-03-12 3:28 ` Viresh Kumar
2019-03-12 9:03 ` Rafael J. Wysocki
2019-03-12 9:04 ` Pavel Machek
2019-03-12 11:41 ` Viresh Kumar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).