[PATCH 1/3] power: validate wakeup source before activating it.

[PATCH 1/3] power: validate wakeup source before activating it.

[Jin Qian]

A rogue wakeup source not registered in wakeup_sources list is not visible
from wakeup_sources_stats_show. Check if the wakeup source is registered
properly by looking at the timer function.

Signed-off-by: Jin Qian <jinqian@android.com>
---
 drivers/base/power/wakeup.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index 7726200..f24c622 100644
--- a/drivers/base/power/wakeup.c
+++ b/drivers/base/power/wakeup.c
@@ -14,6 +14,7 @@
 #include <linux/suspend.h>
 #include <linux/seq_file.h>
 #include <linux/debugfs.h>
+#include <linux/types.h>
 #include <trace/events/power.h>
 
 #include "power.h"
@@ -351,6 +352,19 @@ int device_set_wakeup_enable(struct device *dev, bool enable)
 }
 EXPORT_SYMBOL_GPL(device_set_wakeup_enable);
 
+/**
+ * validate_wakeup_source - validate the given wakeup source.
+ * @ws: Wakeup source to be validated.
+ */
+static bool validate_wakeup_source(struct wakeup_source *ws)
+{
+	/*
+	 * Use timer function to check if the given source is initialized
+	 * by wakeup_source_add.
+	 */
+	return ws->timer.function == pm_wakeup_timer_fn;
+}
+
 /*
  * The functions below use the observation that each wakeup event starts a
  * period in which the system should not be suspended.  The moment this period
@@ -391,6 +405,11 @@ static void wakeup_source_activate(struct wakeup_source *ws)
 {
 	unsigned int cec;
 
+	if (!validate_wakeup_source(ws)) {
+		pr_err("unregistered wakeup source: %s\n", ws->name);
+		return;
+	}
+
 	/*
 	 * active wakeup source should bring the system
 	 * out of PM_SUSPEND_FREEZE state
-- 
2.2.0.rc0.207.ga3a616c

Re: [PATCH 1/3] power: validate wakeup source before activating it.

[Pavel Machek]

On Wed 2015-04-22 17:50:10, Jin Qian wrote:
> A rogue wakeup source not registered in wakeup_sources list is not visible
> from wakeup_sources_stats_show. Check if the wakeup source is registered
> properly by looking at the timer function.

How often does that happen?

Will people on your systems actually notice that pr_err()?
								Pavel

> Signed-off-by: Jin Qian <jinqian@android.com>
> ---
>  drivers/base/power/wakeup.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> index 7726200..f24c622 100644
> --- a/drivers/base/power/wakeup.c
> +++ b/drivers/base/power/wakeup.c
> @@ -14,6 +14,7 @@
>  #include <linux/suspend.h>
>  #include <linux/seq_file.h>
>  #include <linux/debugfs.h>
> +#include <linux/types.h>
>  #include <trace/events/power.h>
>  
>  #include "power.h"
> @@ -351,6 +352,19 @@ int device_set_wakeup_enable(struct device *dev, bool enable)
>  }
>  EXPORT_SYMBOL_GPL(device_set_wakeup_enable);
>  
> +/**
> + * validate_wakeup_source - validate the given wakeup source.
> + * @ws: Wakeup source to be validated.
> + */
> +static bool validate_wakeup_source(struct wakeup_source *ws)
> +{
> +	/*
> +	 * Use timer function to check if the given source is initialized
> +	 * by wakeup_source_add.
> +	 */
> +	return ws->timer.function == pm_wakeup_timer_fn;
> +}
> +
>  /*
>   * The functions below use the observation that each wakeup event starts a
>   * period in which the system should not be suspended.  The moment this period
> @@ -391,6 +405,11 @@ static void wakeup_source_activate(struct wakeup_source *ws)
>  {
>  	unsigned int cec;
>  
> +	if (!validate_wakeup_source(ws)) {
> +		pr_err("unregistered wakeup source: %s\n", ws->name);
> +		return;
> +	}
> +
>  	/*
>  	 * active wakeup source should bring the system
>  	 * out of PM_SUSPEND_FREEZE state

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 1/3] power: validate wakeup source before activating it.

[Jin Qian]

This is a rare case.

My thought is to prevent a rogue wakeup source from preventing system
suspend, which is very difficult to find out if the wakeup source is
not registered and not visible in /d/wakeup_sources. On the other
hand, if the wakeup source failed to activate, it's much easier to be
noticed considering we deal with particular wakeup source. I can make
the pr_err BUG_ON or WARN_ON if they're more noticeable.

Thanks,
jin

On Sat, Apr 25, 2015 at 1:12 PM, Pavel Machek <pavel@ucw.cz> wrote:
> On Wed 2015-04-22 17:50:10, Jin Qian wrote:
>> A rogue wakeup source not registered in wakeup_sources list is not visible
>> from wakeup_sources_stats_show. Check if the wakeup source is registered
>> properly by looking at the timer function.
>
> How often does that happen?
>
> Will people on your systems actually notice that pr_err()?
>                                                                 Pavel
>
>> Signed-off-by: Jin Qian <jinqian@android.com>
>> ---
>>  drivers/base/power/wakeup.c | 19 +++++++++++++++++++
>>  1 file changed, 19 insertions(+)
>>
>> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
>> index 7726200..f24c622 100644
>> --- a/drivers/base/power/wakeup.c
>> +++ b/drivers/base/power/wakeup.c
>> @@ -14,6 +14,7 @@
>>  #include <linux/suspend.h>
>>  #include <linux/seq_file.h>
>>  #include <linux/debugfs.h>
>> +#include <linux/types.h>
>>  #include <trace/events/power.h>
>>
>>  #include "power.h"
>> @@ -351,6 +352,19 @@ int device_set_wakeup_enable(struct device *dev, bool enable)
>>  }
>>  EXPORT_SYMBOL_GPL(device_set_wakeup_enable);
>>
>> +/**
>> + * validate_wakeup_source - validate the given wakeup source.
>> + * @ws: Wakeup source to be validated.
>> + */
>> +static bool validate_wakeup_source(struct wakeup_source *ws)
>> +{
>> +     /*
>> +      * Use timer function to check if the given source is initialized
>> +      * by wakeup_source_add.
>> +      */
>> +     return ws->timer.function == pm_wakeup_timer_fn;
>> +}
>> +
>>  /*
>>   * The functions below use the observation that each wakeup event starts a
>>   * period in which the system should not be suspended.  The moment this period
>> @@ -391,6 +405,11 @@ static void wakeup_source_activate(struct wakeup_source *ws)
>>  {
>>       unsigned int cec;
>>
>> +     if (!validate_wakeup_source(ws)) {
>> +             pr_err("unregistered wakeup source: %s\n", ws->name);
>> +             return;
>> +     }
>> +
>>       /*
>>        * active wakeup source should bring the system
>>        * out of PM_SUSPEND_FREEZE state
>
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCH 2/3] power: increment wakeup_count when save_wakeup_count failed.

[Jin Qian]

user-space aborts suspend attempt if writing wakeup_count failed.
Count the write failure towards wakeup_count.

Signed-off-by: Jin Qian <jinqian@android.com>
---
 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

Re: [PATCH 2/3] power: increment wakeup_count when save_wakeup_count failed.

[Pavel Machek]

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 <jinqian@android.com>
> ---
>  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

Re: [PATCH 2/3] power: increment wakeup_count when save_wakeup_count failed.

[Jin Qian]

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 <rjw@rjwysocki.net>
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 <jinqian@android.com> 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 <rjw@rjwysocki.net>
> 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 <pavel@ucw.cz> 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 <jinqian@android.com>
>> > ---
>> >  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
>
>

Re: [PATCH 2/3] power: increment wakeup_count when save_wakeup_count failed.

[Rafael J. Wysocki]

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 <jinqian@android.com>
> ---
>  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.

Re: [PATCH 2/3] power: increment wakeup_count when save_wakeup_count failed.

[Jin Qian]

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 <rjw@rjwysocki.net> 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 <jinqian@android.com>
>> ---
>>  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.

Re: [PATCH 2/3] power: increment wakeup_count when save_wakeup_count failed.

[Rafael J. Wysocki]

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 <rjw@rjwysocki.net> 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 <jinqian@android.com>
> >> ---
> >>  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 majordomo@vger.kernel.org
> 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.

[PATCH 3/3] power: add a dummy wakeup_source to record statistics

[Jin Qian]

After a wakeup_source is destroyed, we lost all information such as how
long this wakeup_source has been active. Add a dummy wakeup_source to
record such info.

Signed-off-by: Jin Qian <jinqian@android.com>
---
 drivers/base/power/wakeup.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index bdb45f3..732683c 100644
--- a/drivers/base/power/wakeup.c
+++ b/drivers/base/power/wakeup.c
@@ -59,6 +59,11 @@ static DECLARE_WAIT_QUEUE_HEAD(wakeup_count_wait_queue);
 
 static ktime_t last_read_time;
 
+static struct wakeup_source deleted_ws = {
+	.name = "deleted",
+	.lock =  __SPIN_LOCK_UNLOCKED(deleted_ws.lock),
+};
+
 /**
  * wakeup_source_prepare - Prepare a new wakeup source for initialization.
  * @ws: Wakeup source to prepare.
@@ -110,6 +115,33 @@ void wakeup_source_drop(struct wakeup_source *ws)
 }
 EXPORT_SYMBOL_GPL(wakeup_source_drop);
 
+/*
+ * Record wakeup_source statistics being deleted into a dummy wakeup_source.
+ */
+static void wakeup_source_record(struct wakeup_source *ws)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&deleted_ws.lock, flags);
+
+	if (ws->event_count) {
+		deleted_ws.total_time =
+			ktime_add(deleted_ws.total_time, ws->total_time);
+		deleted_ws.max_time =
+			ktime_add(deleted_ws.max_time, ws->max_time);
+		deleted_ws.prevent_sleep_time =
+			ktime_add(deleted_ws.prevent_sleep_time,
+				  ws->prevent_sleep_time);
+		deleted_ws.event_count += ws->event_count;
+		deleted_ws.active_count += ws->active_count;
+		deleted_ws.relax_count += ws->relax_count;
+		deleted_ws.expire_count += ws->expire_count;
+		deleted_ws.wakeup_count += ws->wakeup_count;
+	}
+
+	spin_unlock_irqrestore(&deleted_ws.lock, flags);
+}
+
 /**
  * wakeup_source_destroy - Destroy a struct wakeup_source object.
  * @ws: Wakeup source to destroy.
@@ -122,6 +154,7 @@ void wakeup_source_destroy(struct wakeup_source *ws)
 		return;
 
 	wakeup_source_drop(ws);
+	wakeup_source_record(ws);
 	kfree(ws->name);
 	kfree(ws);
 }
@@ -930,6 +963,8 @@ static int wakeup_sources_stats_show(struct seq_file *m, void *unused)
 		print_wakeup_source_stats(m, ws);
 	rcu_read_unlock();
 
+	print_wakeup_source_stats(m, &deleted_ws);
+
 	return 0;
 }
 
-- 
2.2.0.rc0.207.ga3a616c

Re: [PATCH 3/3] power: add a dummy wakeup_source to record statistics

[Rafael J. Wysocki]

On Wednesday, April 22, 2015 05:50:12 PM Jin Qian wrote:
> After a wakeup_source is destroyed, we lost all information such as how
> long this wakeup_source has been active. Add a dummy wakeup_source to
> record such info.
> 
> Signed-off-by: Jin Qian <jinqian@android.com>

That's fine by me.  Queued up for 4.2, thanks!

> ---
>  drivers/base/power/wakeup.c | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> index bdb45f3..732683c 100644
> --- a/drivers/base/power/wakeup.c
> +++ b/drivers/base/power/wakeup.c
> @@ -59,6 +59,11 @@ static DECLARE_WAIT_QUEUE_HEAD(wakeup_count_wait_queue);
>  
>  static ktime_t last_read_time;
>  
> +static struct wakeup_source deleted_ws = {
> +	.name = "deleted",
> +	.lock =  __SPIN_LOCK_UNLOCKED(deleted_ws.lock),
> +};
> +
>  /**
>   * wakeup_source_prepare - Prepare a new wakeup source for initialization.
>   * @ws: Wakeup source to prepare.
> @@ -110,6 +115,33 @@ void wakeup_source_drop(struct wakeup_source *ws)
>  }
>  EXPORT_SYMBOL_GPL(wakeup_source_drop);
>  
> +/*
> + * Record wakeup_source statistics being deleted into a dummy wakeup_source.
> + */
> +static void wakeup_source_record(struct wakeup_source *ws)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&deleted_ws.lock, flags);
> +
> +	if (ws->event_count) {
> +		deleted_ws.total_time =
> +			ktime_add(deleted_ws.total_time, ws->total_time);
> +		deleted_ws.max_time =
> +			ktime_add(deleted_ws.max_time, ws->max_time);
> +		deleted_ws.prevent_sleep_time =
> +			ktime_add(deleted_ws.prevent_sleep_time,
> +				  ws->prevent_sleep_time);
> +		deleted_ws.event_count += ws->event_count;
> +		deleted_ws.active_count += ws->active_count;
> +		deleted_ws.relax_count += ws->relax_count;
> +		deleted_ws.expire_count += ws->expire_count;
> +		deleted_ws.wakeup_count += ws->wakeup_count;
> +	}
> +
> +	spin_unlock_irqrestore(&deleted_ws.lock, flags);
> +}
> +
>  /**
>   * wakeup_source_destroy - Destroy a struct wakeup_source object.
>   * @ws: Wakeup source to destroy.
> @@ -122,6 +154,7 @@ void wakeup_source_destroy(struct wakeup_source *ws)
>  		return;
>  
>  	wakeup_source_drop(ws);
> +	wakeup_source_record(ws);
>  	kfree(ws->name);
>  	kfree(ws);
>  }
> @@ -930,6 +963,8 @@ static int wakeup_sources_stats_show(struct seq_file *m, void *unused)
>  		print_wakeup_source_stats(m, ws);
>  	rcu_read_unlock();
>  
> +	print_wakeup_source_stats(m, &deleted_ws);
> +
>  	return 0;
>  }
>  
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 3/3] power: add a dummy wakeup_source to record statistics

[Jin Qian]

Hi Rafael,

I made a minor change in [PATCHv3 3/3] power: add a dummy
wakeup_source to record statistics. Sorry lkml seems down. I can send
you the link later.

The only diff is that deleted_ws.max_time = max(deleted_ws.max_time,
ws->max_time) instead of adding them up.

Thanks,
jin


On Fri, May 15, 2015 at 5:41 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Wednesday, April 22, 2015 05:50:12 PM Jin Qian wrote:
>> After a wakeup_source is destroyed, we lost all information such as how
>> long this wakeup_source has been active. Add a dummy wakeup_source to
>> record such info.
>>
>> Signed-off-by: Jin Qian <jinqian@android.com>
>
> That's fine by me.  Queued up for 4.2, thanks!
>
>> ---
>>  drivers/base/power/wakeup.c | 35 +++++++++++++++++++++++++++++++++++
>>  1 file changed, 35 insertions(+)
>>
>> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
>> index bdb45f3..732683c 100644
>> --- a/drivers/base/power/wakeup.c
>> +++ b/drivers/base/power/wakeup.c
>> @@ -59,6 +59,11 @@ static DECLARE_WAIT_QUEUE_HEAD(wakeup_count_wait_queue);
>>
>>  static ktime_t last_read_time;
>>
>> +static struct wakeup_source deleted_ws = {
>> +     .name = "deleted",
>> +     .lock =  __SPIN_LOCK_UNLOCKED(deleted_ws.lock),
>> +};
>> +
>>  /**
>>   * wakeup_source_prepare - Prepare a new wakeup source for initialization.
>>   * @ws: Wakeup source to prepare.
>> @@ -110,6 +115,33 @@ void wakeup_source_drop(struct wakeup_source *ws)
>>  }
>>  EXPORT_SYMBOL_GPL(wakeup_source_drop);
>>
>> +/*
>> + * Record wakeup_source statistics being deleted into a dummy wakeup_source.
>> + */
>> +static void wakeup_source_record(struct wakeup_source *ws)
>> +{
>> +     unsigned long flags;
>> +
>> +     spin_lock_irqsave(&deleted_ws.lock, flags);
>> +
>> +     if (ws->event_count) {
>> +             deleted_ws.total_time =
>> +                     ktime_add(deleted_ws.total_time, ws->total_time);
>> +             deleted_ws.max_time =
>> +                     ktime_add(deleted_ws.max_time, ws->max_time);
>> +             deleted_ws.prevent_sleep_time =
>> +                     ktime_add(deleted_ws.prevent_sleep_time,
>> +                               ws->prevent_sleep_time);
>> +             deleted_ws.event_count += ws->event_count;
>> +             deleted_ws.active_count += ws->active_count;
>> +             deleted_ws.relax_count += ws->relax_count;
>> +             deleted_ws.expire_count += ws->expire_count;
>> +             deleted_ws.wakeup_count += ws->wakeup_count;
>> +     }
>> +
>> +     spin_unlock_irqrestore(&deleted_ws.lock, flags);
>> +}
>> +
>>  /**
>>   * wakeup_source_destroy - Destroy a struct wakeup_source object.
>>   * @ws: Wakeup source to destroy.
>> @@ -122,6 +154,7 @@ void wakeup_source_destroy(struct wakeup_source *ws)
>>               return;
>>
>>       wakeup_source_drop(ws);
>> +     wakeup_source_record(ws);
>>       kfree(ws->name);
>>       kfree(ws);
>>  }
>> @@ -930,6 +963,8 @@ static int wakeup_sources_stats_show(struct seq_file *m, void *unused)
>>               print_wakeup_source_stats(m, ws);
>>       rcu_read_unlock();
>>
>> +     print_wakeup_source_stats(m, &deleted_ws);
>> +
>>       return 0;
>>  }
>>
>>
>
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 3/3] power: add a dummy wakeup_source to record statistics

[Rafael J. Wysocki]

On Friday, May 15, 2015 06:25:41 PM Jin Qian wrote:
> Hi Rafael,
> 
> I made a minor change in [PATCHv3 3/3] power: add a dummy
> wakeup_source to record statistics. Sorry lkml seems down. I can send
> you the link later.
> 
> The only diff is that deleted_ws.max_time = max(deleted_ws.max_time,
> ws->max_time) instead of adding them up.

OK, I'll use the new version, thanks!


> On Fri, May 15, 2015 at 5:41 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Wednesday, April 22, 2015 05:50:12 PM Jin Qian wrote:
> >> After a wakeup_source is destroyed, we lost all information such as how
> >> long this wakeup_source has been active. Add a dummy wakeup_source to
> >> record such info.
> >>
> >> Signed-off-by: Jin Qian <jinqian@android.com>
> >
> > That's fine by me.  Queued up for 4.2, thanks!
> >
> >> ---
> >>  drivers/base/power/wakeup.c | 35 +++++++++++++++++++++++++++++++++++
> >>  1 file changed, 35 insertions(+)
> >>
> >> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> >> index bdb45f3..732683c 100644
> >> --- a/drivers/base/power/wakeup.c
> >> +++ b/drivers/base/power/wakeup.c
> >> @@ -59,6 +59,11 @@ static DECLARE_WAIT_QUEUE_HEAD(wakeup_count_wait_queue);
> >>
> >>  static ktime_t last_read_time;
> >>
> >> +static struct wakeup_source deleted_ws = {
> >> +     .name = "deleted",
> >> +     .lock =  __SPIN_LOCK_UNLOCKED(deleted_ws.lock),
> >> +};
> >> +
> >>  /**
> >>   * wakeup_source_prepare - Prepare a new wakeup source for initialization.
> >>   * @ws: Wakeup source to prepare.
> >> @@ -110,6 +115,33 @@ void wakeup_source_drop(struct wakeup_source *ws)
> >>  }
> >>  EXPORT_SYMBOL_GPL(wakeup_source_drop);
> >>
> >> +/*
> >> + * Record wakeup_source statistics being deleted into a dummy wakeup_source.
> >> + */
> >> +static void wakeup_source_record(struct wakeup_source *ws)
> >> +{
> >> +     unsigned long flags;
> >> +
> >> +     spin_lock_irqsave(&deleted_ws.lock, flags);
> >> +
> >> +     if (ws->event_count) {
> >> +             deleted_ws.total_time =
> >> +                     ktime_add(deleted_ws.total_time, ws->total_time);
> >> +             deleted_ws.max_time =
> >> +                     ktime_add(deleted_ws.max_time, ws->max_time);
> >> +             deleted_ws.prevent_sleep_time =
> >> +                     ktime_add(deleted_ws.prevent_sleep_time,
> >> +                               ws->prevent_sleep_time);
> >> +             deleted_ws.event_count += ws->event_count;
> >> +             deleted_ws.active_count += ws->active_count;
> >> +             deleted_ws.relax_count += ws->relax_count;
> >> +             deleted_ws.expire_count += ws->expire_count;
> >> +             deleted_ws.wakeup_count += ws->wakeup_count;
> >> +     }
> >> +
> >> +     spin_unlock_irqrestore(&deleted_ws.lock, flags);
> >> +}
> >> +
> >>  /**
> >>   * wakeup_source_destroy - Destroy a struct wakeup_source object.
> >>   * @ws: Wakeup source to destroy.
> >> @@ -122,6 +154,7 @@ void wakeup_source_destroy(struct wakeup_source *ws)
> >>               return;
> >>
> >>       wakeup_source_drop(ws);
> >> +     wakeup_source_record(ws);
> >>       kfree(ws->name);
> >>       kfree(ws);
> >>  }
> >> @@ -930,6 +963,8 @@ static int wakeup_sources_stats_show(struct seq_file *m, void *unused)
> >>               print_wakeup_source_stats(m, ws);
> >>       rcu_read_unlock();
> >>
> >> +     print_wakeup_source_stats(m, &deleted_ws);
> >> +
> >>       return 0;
> >>  }
> >>
> >>
> >
> > --
> > 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 majordomo@vger.kernel.org
> 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.