Re: [Bisected] 3.7-rc1 can't resume (still present in 3.9)

[Jonas Heinrich <onny@project-insanity.org>]

[-- Attachment #1.1: Type: text/plain, Size: 2551 bytes --]

On 05-02 02:45, Rafael J. Wysocki wrote:
> On Wednesday, May 01, 2013 11:55:10 AM H. Peter Anvin wrote:
> > On 05/01/2013 11:51 AM, Jonas Heinrich wrote:
> > > Well, you could give me instructions on how to debug this (I'll do 
> > > everything ;)) or I could ship you the Thinkpad T43. I guess this
> > > would worth the effort since this bug is somehow critical.
> > > 
> > > Best regards, Jonas
> > 
> > I'll put together a debug patch unless I can trick Rafael into doing
> > it first...
> 
> I'm afraid that code has changed quite a bit since I looked at it last time.
> [Jarkko Sakkinen seems to have worked on it lately, CCed.]
> 
> Jonas, I wonder what happens if you drop the first hunk of the patch (it just
> uses a different register, which shouldn't matter)?  Does it still help then?

Hello Rafel, first of all, thank you for helping me out :)
You're right, the patch still solves the suspend bug, after removing the first 
hunk of the patch and applying it (see attachement:
suspendfix_first_hunk_dropped.patch).

> 
> If so, there are still a few things you can do to it, e.g:
> (1) drop the
> 
> -       btl     $WAKEUP_BEHAVIOR_RESTORE_CR4, %edi
> -       jnc     1f
> 

Still works :) (used suspendfix_1.patch)

> lines,
> (2) drop the
> 
> -       btl     $WAKEUP_BEHAVIOR_RESTORE_EFER, %edi
> -       jnc     1f
> 
> lines,

Still works :) (used suspendfix_2.patch)

> (3) drop the
> 
> +       jecxz   1f
> 

Still works :) (used suspendfix_3.patch)

> line,
> (4) drop the
> 
> +       movl    %eax, %ecx
> +       orl     %edx, %ecx
> +       jz      1f
> 

At this point, the bug reoccurs (used suspendfix_4.patch)! 
But that doesn't mean these lines are the only critical, because the more
minimal patch

@@ -119,6 +119,9 @@
        jnc     1f
        movl    pmode_efer, %eax
        movl    pmode_efer + 4, %edx
+       movl    %eax, %ecx
+       orl     %edx, %ecx
+       jz      1f
        movl    $MSR_EFER, %ecx
        wrmsr
 1:


with removing this part

-       movl    pmode_cr4, %eax
-       movl    %eax, %cr4
+       movl    pmode_cr4, %ecx
+       movl    %ecx, %cr4

also doesn't fix the issue (see suspendfix_5.patch).

> lines and see what the minimal patch needed for things to work again is.
> 

So the most minimal working patch is suspendfix_3.patch.

> Thanks,
> Rafael

Thank you and best regards,
Jonas

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

[-- Attachment #1.2: suspendfix_first_hunk_dropped.patch --]
[-- Type: text/plain, Size: 569 bytes --]

--- a/arch/x86/realmode/rm/wakeup_asm.S	2013-04-29 02:36:01.000000000 +0200
+++ b/arch/x86/realmode/rm/wakeup_asm.S	2013-05-01 11:42:07.359999895 +0200
@@ -110,15 +110,15 @@
 	movl	pmode_cr3, %eax
 	movl	%eax, %cr3
 
-	btl	$WAKEUP_BEHAVIOR_RESTORE_CR4, %edi
-	jnc	1f
-	movl	pmode_cr4, %eax
-	movl	%eax, %cr4
+	movl	pmode_cr4, %ecx
+	jecxz	1f
+	movl	%ecx, %cr4
 1:
-	btl	$WAKEUP_BEHAVIOR_RESTORE_EFER, %edi
-	jnc	1f
 	movl	pmode_efer, %eax
 	movl	pmode_efer + 4, %edx
+	movl	%eax, %ecx
+	orl	%edx, %ecx
+	jz	1f
 	movl	$MSR_EFER, %ecx
 	wrmsr
 1:

[-- Attachment #1.3: suspendfix_1.patch --]
[-- Type: text/plain, Size: 526 bytes --]

--- a/arch/x86/realmode/rm/wakeup_asm.S 2013-05-02 17:20:25.266667541 +0200
+++ b/arch/x86/realmode/rm/wakeup_asm.S	2013-05-02 17:22:51.880000874 +0200
@@ -112,13 +112,15 @@
 
 	btl	$WAKEUP_BEHAVIOR_RESTORE_CR4, %edi
 	jnc	1f
-	movl	pmode_cr4, %eax
-	movl	%eax, %cr4
+	movl	pmode_cr4, %ecx
+	jecxz	1f
+	movl	%ecx, %cr4
 1:
-	btl	$WAKEUP_BEHAVIOR_RESTORE_EFER, %edi
-	jnc	1f
 	movl	pmode_efer, %eax
 	movl	pmode_efer + 4, %edx
+	movl	%eax, %ecx
+	orl	%edx, %ecx
+	jz	1f
 	movl	$MSR_EFER, %ecx
 	wrmsr
 1:

[-- Attachment #1.4: suspendfix_2.patch --]
[-- Type: text/plain, Size: 526 bytes --]

--- a/arch/x86/realmode/rm/wakeup_asm.S	2013-05-02 17:20:25.266667541 +0200
+++ b/arch/x86/realmode/rm/wakeup_asm.S	2013-05-02 18:54:49.176667541 +0200
@@ -112,13 +112,17 @@
 
 	btl	$WAKEUP_BEHAVIOR_RESTORE_CR4, %edi
 	jnc	1f
-	movl	pmode_cr4, %eax
-	movl	%eax, %cr4
+	movl	pmode_cr4, %ecx
+	jecxz	1f
+	movl	%ecx, %cr4
 1:
 	btl	$WAKEUP_BEHAVIOR_RESTORE_EFER, %edi
 	jnc	1f
 	movl	pmode_efer, %eax
 	movl	pmode_efer + 4, %edx
+	movl	%eax, %ecx
+	orl	%edx, %ecx
+	jz	1f
 	movl	$MSR_EFER, %ecx
 	wrmsr
 1:

[-- Attachment #1.5: suspendfix_3.patch --]
[-- Type: text/plain, Size: 518 bytes --]

--- a/arch/x86/realmode/rm/wakeup_asm.S.org	2013-05-02 17:20:25.266667541 +0200
+++ b/arch/x86/realmode/rm/wakeup_asm.S	2013-05-02 19:56:58.083334207 +0200
@@ -112,13 +112,16 @@
 
 	btl	$WAKEUP_BEHAVIOR_RESTORE_CR4, %edi
 	jnc	1f
-	movl	pmode_cr4, %eax
-	movl	%eax, %cr4
+	movl	pmode_cr4, %ecx
+	movl	%ecx, %cr4
 1:
 	btl	$WAKEUP_BEHAVIOR_RESTORE_EFER, %edi
 	jnc	1f
 	movl	pmode_efer, %eax
 	movl	pmode_efer + 4, %edx
+	movl	%eax, %ecx
+	orl	%edx, %ecx
+	jz	1f
 	movl	$MSR_EFER, %ecx
 	wrmsr
 1:

[-- Attachment #1.6: suspendfix_4.patch --]
[-- Type: text/plain, Size: 374 bytes --]

--- a/arch/x86/realmode/rm/wakeup_asm.S	2013-05-02 17:20:25.266667541 +0200
+++ b/arch/x86/realmode/rm/wakeup_asm.S	2013-05-02 20:43:40.623334207 +0200
@@ -112,8 +112,8 @@
 
 	btl	$WAKEUP_BEHAVIOR_RESTORE_CR4, %edi
 	jnc	1f
-	movl	pmode_cr4, %eax
-	movl	%eax, %cr4
+	movl	pmode_cr4, %ecx
+	movl	%ecx, %cr4
 1:
 	btl	$WAKEUP_BEHAVIOR_RESTORE_EFER, %edi
 	jnc	1f

[-- Attachment #1.7: suspendfix_5.patch --]
[-- Type: text/plain, Size: 311 bytes --]

--- a/arch/x86/realmode/rm/wakeup_asm.S	2013-05-02 17:20:25.266667541 +0200
+++ b/arch/x86/realmode/rm/wakeup_asm.S	2013-05-02 21:44:57.973334207 +0200
@@ -119,6 +119,9 @@
 	jnc	1f
 	movl	pmode_efer, %eax
 	movl	pmode_efer + 4, %edx
+	movl	%eax, %ecx
+	orl	%edx, %ecx
+	jz	1f
 	movl	$MSR_EFER, %ecx
 	wrmsr
 1:

[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]