user-space aborts suspend attempt if writing wakeup_count failed.
Count the write failure towards wakeup_count.
Signed-off-by: Jin Qian <[email protected]>
---
drivers/base/power/wakeup.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index f24c622..bdb45f3 100644
--- a/drivers/base/power/wakeup.c
+++ b/drivers/base/power/wakeup.c
@@ -57,6 +57,8 @@ static LIST_HEAD(wakeup_sources);
static DECLARE_WAIT_QUEUE_HEAD(wakeup_count_wait_queue);
+static ktime_t last_read_time;
+
/**
* wakeup_source_prepare - Prepare a new wakeup source for initialization.
* @ws: Wakeup source to prepare.
@@ -771,10 +773,15 @@ void pm_wakeup_clear(void)
bool pm_get_wakeup_count(unsigned int *count, bool block)
{
unsigned int cnt, inpr;
+ unsigned long flags;
if (block) {
DEFINE_WAIT(wait);
+ spin_lock_irqsave(&events_lock, flags);
+ last_read_time = ktime_get();
+ spin_unlock_irqrestore(&events_lock, flags);
+
for (;;) {
prepare_to_wait(&wakeup_count_wait_queue, &wait,
TASK_INTERRUPTIBLE);
@@ -806,6 +813,7 @@ bool pm_save_wakeup_count(unsigned int count)
{
unsigned int cnt, inpr;
unsigned long flags;
+ struct wakeup_source *ws;
events_check_enabled = false;
spin_lock_irqsave(&events_lock, flags);
@@ -813,6 +821,15 @@ bool pm_save_wakeup_count(unsigned int count)
if (cnt == count && inpr == 0) {
saved_count = count;
events_check_enabled = true;
+ } else {
+ rcu_read_lock();
+ list_for_each_entry_rcu(ws, &wakeup_sources, entry) {
+ if (ws->active ||
+ ktime_compare(ws->last_time, last_read_time) > 0) {
+ ws->wakeup_count++;
+ }
+ }
+ rcu_read_unlock();
}
spin_unlock_irqrestore(&events_lock, flags);
return events_check_enabled;
--
2.2.0.rc0.207.ga3a616c
On Wed 2015-04-22 17:50:11, Jin Qian wrote:
> user-space aborts suspend attempt if writing wakeup_count failed.
> Count the write failure towards wakeup_count.
Why should writing wakeup_count fail? Can you make sure it does not
happen, instead?
Pavel
> Signed-off-by: Jin Qian <[email protected]>
> ---
> drivers/base/power/wakeup.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> index f24c622..bdb45f3 100644
> --- a/drivers/base/power/wakeup.c
> +++ b/drivers/base/power/wakeup.c
> @@ -57,6 +57,8 @@ static LIST_HEAD(wakeup_sources);
>
> static DECLARE_WAIT_QUEUE_HEAD(wakeup_count_wait_queue);
>
> +static ktime_t last_read_time;
> +
> /**
> * wakeup_source_prepare - Prepare a new wakeup source for initialization.
> * @ws: Wakeup source to prepare.
> @@ -771,10 +773,15 @@ void pm_wakeup_clear(void)
> bool pm_get_wakeup_count(unsigned int *count, bool block)
> {
> unsigned int cnt, inpr;
> + unsigned long flags;
>
> if (block) {
> DEFINE_WAIT(wait);
>
> + spin_lock_irqsave(&events_lock, flags);
> + last_read_time = ktime_get();
> + spin_unlock_irqrestore(&events_lock, flags);
> +
> for (;;) {
> prepare_to_wait(&wakeup_count_wait_queue, &wait,
> TASK_INTERRUPTIBLE);
> @@ -806,6 +813,7 @@ bool pm_save_wakeup_count(unsigned int count)
> {
> unsigned int cnt, inpr;
> unsigned long flags;
> + struct wakeup_source *ws;
>
> events_check_enabled = false;
> spin_lock_irqsave(&events_lock, flags);
> @@ -813,6 +821,15 @@ bool pm_save_wakeup_count(unsigned int count)
> if (cnt == count && inpr == 0) {
> saved_count = count;
> events_check_enabled = true;
> + } else {
> + rcu_read_lock();
> + list_for_each_entry_rcu(ws, &wakeup_sources, entry) {
> + if (ws->active ||
> + ktime_compare(ws->last_time, last_read_time) > 0) {
> + ws->wakeup_count++;
> + }
> + }
> + rcu_read_unlock();
> }
> spin_unlock_irqrestore(&events_lock, flags);
> return events_check_enabled;
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Sorry about duplicate email. Sent in text mode so that it's delivered
to mailing list.
write fails when there's any new wakeup event after previous read.
https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-power
What: /sys/power/wakeup_count
Date: July 2010
Contact: Rafael J. Wysocki <[email protected]>
Description:
The /sys/power/wakeup_count file allows user space to put the
system into a sleep state while taking into account the
concurrent arrival of wakeup events. Reading from it returns
the current number of registered wakeup events and it blocks if
some wakeup events are being processed at the time the file is
read from. Writing to it will only succeed if the current
number of wakeup events is equal to the written value and, if
successful, will make the kernel abort a subsequent transition
to a sleep state if any wakeup events are reported after the
write has returned.
Thanks,
jin
On Mon, Apr 27, 2015 at 10:53 AM, Jin Qian <[email protected]> wrote:
> write fails when there's any new wakeup event after previous read.
>
> https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-power
>
> What: /sys/power/wakeup_count
> Date: July 2010
> Contact: Rafael J. Wysocki <[email protected]>
> Description:
> The /sys/power/wakeup_count file allows user space to put the
> system into a sleep state while taking into account the
> concurrent arrival of wakeup events. Reading from it returns
> the current number of registered wakeup events and it blocks if
> some wakeup events are being processed at the time the file is
> read from. Writing to it will only succeed if the current
> number of wakeup events is equal to the written value and, if
> successful, will make the kernel abort a subsequent transition
> to a sleep state if any wakeup events are reported after the
> write has returned.
>
>
> Thanks,
>
> jin
>
>
> On Sat, Apr 25, 2015 at 1:10 PM, Pavel Machek <[email protected]> wrote:
>>
>> On Wed 2015-04-22 17:50:11, Jin Qian wrote:
>> > user-space aborts suspend attempt if writing wakeup_count failed.
>> > Count the write failure towards wakeup_count.
>>
>> Why should writing wakeup_count fail? Can you make sure it does not
>> happen, instead?
>> Pavel
>>
>> > Signed-off-by: Jin Qian <[email protected]>
>> > ---
>> > drivers/base/power/wakeup.c | 17 +++++++++++++++++
>> > 1 file changed, 17 insertions(+)
>> >
>> > diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
>> > index f24c622..bdb45f3 100644
>> > --- a/drivers/base/power/wakeup.c
>> > +++ b/drivers/base/power/wakeup.c
>> > @@ -57,6 +57,8 @@ static LIST_HEAD(wakeup_sources);
>> >
>> > static DECLARE_WAIT_QUEUE_HEAD(wakeup_count_wait_queue);
>> >
>> > +static ktime_t last_read_time;
>> > +
>> > /**
>> > * wakeup_source_prepare - Prepare a new wakeup source for
>> > initialization.
>> > * @ws: Wakeup source to prepare.
>> > @@ -771,10 +773,15 @@ void pm_wakeup_clear(void)
>> > bool pm_get_wakeup_count(unsigned int *count, bool block)
>> > {
>> > unsigned int cnt, inpr;
>> > + unsigned long flags;
>> >
>> > if (block) {
>> > DEFINE_WAIT(wait);
>> >
>> > + spin_lock_irqsave(&events_lock, flags);
>> > + last_read_time = ktime_get();
>> > + spin_unlock_irqrestore(&events_lock, flags);
>> > +
>> > for (;;) {
>> > prepare_to_wait(&wakeup_count_wait_queue, &wait,
>> > TASK_INTERRUPTIBLE);
>> > @@ -806,6 +813,7 @@ bool pm_save_wakeup_count(unsigned int count)
>> > {
>> > unsigned int cnt, inpr;
>> > unsigned long flags;
>> > + struct wakeup_source *ws;
>> >
>> > events_check_enabled = false;
>> > spin_lock_irqsave(&events_lock, flags);
>> > @@ -813,6 +821,15 @@ bool pm_save_wakeup_count(unsigned int count)
>> > if (cnt == count && inpr == 0) {
>> > saved_count = count;
>> > events_check_enabled = true;
>> > + } else {
>> > + rcu_read_lock();
>> > + list_for_each_entry_rcu(ws, &wakeup_sources, entry) {
>> > + if (ws->active ||
>> > + ktime_compare(ws->last_time, last_read_time) >
>> > 0) {
>> > + ws->wakeup_count++;
>> > + }
>> > + }
>> > + rcu_read_unlock();
>> > }
>> > spin_unlock_irqrestore(&events_lock, flags);
>> > return events_check_enabled;
>>
>> --
>> (english) http://www.livejournal.com/~pavelmachek
>> (cesky, pictures)
>> http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
>
>
On Wednesday, April 22, 2015 05:50:11 PM Jin Qian wrote:
> user-space aborts suspend attempt if writing wakeup_count failed.
> Count the write failure towards wakeup_count.
A use case, please?
> Signed-off-by: Jin Qian <[email protected]>
> ---
> drivers/base/power/wakeup.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> index f24c622..bdb45f3 100644
> --- a/drivers/base/power/wakeup.c
> +++ b/drivers/base/power/wakeup.c
> @@ -57,6 +57,8 @@ static LIST_HEAD(wakeup_sources);
>
> static DECLARE_WAIT_QUEUE_HEAD(wakeup_count_wait_queue);
>
> +static ktime_t last_read_time;
> +
> /**
> * wakeup_source_prepare - Prepare a new wakeup source for initialization.
> * @ws: Wakeup source to prepare.
> @@ -771,10 +773,15 @@ void pm_wakeup_clear(void)
> bool pm_get_wakeup_count(unsigned int *count, bool block)
> {
> unsigned int cnt, inpr;
> + unsigned long flags;
>
> if (block) {
> DEFINE_WAIT(wait);
>
> + spin_lock_irqsave(&events_lock, flags);
> + last_read_time = ktime_get();
> + spin_unlock_irqrestore(&events_lock, flags);
> +
> for (;;) {
> prepare_to_wait(&wakeup_count_wait_queue, &wait,
> TASK_INTERRUPTIBLE);
> @@ -806,6 +813,7 @@ bool pm_save_wakeup_count(unsigned int count)
> {
> unsigned int cnt, inpr;
> unsigned long flags;
> + struct wakeup_source *ws;
>
> events_check_enabled = false;
> spin_lock_irqsave(&events_lock, flags);
> @@ -813,6 +821,15 @@ bool pm_save_wakeup_count(unsigned int count)
> if (cnt == count && inpr == 0) {
> saved_count = count;
> events_check_enabled = true;
> + } else {
> + rcu_read_lock();
> + list_for_each_entry_rcu(ws, &wakeup_sources, entry) {
> + if (ws->active ||
> + ktime_compare(ws->last_time, last_read_time) > 0) {
> + ws->wakeup_count++;
> + }
> + }
> + rcu_read_unlock();
> }
> spin_unlock_irqrestore(&events_lock, flags);
> return events_check_enabled;
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
Some wakeup event happens every frequently between reading
wakeup_count and writing back wakeup_count.
They changed wakeup event count so writes fail and usespace doesn't
continue to suspend. However, such
occurrences are not counted in ws->wakeup_count. I spent quite
sometime finding out the problematic wakeup
event with inaccurate wakeup_count : )
Thanks,
jin
On Fri, May 15, 2015 at 5:34 PM, Rafael J. Wysocki <[email protected]> wrote:
> On Wednesday, April 22, 2015 05:50:11 PM Jin Qian wrote:
>> user-space aborts suspend attempt if writing wakeup_count failed.
>> Count the write failure towards wakeup_count.
>
> A use case, please?
>
>> Signed-off-by: Jin Qian <[email protected]>
>> ---
>> drivers/base/power/wakeup.c | 17 +++++++++++++++++
>> 1 file changed, 17 insertions(+)
>>
>> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
>> index f24c622..bdb45f3 100644
>> --- a/drivers/base/power/wakeup.c
>> +++ b/drivers/base/power/wakeup.c
>> @@ -57,6 +57,8 @@ static LIST_HEAD(wakeup_sources);
>>
>> static DECLARE_WAIT_QUEUE_HEAD(wakeup_count_wait_queue);
>>
>> +static ktime_t last_read_time;
>> +
>> /**
>> * wakeup_source_prepare - Prepare a new wakeup source for initialization.
>> * @ws: Wakeup source to prepare.
>> @@ -771,10 +773,15 @@ void pm_wakeup_clear(void)
>> bool pm_get_wakeup_count(unsigned int *count, bool block)
>> {
>> unsigned int cnt, inpr;
>> + unsigned long flags;
>>
>> if (block) {
>> DEFINE_WAIT(wait);
>>
>> + spin_lock_irqsave(&events_lock, flags);
>> + last_read_time = ktime_get();
>> + spin_unlock_irqrestore(&events_lock, flags);
>> +
>> for (;;) {
>> prepare_to_wait(&wakeup_count_wait_queue, &wait,
>> TASK_INTERRUPTIBLE);
>> @@ -806,6 +813,7 @@ bool pm_save_wakeup_count(unsigned int count)
>> {
>> unsigned int cnt, inpr;
>> unsigned long flags;
>> + struct wakeup_source *ws;
>>
>> events_check_enabled = false;
>> spin_lock_irqsave(&events_lock, flags);
>> @@ -813,6 +821,15 @@ bool pm_save_wakeup_count(unsigned int count)
>> if (cnt == count && inpr == 0) {
>> saved_count = count;
>> events_check_enabled = true;
>> + } else {
>> + rcu_read_lock();
>> + list_for_each_entry_rcu(ws, &wakeup_sources, entry) {
>> + if (ws->active ||
>> + ktime_compare(ws->last_time, last_read_time) > 0) {
>> + ws->wakeup_count++;
>> + }
>> + }
>> + rcu_read_unlock();
>> }
>> spin_unlock_irqrestore(&events_lock, flags);
>> return events_check_enabled;
>>
>
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.
On Friday, May 15, 2015 06:08:32 PM Jin Qian wrote:
> Some wakeup event happens every frequently between reading
> wakeup_count and writing back wakeup_count.
> They changed wakeup event count so writes fail and usespace doesn't
> continue to suspend. However, such
> occurrences are not counted in ws->wakeup_count.
So I hope you've read the comment in wakeup_source_report_event().
> I spent quite sometime finding out the problematic wakeup
> event with inaccurate wakeup_count : )
But your patch doesn't fix the problem. ws->wakeup_count is still racy
and kind of even more after the patch than before.
> On Fri, May 15, 2015 at 5:34 PM, Rafael J. Wysocki <[email protected]> wrote:
> > On Wednesday, April 22, 2015 05:50:11 PM Jin Qian wrote:
> >> user-space aborts suspend attempt if writing wakeup_count failed.
> >> Count the write failure towards wakeup_count.
> >
> > A use case, please?
> >
> >> Signed-off-by: Jin Qian <[email protected]>
> >> ---
> >> drivers/base/power/wakeup.c | 17 +++++++++++++++++
> >> 1 file changed, 17 insertions(+)
> >>
> >> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> >> index f24c622..bdb45f3 100644
> >> --- a/drivers/base/power/wakeup.c
> >> +++ b/drivers/base/power/wakeup.c
> >> @@ -57,6 +57,8 @@ static LIST_HEAD(wakeup_sources);
> >>
> >> static DECLARE_WAIT_QUEUE_HEAD(wakeup_count_wait_queue);
> >>
> >> +static ktime_t last_read_time;
> >> +
> >> /**
> >> * wakeup_source_prepare - Prepare a new wakeup source for initialization.
> >> * @ws: Wakeup source to prepare.
> >> @@ -771,10 +773,15 @@ void pm_wakeup_clear(void)
> >> bool pm_get_wakeup_count(unsigned int *count, bool block)
> >> {
> >> unsigned int cnt, inpr;
> >> + unsigned long flags;
> >>
> >> if (block) {
> >> DEFINE_WAIT(wait);
> >>
> >> + spin_lock_irqsave(&events_lock, flags);
> >> + last_read_time = ktime_get();
> >> + spin_unlock_irqrestore(&events_lock, flags);
> >> +
> >> for (;;) {
> >> prepare_to_wait(&wakeup_count_wait_queue, &wait,
> >> TASK_INTERRUPTIBLE);
> >> @@ -806,6 +813,7 @@ bool pm_save_wakeup_count(unsigned int count)
> >> {
> >> unsigned int cnt, inpr;
> >> unsigned long flags;
> >> + struct wakeup_source *ws;
> >>
> >> events_check_enabled = false;
> >> spin_lock_irqsave(&events_lock, flags);
> >> @@ -813,6 +821,15 @@ bool pm_save_wakeup_count(unsigned int count)
> >> if (cnt == count && inpr == 0) {
> >> saved_count = count;
> >> events_check_enabled = true;
> >> + } else {
> >> + rcu_read_lock();
> >> + list_for_each_entry_rcu(ws, &wakeup_sources, entry) {
> >> + if (ws->active ||
> >> + ktime_compare(ws->last_time, last_read_time) > 0) {
> >> + ws->wakeup_count++;
> >> + }
> >> + }
> >> + rcu_read_unlock();
> >> }
> >> spin_unlock_irqrestore(&events_lock, flags);
> >> return events_check_enabled;
> >>
> >
> > --
> > I speak only for myself.
> > Rafael J. Wysocki, Intel Open Source Technology Center.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.