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

["Rafael J. Wysocki" <rjw@rjwysocki.net>]

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.