2013-04-20 15:46:58

by Jiri Slaby

[permalink] [raw]
Subject: Resume does not work after timekeeping change

Hi,

my machine does not wake from suspend to RAM on my box running the -next
kernel. The last thing I see is "Disabling non-boot CPUs ...". I
bisected it to this commit:

commit 7ec98e15aa049b7a2ca73485f31cf4f90c34e2dd
Author: Thomas Gleixner <[email protected]>
Date: Thu Feb 21 22:51:39 2013 +0000

timekeeping: Delay update of clock->cycle_last


Reverting that one on the top of -next-20130419 makes it work again.


I also tried it inside a VM using suspend to disk. There, it behaves
like it takes a minute or so to wake up. So I tried to wait on the real
HW too, but it never resumes there.

regards,
--
js
suse labs


2013-04-20 18:41:58

by Borislav Petkov

[permalink] [raw]
Subject: Re: Resume does not work after timekeeping change

On Sat, Apr 20, 2013 at 05:46:52PM +0200, Jiri Slaby wrote:
> Hi,
>
> my machine does not wake from suspend to RAM on my box running the -next
> kernel. The last thing I see is "Disabling non-boot CPUs ...". I
> bisected it to this commit:
>
> commit 7ec98e15aa049b7a2ca73485f31cf4f90c34e2dd
> Author: Thomas Gleixner <[email protected]>
> Date: Thu Feb 21 22:51:39 2013 +0000
>
> timekeeping: Delay update of clock->cycle_last
>
>
> Reverting that one on the top of -next-20130419 makes it work again.
>
>
> I also tried it inside a VM using suspend to disk. There, it behaves
> like it takes a minute or so to wake up. So I tried to wait on the real
> HW too, but it never resumes there.

Right, and I'm seeing it here too but it sometimes does resume after
about a minute or so and sometimes it doesn't.

Reverting the patch by hand ontop of tip/master fixes the issue here
too.

Btw, Ingo, remember how I was telling you that I'm seeing
temporary lockups with tip/master and resume issues
with the last line being "Disabling non-boot CPUs ..."
http://marc.info/?l=linux-kernel&m=136610574502492 - this commit was the
culprit.

Good job Jiri.

--
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

2013-04-21 14:48:53

by Thomas Gleixner

[permalink] [raw]
Subject: Re: Resume does not work after timekeeping change

On Sat, 20 Apr 2013, Jiri Slaby wrote:
> Hi,
>
> my machine does not wake from suspend to RAM on my box running the -next
> kernel. The last thing I see is "Disabling non-boot CPUs ...". I
> bisected it to this commit:
>
> commit 7ec98e15aa049b7a2ca73485f31cf4f90c34e2dd
> Author: Thomas Gleixner <[email protected]>
> Date: Thu Feb 21 22:51:39 2013 +0000
>
> timekeeping: Delay update of clock->cycle_last
>
>
> Reverting that one on the top of -next-20130419 makes it work again.

Does the patch below fix the issue?

Thanks,

tglx
---
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 675f720..98cd470 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -951,7 +951,7 @@ static void timekeeping_resume(void)
__timekeeping_inject_sleeptime(tk, &ts_delta);

/* Re-base the last cycle value */
- clock->cycle_last = cycle_now;
+ tk->cycle_last = clock->cycle_last = cycle_now;
tk->ntp_error = 0;
timekeeping_suspended = 0;
timekeeping_update(tk, false, true);

2013-04-21 16:39:14

by Borislav Petkov

[permalink] [raw]
Subject: Re: Resume does not work after timekeeping change

On Sun, Apr 21, 2013 at 04:48:45PM +0200, Thomas Gleixner wrote:
> Does the patch below fix the issue?

It does here.

Applied it ontop of tip/master of today and did three suspend/resume
cycles, back-to-back - looks good and no hickups.

Tested-by: Borislav Petkov <[email protected]>

Thanks.

--
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

2013-04-22 16:05:46

by John Stultz

[permalink] [raw]
Subject: Re: Resume does not work after timekeeping change

On 04/20/2013 08:46 AM, Jiri Slaby wrote:
> Hi,
>
> my machine does not wake from suspend to RAM on my box running the -next
> kernel. The last thing I see is "Disabling non-boot CPUs ...". I
> bisected it to this commit:
>
> commit 7ec98e15aa049b7a2ca73485f31cf4f90c34e2dd
> Author: Thomas Gleixner <[email protected]>
> Date: Thu Feb 21 22:51:39 2013 +0000
>
> timekeeping: Delay update of clock->cycle_last
>
>
> Reverting that one on the top of -next-20130419 makes it work again.
>
>
> I also tried it inside a VM using suspend to disk. There, it behaves
> like it takes a minute or so to wake up. So I tried to wait on the real
> HW too, but it never resumes there.

Thanks for chasing this down! Sorry for the trouble, does the following fix resolve this?

thanks
-john

diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 675f720..94041a9 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -952,6 +952,7 @@ static void timekeeping_resume(void)

/* Re-base the last cycle value */
clock->cycle_last = cycle_now;
+ tk->cycle_last = cycle_now;
tk->ntp_error = 0;
timekeeping_suspended = 0;
timekeeping_update(tk, false, true);

2013-04-22 16:40:50

by John Stultz

[permalink] [raw]
Subject: Re: Resume does not work after timekeeping change

On 04/22/2013 09:05 AM, John Stultz wrote:
> On 04/20/2013 08:46 AM, Jiri Slaby wrote:
>> Hi,
>>
>> my machine does not wake from suspend to RAM on my box running the -next
>> kernel. The last thing I see is "Disabling non-boot CPUs ...". I
>> bisected it to this commit:
>>
>> commit 7ec98e15aa049b7a2ca73485f31cf4f90c34e2dd
>> Author: Thomas Gleixner <[email protected]>
>> Date: Thu Feb 21 22:51:39 2013 +0000
>>
>> timekeeping: Delay update of clock->cycle_last
>>
>>
>> Reverting that one on the top of -next-20130419 makes it work again.
>>
>>
>> I also tried it inside a VM using suspend to disk. There, it behaves
>> like it takes a minute or so to wake up. So I tried to wait on the real
>> HW too, but it never resumes there.
>
> Thanks for chasing this down! Sorry for the trouble, does the
> following fix resolve this?
>
Nevermind, I see Thomas already provided the same fix yesterday.

thanks
-john

2013-04-22 17:02:01

by John Stultz

[permalink] [raw]
Subject: Re: Resume does not work after timekeeping change

On 04/21/2013 07:48 AM, Thomas Gleixner wrote:
> On Sat, 20 Apr 2013, Jiri Slaby wrote:
>> Hi,
>>
>> my machine does not wake from suspend to RAM on my box running the -next
>> kernel. The last thing I see is "Disabling non-boot CPUs ...". I
>> bisected it to this commit:
>>
>> commit 7ec98e15aa049b7a2ca73485f31cf4f90c34e2dd
>> Author: Thomas Gleixner <[email protected]>
>> Date: Thu Feb 21 22:51:39 2013 +0000
>>
>> timekeeping: Delay update of clock->cycle_last
>>
>>
>> Reverting that one on the top of -next-20130419 makes it work again.
> Does the patch below fix the issue?
>
> Thanks,
>
> tglx
> ---
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index 675f720..98cd470 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -951,7 +951,7 @@ static void timekeeping_resume(void)
> __timekeeping_inject_sleeptime(tk, &ts_delta);
>
> /* Re-base the last cycle value */
> - clock->cycle_last = cycle_now;
> + tk->cycle_last = clock->cycle_last = cycle_now;

Looks good.

Acked-by: John Stultz <[email protected]>

Subject: [tip:timers/core] timekeeping: Update tk->cycle_last in resume

Commit-ID: 77c675ba18836802f6b73d2d773481d06ebc0f04
Gitweb: http://git.kernel.org/tip/77c675ba18836802f6b73d2d773481d06ebc0f04
Author: Thomas Gleixner <[email protected]>
AuthorDate: Mon, 22 Apr 2013 09:37:04 +0200
Committer: Thomas Gleixner <[email protected]>
CommitDate: Mon, 22 Apr 2013 20:17:51 +0200

timekeeping: Update tk->cycle_last in resume

commit 7ec98e15aa (timekeeping: Delay update of clock->cycle_last)
forgot to update tk->cycle_last in the resume path. This results in a
stale value versus clock->cycle_last and prevents resume in the worst
case.

Reported-by: Jiri Slaby <[email protected]>
Reported-and-tested-by: Borislav Petkov <[email protected]>
Acked-by: John Stultz <[email protected]>
Cc: Linux-pm mailing list <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Thomas Gleixner <[email protected]>
---
kernel/time/timekeeping.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 675f720..98cd470 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -951,7 +951,7 @@ static void timekeeping_resume(void)
__timekeeping_inject_sleeptime(tk, &ts_delta);

/* Re-base the last cycle value */
- clock->cycle_last = cycle_now;
+ tk->cycle_last = clock->cycle_last = cycle_now;
tk->ntp_error = 0;
timekeeping_suspended = 0;
timekeeping_update(tk, false, true);