[PATCH 1/1] kernel/power/autosleep.c: check for pm_suspend() return before queueing suspend again

[PATCH 1/1] kernel/power/autosleep.c: check for pm_suspend() return before queueing suspend again

[Nitish Ambastha]

Prevent tight loop for suspend-resume when some
devices failed to suspend
If some devices failed to suspend, we monitor this
error in try_to_suspend(). pm_suspend() is already
an 'int' returning function, how about checking return
from pm_suspend() before queueing suspend again?

For devices which do not register for pending events,
this will prevent tight loop for suspend-resume in
suspend abort scenarios due to device suspend failures

Signed-off-by: Nitish Ambastha <nitish.a@samsung.com>
---
 kernel/power/autosleep.c |   13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/kernel/power/autosleep.c b/kernel/power/autosleep.c
index 9012ecf..3923148 100644
--- a/kernel/power/autosleep.c
+++ b/kernel/power/autosleep.c
@@ -26,6 +26,7 @@ static struct wakeup_source *autosleep_ws;
 static void try_to_suspend(struct work_struct *work)
 {
 	unsigned int initial_count, final_count;
+	int error = 0;
 
 	if (!pm_get_wakeup_count(&initial_count, true))
 		goto out;
@@ -45,10 +46,20 @@ static void try_to_suspend(struct work_struct *work)
 	if (autosleep_state >= PM_SUSPEND_MAX)
 		hibernate();
 	else
-		pm_suspend(autosleep_state);
+		error = pm_suspend(autosleep_state);
 
 	mutex_unlock(&autosleep_lock);
 
+	if (error) {
+		/*
+		 * If some devices failed to suspend, its better to wait
+		 * to prevent system from trying to suspend in tight loop
+		 */
+		pr_info("PM: suspend returned (%d)\n", error);
+		schedule_timeout_uninterruptible(HZ / 2);
+		goto out;
+	}
+
 	if (!pm_get_wakeup_count(&final_count, false))
 		goto out;
 
-- 
1.7.9.5

Re: [PATCH 1/1] kernel/power/autosleep.c: check for pm_suspend() return before queueing suspend again

[Pavel Machek]

On Mon 2015-06-29 18:04:57, Nitish Ambastha wrote:
> Prevent tight loop for suspend-resume when some
> devices failed to suspend
> If some devices failed to suspend, we monitor this
> error in try_to_suspend(). pm_suspend() is already
> an 'int' returning function, how about checking return
> from pm_suspend() before queueing suspend again?
> 
> For devices which do not register for pending events,
> this will prevent tight loop for suspend-resume in
> suspend abort scenarios due to device suspend failures

Umm. I guess code could be rearranged so that wait entry is shared
with the already existing case?

Plus, I don't think "INFO" is right log level for failure...

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

[PATCHv2 1/1] kernel/power/autosleep.c: check for pm_suspend() return before queueing suspend again

[Nitish Ambastha]

Prevent tight loop for suspend-resume when some
devices failed to suspend
If some devices failed to suspend, we monitor this
error in try_to_suspend(). pm_suspend() is already
an 'int' returning function, how about checking return
from pm_suspend() before queueing suspend again?

For devices which do not register for pending events,
this will prevent tight loop for suspend-resume in
suspend abort scenarios due to device suspend failures

Signed-off-by: Nitish Ambastha <nitish.a@samsung.com>
---
v2: Rearranged code to make wait entry shared with
    existing one as suggested by Pavel Machek <pavel@ucw.cz>
    Corrected log level from pr_info to pr_err for failure log
    Added return check for hibernate()

 kernel/power/autosleep.c |   23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/kernel/power/autosleep.c b/kernel/power/autosleep.c
index 9012ecf..1a86698 100644
--- a/kernel/power/autosleep.c
+++ b/kernel/power/autosleep.c
@@ -26,6 +26,7 @@ static struct wakeup_source *autosleep_ws;
 static void try_to_suspend(struct work_struct *work)
 {
 	unsigned int initial_count, final_count;
+	int error = 0;
 
 	if (!pm_get_wakeup_count(&initial_count, true))
 		goto out;
@@ -43,22 +44,30 @@ static void try_to_suspend(struct work_struct *work)
 		return;
 	}
 	if (autosleep_state >= PM_SUSPEND_MAX)
-		hibernate();
+		error = hibernate();
 	else
-		pm_suspend(autosleep_state);
+		error = pm_suspend(autosleep_state);
 
 	mutex_unlock(&autosleep_lock);
 
+	if (error) {
+		pr_err("PM: suspend returned (%d)\n", error);
+		goto wait;
+	}
+
 	if (!pm_get_wakeup_count(&final_count, false))
 		goto out;
 
+	if (final_count != initial_count)
+		goto out;
+
+ wait:
 	/*
-	 * If the wakeup occured for an unknown reason, wait to prevent the
-	 * system from trying to suspend and waking up in a tight loop.
+	 * If some devices failed to suspend or if the wakeup ocurred
+	 * for an unknown reason, wait to prevent the system from
+	 * trying to suspend and waking up in a tight loop.
 	 */
-	if (final_count == initial_count)
-		schedule_timeout_uninterruptible(HZ / 2);
-
+	schedule_timeout_uninterruptible(HZ / 2);
  out:
 	queue_up_suspend_work();
 }
-- 
1.7.9.5

Re: [PATCHv2 1/1] kernel/power/autosleep.c: check for pm_suspend() return before queueing suspend again

[Rafael J. Wysocki]

On Tuesday, June 30, 2015 12:24:14 AM Nitish Ambastha wrote:
> Prevent tight loop for suspend-resume when some
> devices failed to suspend
> If some devices failed to suspend, we monitor this
> error in try_to_suspend(). pm_suspend() is already
> an 'int' returning function, how about checking return
> from pm_suspend() before queueing suspend again?
> 
> For devices which do not register for pending events,
> this will prevent tight loop for suspend-resume in
> suspend abort scenarios due to device suspend failures
> 
> Signed-off-by: Nitish Ambastha <nitish.a@samsung.com>
> ---
> v2: Rearranged code to make wait entry shared with
>     existing one as suggested by Pavel Machek <pavel@ucw.cz>
>     Corrected log level from pr_info to pr_err for failure log
>     Added return check for hibernate()
> 
>  kernel/power/autosleep.c |   23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/power/autosleep.c b/kernel/power/autosleep.c
> index 9012ecf..1a86698 100644
> --- a/kernel/power/autosleep.c
> +++ b/kernel/power/autosleep.c
> @@ -26,6 +26,7 @@ static struct wakeup_source *autosleep_ws;
>  static void try_to_suspend(struct work_struct *work)
>  {
>  	unsigned int initial_count, final_count;
> +	int error = 0;

The initial value is not needed.

>  
>  	if (!pm_get_wakeup_count(&initial_count, true))
>  		goto out;
> @@ -43,22 +44,30 @@ static void try_to_suspend(struct work_struct *work)
>  		return;
>  	}
>  	if (autosleep_state >= PM_SUSPEND_MAX)
> -		hibernate();
> +		error = hibernate();
>  	else
> -		pm_suspend(autosleep_state);
> +		error = pm_suspend(autosleep_state);

I'd prefer to write that as

	error = autosleep_state < PM_SUSPEND_MAX ?
		pm_suspend(autosleep_state) : hibernate();

>  
>  	mutex_unlock(&autosleep_lock);
>  
> +	if (error) {
> +		pr_err("PM: suspend returned (%d)\n", error);

There is a debug message printed for that in the device suspend code, do we
need one more here?

> +		goto wait;
> +	}
> +
>  	if (!pm_get_wakeup_count(&final_count, false))
>  		goto out;
>  
> +	if (final_count != initial_count)
> +		goto out;
> +
> + wait:
>  	/*
> -	 * If the wakeup occured for an unknown reason, wait to prevent the
> -	 * system from trying to suspend and waking up in a tight loop.
> +	 * If some devices failed to suspend or if the wakeup ocurred
> +	 * for an unknown reason, wait to prevent the system from
> +	 * trying to suspend and waking up in a tight loop.
>  	 */
> -	if (final_count == initial_count)
> -		schedule_timeout_uninterruptible(HZ / 2);
> -
> +	schedule_timeout_uninterruptible(HZ / 2);
>   out:
>  	queue_up_suspend_work();

I'd arrange it this way:

	if (error || pm_get_wakeup_count(&final_count, false)
	    || final_count == initial_count)
		schedule_timeout_uninterruptible(HZ / 2);

 out:
  	queue_up_suspend_work();
>  }
> 

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

Re: [PATCHv2 1/1] kernel/power/autosleep.c: check for pm_suspend() return before queueing suspend again

[Rafael J. Wysocki]

On Monday, June 29, 2015 09:56:18 PM Rafael J. Wysocki wrote:
> On Tuesday, June 30, 2015 12:24:14 AM Nitish Ambastha wrote:
> > Prevent tight loop for suspend-resume when some
> > devices failed to suspend
> > If some devices failed to suspend, we monitor this
> > error in try_to_suspend(). pm_suspend() is already
> > an 'int' returning function, how about checking return
> > from pm_suspend() before queueing suspend again?
> > 
> > For devices which do not register for pending events,
> > this will prevent tight loop for suspend-resume in
> > suspend abort scenarios due to device suspend failures

Having said the below I'm not sure why the current code doesn't cover this
for you?

That would be the final_count == initial_count case, no?


> > 
> > Signed-off-by: Nitish Ambastha <nitish.a@samsung.com>
> > ---
> > v2: Rearranged code to make wait entry shared with
> >     existing one as suggested by Pavel Machek <pavel@ucw.cz>
> >     Corrected log level from pr_info to pr_err for failure log
> >     Added return check for hibernate()
> > 
> >  kernel/power/autosleep.c |   23 ++++++++++++++++-------
> >  1 file changed, 16 insertions(+), 7 deletions(-)
> > 
> > diff --git a/kernel/power/autosleep.c b/kernel/power/autosleep.c
> > index 9012ecf..1a86698 100644
> > --- a/kernel/power/autosleep.c
> > +++ b/kernel/power/autosleep.c
> > @@ -26,6 +26,7 @@ static struct wakeup_source *autosleep_ws;
> >  static void try_to_suspend(struct work_struct *work)
> >  {
> >  	unsigned int initial_count, final_count;
> > +	int error = 0;
> 
> The initial value is not needed.
> 
> >  
> >  	if (!pm_get_wakeup_count(&initial_count, true))
> >  		goto out;
> > @@ -43,22 +44,30 @@ static void try_to_suspend(struct work_struct *work)
> >  		return;
> >  	}
> >  	if (autosleep_state >= PM_SUSPEND_MAX)
> > -		hibernate();
> > +		error = hibernate();
> >  	else
> > -		pm_suspend(autosleep_state);
> > +		error = pm_suspend(autosleep_state);
> 
> I'd prefer to write that as
> 
> 	error = autosleep_state < PM_SUSPEND_MAX ?
> 		pm_suspend(autosleep_state) : hibernate();
> 
> >  
> >  	mutex_unlock(&autosleep_lock);
> >  
> > +	if (error) {
> > +		pr_err("PM: suspend returned (%d)\n", error);
> 
> There is a debug message printed for that in the device suspend code, do we
> need one more here?
> 
> > +		goto wait;
> > +	}
> > +
> >  	if (!pm_get_wakeup_count(&final_count, false))
> >  		goto out;
> >  
> > +	if (final_count != initial_count)
> > +		goto out;
> > +
> > + wait:
> >  	/*
> > -	 * If the wakeup occured for an unknown reason, wait to prevent the
> > -	 * system from trying to suspend and waking up in a tight loop.
> > +	 * If some devices failed to suspend or if the wakeup ocurred
> > +	 * for an unknown reason, wait to prevent the system from
> > +	 * trying to suspend and waking up in a tight loop.
> >  	 */
> > -	if (final_count == initial_count)
> > -		schedule_timeout_uninterruptible(HZ / 2);
> > -
> > +	schedule_timeout_uninterruptible(HZ / 2);
> >   out:
> >  	queue_up_suspend_work();
> 
> I'd arrange it this way:
> 
> 	if (error || pm_get_wakeup_count(&final_count, false)
> 	    || final_count == initial_count)
> 		schedule_timeout_uninterruptible(HZ / 2);
> 
>  out:
>   	queue_up_suspend_work();
> >  }
> > 
> 
> 

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

Re: [PATCHv2 1/1] kernel/power/autosleep.c: check for pm_suspend() return before queueing suspend again

[Nitish Ambastha]

Hi Rafael

Thanks for your feedback

On Tue, Jun 30, 2015 at 1:37 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Monday, June 29, 2015 09:56:18 PM Rafael J. Wysocki wrote:
>> On Tuesday, June 30, 2015 12:24:14 AM Nitish Ambastha wrote:
>> > Prevent tight loop for suspend-resume when some
>> > devices failed to suspend
>> > If some devices failed to suspend, we monitor this
>> > error in try_to_suspend(). pm_suspend() is already
>> > an 'int' returning function, how about checking return
>> > from pm_suspend() before queueing suspend again?
>> >
>> > For devices which do not register for pending events,
>> > this will prevent tight loop for suspend-resume in
>> > suspend abort scenarios due to device suspend failures
>
> Having said the below I'm not sure why the current code doesn't cover this
> for you?
>
> That would be the final_count == initial_count case, no?
>
Agree, this should cover most of the cases, however there are some
cases where final_count may not match initial_count here

A couple of such scenario I came across is
1) when tasks are restarted again due to suspend failure, sometimes
battery kernel thread acquires lock for battery monitoring resulting
in either pm_get_wakeup_count() returning false or increment in
final_count from initial_count
2) In some platforms, power transitions are carried from User space
(power manager), these power-manager tries to hold some wake lock
after being restarted on resume

It seems to me that we can identify the error in suspend through
return values earlier and may not need to go ahead and check
final_count to catch the same later

>
>> >
>> > Signed-off-by: Nitish Ambastha <nitish.a@samsung.com>
>> > ---
>> > v2: Rearranged code to make wait entry shared with
>> >     existing one as suggested by Pavel Machek <pavel@ucw.cz>
>> >     Corrected log level from pr_info to pr_err for failure log
>> >     Added return check for hibernate()
>> >
>> >  kernel/power/autosleep.c |   23 ++++++++++++++++-------
>> >  1 file changed, 16 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/kernel/power/autosleep.c b/kernel/power/autosleep.c
>> > index 9012ecf..1a86698 100644
>> > --- a/kernel/power/autosleep.c
>> > +++ b/kernel/power/autosleep.c
>> > @@ -26,6 +26,7 @@ static struct wakeup_source *autosleep_ws;
>> >  static void try_to_suspend(struct work_struct *work)
>> >  {
>> >     unsigned int initial_count, final_count;
>> > +   int error = 0;
>>
>> The initial value is not needed.
>>
>> >
>> >     if (!pm_get_wakeup_count(&initial_count, true))
>> >             goto out;
>> > @@ -43,22 +44,30 @@ static void try_to_suspend(struct work_struct *work)
>> >             return;
>> >     }
>> >     if (autosleep_state >= PM_SUSPEND_MAX)
>> > -           hibernate();
>> > +           error = hibernate();
>> >     else
>> > -           pm_suspend(autosleep_state);
>> > +           error = pm_suspend(autosleep_state);
>>
>> I'd prefer to write that as
>>
>>       error = autosleep_state < PM_SUSPEND_MAX ?
>>               pm_suspend(autosleep_state) : hibernate();
>>
>> >
>> >     mutex_unlock(&autosleep_lock);
>> >
>> > +   if (error) {
>> > +           pr_err("PM: suspend returned (%d)\n", error);
>>
>> There is a debug message printed for that in the device suspend code, do we
>> need one more here?
>>
>> > +           goto wait;
>> > +   }
>> > +
>> >     if (!pm_get_wakeup_count(&final_count, false))
>> >             goto out;
>> >
>> > +   if (final_count != initial_count)
>> > +           goto out;
>> > +
>> > + wait:
>> >     /*
>> > -    * If the wakeup occured for an unknown reason, wait to prevent the
>> > -    * system from trying to suspend and waking up in a tight loop.
>> > +    * If some devices failed to suspend or if the wakeup ocurred
>> > +    * for an unknown reason, wait to prevent the system from
>> > +    * trying to suspend and waking up in a tight loop.
>> >      */
>> > -   if (final_count == initial_count)
>> > -           schedule_timeout_uninterruptible(HZ / 2);
>> > -
>> > +   schedule_timeout_uninterruptible(HZ / 2);
>> >   out:
>> >     queue_up_suspend_work();
>>
>> I'd arrange it this way:
>>
>>       if (error || pm_get_wakeup_count(&final_count, false)
>>           || final_count == initial_count)
>>               schedule_timeout_uninterruptible(HZ / 2);
>>
>>  out:
>>       queue_up_suspend_work();
>> >  }
>> >
>>
>>
>
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCHv2 1/1] kernel/power/autosleep.c: check for pm_suspend() return before queueing suspend again

[Rafael J. Wysocki]

On Wednesday, July 01, 2015 12:52:43 AM Nitish Ambastha wrote:
> Hi Rafael
> 
> Thanks for your feedback
> 
> On Tue, Jun 30, 2015 at 1:37 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Monday, June 29, 2015 09:56:18 PM Rafael J. Wysocki wrote:
> >> On Tuesday, June 30, 2015 12:24:14 AM Nitish Ambastha wrote:
> >> > Prevent tight loop for suspend-resume when some
> >> > devices failed to suspend
> >> > If some devices failed to suspend, we monitor this
> >> > error in try_to_suspend(). pm_suspend() is already
> >> > an 'int' returning function, how about checking return
> >> > from pm_suspend() before queueing suspend again?
> >> >
> >> > For devices which do not register for pending events,
> >> > this will prevent tight loop for suspend-resume in
> >> > suspend abort scenarios due to device suspend failures
> >
> > Having said the below I'm not sure why the current code doesn't cover this
> > for you?
> >
> > That would be the final_count == initial_count case, no?
> >
> Agree, this should cover most of the cases, however there are some
> cases where final_count may not match initial_count here
> 
> A couple of such scenario I came across is
> 1) when tasks are restarted again due to suspend failure, sometimes
> battery kernel thread acquires lock for battery monitoring resulting
> in either pm_get_wakeup_count() returning false or increment in
> final_count from initial_count

Locks should not have any effect on the return value of pm_get_wakeup_count()
and if false is returned by it, a wakeup event was being processed when it
was called.

In turn, if pm_get_wakeup_count() returns false or final_count != initial_count,
this means that *somebody* called pm_wakeup_event() or equivalent in the meantime
and there *was* a valid wakeup event (regardless of or in addition to the driver
error).

> 2) In some platforms, power transitions are carried from User space
> (power manager), these power-manager tries to hold some wake lock
> after being restarted on resume

And what exactly is the failing scenario in that case?


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

Re: [PATCHv2 1/1] kernel/power/autosleep.c: check for pm_suspend() return before queueing suspend again

[Nitish Ambastha]

On Wed, Jul 1, 2015 at 1:31 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> On Wednesday, July 01, 2015 12:52:43 AM Nitish Ambastha wrote:
> > Hi Rafael
> >
> > Thanks for your feedback
> >
> > On Tue, Jun 30, 2015 at 1:37 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > On Monday, June 29, 2015 09:56:18 PM Rafael J. Wysocki wrote:
> > >> On Tuesday, June 30, 2015 12:24:14 AM Nitish Ambastha wrote:
> > >> > Prevent tight loop for suspend-resume when some
> > >> > devices failed to suspend
> > >> > If some devices failed to suspend, we monitor this
> > >> > error in try_to_suspend(). pm_suspend() is already
> > >> > an 'int' returning function, how about checking return
> > >> > from pm_suspend() before queueing suspend again?
> > >> >
> > >> > For devices which do not register for pending events,
> > >> > this will prevent tight loop for suspend-resume in
> > >> > suspend abort scenarios due to device suspend failures
> > >
> > > Having said the below I'm not sure why the current code doesn't cover this
> > > for you?
> > >
> > > That would be the final_count == initial_count case, no?
> > >
> > Agree, this should cover most of the cases, however there are some
> > cases where final_count may not match initial_count here
> >
> > A couple of such scenario I came across is
> > 1) when tasks are restarted again due to suspend failure, sometimes
> > battery kernel thread acquires lock for battery monitoring resulting
> > in either pm_get_wakeup_count() returning false or increment in
> > final_count from initial_count
>
> Locks should not have any effect on the return value of pm_get_wakeup_count()
> and if false is returned by it, a wakeup event was being processed when it
> was called.
>
'lock' was not a correct term used, sorry about it.
By 'lock', I actually meant battery monitoring acquiring 'wake lock' here
i.e pm_wakeup_event/pm_stay_awake().

when battery kernel thread calls pm_stay_awake() or pm_wakeup_event()
momentarily and then pm_relax(), after being restarted on suspend failure,
this affects the wakeup event count

> In turn, if pm_get_wakeup_count() returns false or final_count != initial_count,
> this means that *somebody* called pm_wakeup_event() or equivalent in the meantime
> and there *was* a valid wakeup event (regardless of or in addition to the driver
> error).
>
ok, as I understand, if some driver failed to suspend, and during resume
if *somebody* called pm_stay_awake() or pm_wakeup_event() meantime,
and then pm_relax(), final_count and initial_count will not be
same in try_to_suspend(), and it will be considered as a *valid wakeup*
event, though the actual reason of resume was suspend failure.
In this condition, it will again try to queue suspend

Will it be a reasonable idea to wait in this case, before queueing
suspend as some drivers failed to suspend in the current attempt?

> > 2) In some platforms, power transitions are carried from User space
> > (power manager), these power-manager tries to hold some wake lock
> > after being restarted on resume
>
> And what exactly is the failing scenario in that case?
>
>
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.

[PATCHv3 1/1] kernel/power/autosleep.c: check for pm_suspend() return before queueing suspend again

[Nitish Ambastha]

Prevent tight loop for suspend-resume when some
devices failed to suspend
If some devices failed to suspend, we monitor this
error in try_to_suspend(). pm_suspend() is already
an 'int' returning function, how about checking return
from pm_suspend() before queueing suspend again?

For devices which do not register for pending events,
this will prevent tight loop for suspend-resume in
suspend abort scenarios due to device suspend failures

Signed-off-by: Nitish Ambastha <nitish.a@samsung.com>
---
v2: Rearranged code to make wait entry shared with
    existing one as suggested by Pavel Machek <pavel@ucw.cz>
    Corrected log level from pr_info to pr_err for failure log
    Added return check for hibernate()

v3: Restructured code as suggested by Rafael J Wysocki <rjw@rjwysocki.net>

 kernel/power/autosleep.c |   20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/kernel/power/autosleep.c b/kernel/power/autosleep.c
index 9012ecf..e458d0c 100644
--- a/kernel/power/autosleep.c
+++ b/kernel/power/autosleep.c
@@ -26,6 +26,7 @@ static struct wakeup_source *autosleep_ws;
 static void try_to_suspend(struct work_struct *work)
 {
 	unsigned int initial_count, final_count;
+	int error;
 
 	if (!pm_get_wakeup_count(&initial_count, true))
 		goto out;
@@ -42,23 +43,20 @@ static void try_to_suspend(struct work_struct *work)
 		mutex_unlock(&autosleep_lock);
 		return;
 	}
-	if (autosleep_state >= PM_SUSPEND_MAX)
-		hibernate();
-	else
-		pm_suspend(autosleep_state);
 
-	mutex_unlock(&autosleep_lock);
+	error = autosleep_state < PM_SUSPEND_MAX ?
+		pm_suspend(autosleep_state) : hibernate();
 
-	if (!pm_get_wakeup_count(&final_count, false))
-		goto out;
+	mutex_unlock(&autosleep_lock);
 
 	/*
-	 * If the wakeup occured for an unknown reason, wait to prevent the
-	 * system from trying to suspend and waking up in a tight loop.
+	 * If some devices failed to suspend or if the wakeup ocurred
+	 * for an unknown reason, wait to prevent the system from
+	 * trying to suspend and waking up in a tight loop.
 	 */
-	if (final_count == initial_count)
+	if (error || (pm_get_wakeup_count(&final_count, false)
+		&& (final_count == initial_count)))
 		schedule_timeout_uninterruptible(HZ / 2);
-
  out:
 	queue_up_suspend_work();
 }
-- 
1.7.9.5

Re: [PATCHv3 1/1] kernel/power/autosleep.c: check for pm_suspend() return before queueing suspend again

[Nitish Ambastha]

I have uploaded a new patch as per comments

Since some drivers failed to suspend, and is/are not registered to
wakeup sources fwk, we cannot keep trying suspend based on wakeup
event count. It looks valid to me to wait before queuing another
suspend request tightly

Please let me know if this is reasonable?

On Tue, Jul 14, 2015 at 1:38 AM, Nitish Ambastha <nitish.a@samsung.com> wrote:
> Prevent tight loop for suspend-resume when some
> devices failed to suspend
> If some devices failed to suspend, we monitor this
> error in try_to_suspend(). pm_suspend() is already
> an 'int' returning function, how about checking return
> from pm_suspend() before queueing suspend again?
>
> For devices which do not register for pending events,
> this will prevent tight loop for suspend-resume in
> suspend abort scenarios due to device suspend failures
>
> Signed-off-by: Nitish Ambastha <nitish.a@samsung.com>
> ---
> v2: Rearranged code to make wait entry shared with
>     existing one as suggested by Pavel Machek <pavel@ucw.cz>
>     Corrected log level from pr_info to pr_err for failure log
>     Added return check for hibernate()
>
> v3: Restructured code as suggested by Rafael J Wysocki <rjw@rjwysocki.net>
>
>  kernel/power/autosleep.c |   20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/kernel/power/autosleep.c b/kernel/power/autosleep.c
> index 9012ecf..e458d0c 100644
> --- a/kernel/power/autosleep.c
> +++ b/kernel/power/autosleep.c
> @@ -26,6 +26,7 @@ static struct wakeup_source *autosleep_ws;
>  static void try_to_suspend(struct work_struct *work)
>  {
>         unsigned int initial_count, final_count;
> +       int error;
>
>         if (!pm_get_wakeup_count(&initial_count, true))
>                 goto out;
> @@ -42,23 +43,20 @@ static void try_to_suspend(struct work_struct *work)
>                 mutex_unlock(&autosleep_lock);
>                 return;
>         }
> -       if (autosleep_state >= PM_SUSPEND_MAX)
> -               hibernate();
> -       else
> -               pm_suspend(autosleep_state);
>
> -       mutex_unlock(&autosleep_lock);
> +       error = autosleep_state < PM_SUSPEND_MAX ?
> +               pm_suspend(autosleep_state) : hibernate();
>
> -       if (!pm_get_wakeup_count(&final_count, false))
> -               goto out;
> +       mutex_unlock(&autosleep_lock);
>
>         /*
> -        * If the wakeup occured for an unknown reason, wait to prevent the
> -        * system from trying to suspend and waking up in a tight loop.
> +        * If some devices failed to suspend or if the wakeup ocurred
> +        * for an unknown reason, wait to prevent the system from
> +        * trying to suspend and waking up in a tight loop.
>          */
> -       if (final_count == initial_count)
> +       if (error || (pm_get_wakeup_count(&final_count, false)
> +               && (final_count == initial_count)))
>                 schedule_timeout_uninterruptible(HZ / 2);
> -
>   out:
>         queue_up_suspend_work();
>  }
> --
> 1.7.9.5
>

Re: [PATCHv3 1/1] kernel/power/autosleep.c: check for pm_suspend() return before queueing suspend again

[Rafael J. Wysocki]

On Tuesday, July 14, 2015 01:38:02 AM Nitish Ambastha wrote:
> Prevent tight loop for suspend-resume when some
> devices failed to suspend

This *still* doesn't explain what problem you're *really* trying to address.

Even if a driver returns an error code from one of its suspend callbacks,
you should get final_count == initial_count in the final check and we'll
schedule the timeout.

So there is a failure scenarion you're trying to address where that check is
not sufficient, but you're not saying what the scenario is.

Quite frankly, it seems to me that you're trying to hide a spurious wakeup.


> If some devices failed to suspend, we monitor this
> error in try_to_suspend(). pm_suspend() is already
> an 'int' returning function, how about checking return
> from pm_suspend() before queueing suspend again?
> 
> For devices which do not register for pending events,
> this will prevent tight loop for suspend-resume in
> suspend abort scenarios due to device suspend failures
> 
> Signed-off-by: Nitish Ambastha <nitish.a@samsung.com>
> ---
> v2: Rearranged code to make wait entry shared with
>     existing one as suggested by Pavel Machek <pavel@ucw.cz>
>     Corrected log level from pr_info to pr_err for failure log
>     Added return check for hibernate()
> 
> v3: Restructured code as suggested by Rafael J Wysocki <rjw@rjwysocki.net>
> 
>  kernel/power/autosleep.c |   20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/kernel/power/autosleep.c b/kernel/power/autosleep.c
> index 9012ecf..e458d0c 100644
> --- a/kernel/power/autosleep.c
> +++ b/kernel/power/autosleep.c
> @@ -26,6 +26,7 @@ static struct wakeup_source *autosleep_ws;
>  static void try_to_suspend(struct work_struct *work)
>  {
>  	unsigned int initial_count, final_count;
> +	int error;
>  
>  	if (!pm_get_wakeup_count(&initial_count, true))
>  		goto out;
> @@ -42,23 +43,20 @@ static void try_to_suspend(struct work_struct *work)
>  		mutex_unlock(&autosleep_lock);
>  		return;
>  	}
> -	if (autosleep_state >= PM_SUSPEND_MAX)
> -		hibernate();
> -	else
> -		pm_suspend(autosleep_state);
>  
> -	mutex_unlock(&autosleep_lock);
> +	error = autosleep_state < PM_SUSPEND_MAX ?
> +		pm_suspend(autosleep_state) : hibernate();
>  
> -	if (!pm_get_wakeup_count(&final_count, false))
> -		goto out;
> +	mutex_unlock(&autosleep_lock);
>  
>  	/*
> -	 * If the wakeup occured for an unknown reason, wait to prevent the
> -	 * system from trying to suspend and waking up in a tight loop.
> +	 * If some devices failed to suspend or if the wakeup ocurred
> +	 * for an unknown reason, wait to prevent the system from
> +	 * trying to suspend and waking up in a tight loop.
>  	 */
> -	if (final_count == initial_count)
> +	if (error || (pm_get_wakeup_count(&final_count, false)
> +		&& (final_count == initial_count)))
>  		schedule_timeout_uninterruptible(HZ / 2);
> -
>   out:
>  	queue_up_suspend_work();
>  }
> 

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

Re: [PATCHv3 1/1] kernel/power/autosleep.c: check for pm_suspend() return before queueing suspend again

[Nitish Ambastha]

On Tue, Jul 14, 2015 at 5:13 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Tuesday, July 14, 2015 01:38:02 AM Nitish Ambastha wrote:
>> Prevent tight loop for suspend-resume when some
>> devices failed to suspend
>
> This *still* doesn't explain what problem you're *really* trying to address.
>
> Even if a driver returns an error code from one of its suspend callbacks,
> you should get final_count == initial_count in the final check and we'll
> schedule the timeout.
>
> So there is a failure scenarion you're trying to address where that check is
> not sufficient, but you're not saying what the scenario is.
>
As I mentioned earlier, if some driver failed to suspend, and during
resume if *somebody* called pm_stay_awake() or pm_wakeup_event()
meantime, and then pm_relax(), final_count and initial_count will not
be the same in try_to_suspend(). We observed this behavior with
battery monitor thread on being restarted

In these scenarios, it will be considered a *valid wakeup* event and
it will try to queue suspend immediately, though the actual reason of
resume was driver returning error code.
For such scenarios, event count may not always be safe way to identify.

> Quite frankly, it seems to me that you're trying to hide a spurious wakeup.
>
>
>> If some devices failed to suspend, we monitor this
>> error in try_to_suspend(). pm_suspend() is already
>> an 'int' returning function, how about checking return
>> from pm_suspend() before queueing suspend again?
>>
>> For devices which do not register for pending events,
>> this will prevent tight loop for suspend-resume in
>> suspend abort scenarios due to device suspend failures
>>
>> Signed-off-by: Nitish Ambastha <nitish.a@samsung.com>
>> ---
>> v2: Rearranged code to make wait entry shared with
>>     existing one as suggested by Pavel Machek <pavel@ucw.cz>
>>     Corrected log level from pr_info to pr_err for failure log
>>     Added return check for hibernate()
>>
>> v3: Restructured code as suggested by Rafael J Wysocki <rjw@rjwysocki.net>
>>
>>  kernel/power/autosleep.c |   20 +++++++++-----------
>>  1 file changed, 9 insertions(+), 11 deletions(-)
>>
>> diff --git a/kernel/power/autosleep.c b/kernel/power/autosleep.c
>> index 9012ecf..e458d0c 100644
>> --- a/kernel/power/autosleep.c
>> +++ b/kernel/power/autosleep.c
>> @@ -26,6 +26,7 @@ static struct wakeup_source *autosleep_ws;
>>  static void try_to_suspend(struct work_struct *work)
>>  {
>>       unsigned int initial_count, final_count;
>> +     int error;
>>
>>       if (!pm_get_wakeup_count(&initial_count, true))
>>               goto out;
>> @@ -42,23 +43,20 @@ static void try_to_suspend(struct work_struct *work)
>>               mutex_unlock(&autosleep_lock);
>>               return;
>>       }
>> -     if (autosleep_state >= PM_SUSPEND_MAX)
>> -             hibernate();
>> -     else
>> -             pm_suspend(autosleep_state);
>>
>> -     mutex_unlock(&autosleep_lock);
>> +     error = autosleep_state < PM_SUSPEND_MAX ?
>> +             pm_suspend(autosleep_state) : hibernate();
>>
>> -     if (!pm_get_wakeup_count(&final_count, false))
>> -             goto out;
>> +     mutex_unlock(&autosleep_lock);
>>
>>       /*
>> -      * If the wakeup occured for an unknown reason, wait to prevent the
>> -      * system from trying to suspend and waking up in a tight loop.
>> +      * If some devices failed to suspend or if the wakeup ocurred
>> +      * for an unknown reason, wait to prevent the system from
>> +      * trying to suspend and waking up in a tight loop.
>>        */
>> -     if (final_count == initial_count)
>> +     if (error || (pm_get_wakeup_count(&final_count, false)
>> +             && (final_count == initial_count)))
>>               schedule_timeout_uninterruptible(HZ / 2);
>> -
>>   out:
>>       queue_up_suspend_work();
>>  }
>>
>
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCHv3 1/1] kernel/power/autosleep.c: check for pm_suspend() return before queueing suspend again

[Rafael J. Wysocki]

On Tuesday, July 14, 2015 09:34:08 AM Nitish Ambastha wrote:
> On Tue, Jul 14, 2015 at 5:13 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Tuesday, July 14, 2015 01:38:02 AM Nitish Ambastha wrote:
> >> Prevent tight loop for suspend-resume when some
> >> devices failed to suspend
> >
> > This *still* doesn't explain what problem you're *really* trying to address.
> >
> > Even if a driver returns an error code from one of its suspend callbacks,
> > you should get final_count == initial_count in the final check and we'll
> > schedule the timeout.
> >
> > So there is a failure scenarion you're trying to address where that check is
> > not sufficient, but you're not saying what the scenario is.
> >
> As I mentioned earlier, if some driver failed to suspend, and during
> resume if *somebody* called pm_stay_awake() or pm_wakeup_event()
> meantime, and then pm_relax(), final_count and initial_count will not
> be the same in try_to_suspend(). We observed this behavior with
> battery monitor thread on being restarted

But that means there was a valid wakeup event, doesn't it?

> In these scenarios, it will be considered a *valid wakeup* event and
> it will try to queue suspend immediately, though the actual reason of
> resume was driver returning error code.

Even if a wakeup event occurs in addition to a driver failing the suspend, it
is still valid.

So it looks like you want to schedule the timeout unconditionally in case of
a failed suspend, but then you need to filter out -EBUSY (which is returned
on valid wakeup events).  Essentially, that would slow down autosleep, but
how does that help exactly?

Thanks,
Rafael

Re: [PATCHv3 1/1] kernel/power/autosleep.c: check for pm_suspend() return before queueing suspend again

[Nitish Ambastha]

Hi Rafael,

Thank you very much for your comments and feedback
I have planned to drop this one now. Before I close, I would just
share few more comments and idea behind this.

Before queueing next suspend, we track wakeup event counts
consistently for drivers registered to wakeup events, to know if
events are pending or completed. However we cannot identify or track
the same for other drivers which are not registered with wakeup
events.
Since we can't track the events and behavior of these drivers, we
cannot predict if the next attempt of suspend will be successful on
those drivers, while the current attempt failed. This resulting in a
possibility of tight suspend resume if there is suspend failures by
these drivers more frequently.
So, my opinion was to prevent immediate suspend requests when some
drivers (which are not registered to wakeup events) failed to suspend
in current attempt. I might be wrong, but this is how I understand it.


Regards
Nitish Ambastha


On Thu, Jul 16, 2015 at 3:59 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Tuesday, July 14, 2015 09:34:08 AM Nitish Ambastha wrote:
>> On Tue, Jul 14, 2015 at 5:13 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> > On Tuesday, July 14, 2015 01:38:02 AM Nitish Ambastha wrote:
>> >> Prevent tight loop for suspend-resume when some
>> >> devices failed to suspend
>> >
>> > This *still* doesn't explain what problem you're *really* trying to address.
>> >
>> > Even if a driver returns an error code from one of its suspend callbacks,
>> > you should get final_count == initial_count in the final check and we'll
>> > schedule the timeout.
>> >
>> > So there is a failure scenarion you're trying to address where that check is
>> > not sufficient, but you're not saying what the scenario is.
>> >
>> As I mentioned earlier, if some driver failed to suspend, and during
>> resume if *somebody* called pm_stay_awake() or pm_wakeup_event()
>> meantime, and then pm_relax(), final_count and initial_count will not
>> be the same in try_to_suspend(). We observed this behavior with
>> battery monitor thread on being restarted
>
> But that means there was a valid wakeup event, doesn't it?
>
>> In these scenarios, it will be considered a *valid wakeup* event and
>> it will try to queue suspend immediately, though the actual reason of
>> resume was driver returning error code.
>
> Even if a wakeup event occurs in addition to a driver failing the suspend, it
> is still valid.
>
> So it looks like you want to schedule the timeout unconditionally in case of
> a failed suspend, but then you need to filter out -EBUSY (which is returned
> on valid wakeup events).  Essentially, that would slow down autosleep, but
> how does that help exactly?
>
> Thanks,
> Rafael
>