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 <[email protected]>
> ---
> v2: Rearranged code to make wait entry shared with
> existing one as suggested by Pavel Machek <[email protected]>
> 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.
--
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/
