On Fri, Feb 19, 2016 at 04:20:50PM +0000, James Morse wrote:

[...]

quoted

quoted

diff --git a/arch/arm64/kernel/sleep.S b/arch/arm64/kernel/sleep.S
index dca81612fe90..0e2b36f1fb44 100644
--- a/arch/arm64/kernel/sleep.S
+++ b/arch/arm64/kernel/sleep.S

@@ -73,8 +73,8 @@ ENTRY(__cpu_suspend_enter)
 	str	x2, [x0, #SLEEP_STACK_DATA_SYSTEM_REGS + CPU_CTX_SP]
 
 	/* find the mpidr_hash */
-	ldr	x1, =sleep_save_sp
-	ldr	x1, [x1, #SLEEP_SAVE_SP_VIRT]
+	ldr	x1, =sleep_save_stash
+	ldr	x1, [x1]
 	mrs	x7, mpidr_el1
 	ldr	x9, =mpidr_hash
 	ldr	x10, [x9, #MPIDR_HASH_MASK]

@@ -87,40 +87,26 @@ ENTRY(__cpu_suspend_enter)
 	compute_mpidr_hash x8, x3, x4, x5, x6, x7, x10
 	add	x1, x1, x8, lsl #3
 
+	str	x0, [x1]
+	add	x0, x0, #SLEEP_STACK_DATA_SYSTEM_REGS

Mmm...this instruction does not really belong in this patch,
it should be part of patch 6, correct ? What I mean is, the new
struct to stash system regs (struct sleep_stack_data) was added in
patch 6, if the offset #SLEEP_STACK_DATA_SYSTEM_REGS (which is 0) had
to be added it had to be added in patch 6 too, it does not belong
in this patch, am I right ?

In the previous patch __cpu_suspend_save() was changed to take a
struct sleep_stack_data, this then passes system_regs to
cpu_do_suspend(). The 'add	x0, x0, #SLEEP_STACK_DATA_SYSTEM_REGS'
is being done in C, (and hopefully optimised out by the compiler).

In this patch, we removed __cpu_suspend_save() and call cpu_do_suspend()
directly, so need to account for system_regs's position in struct
sleep_stack_data ourselves.

Yes, I missed that during the review, that explains, thanks.

I'm paranoid, but if you prefer I can remove the 'add 0', and dump a
comment next to the struct definition making it someone elses problem
to fix it if they ever change the layout of the struct...

No, what you do is correct and should be kept as-is, I thought it
had to be added in patch 6 but I was wrong, see above, code is
correct as it is.

Thanks,
Lorenzo

quoted

quoted

 	push	x29, lr
-	bl	__cpu_suspend_save
+	bl	cpu_do_suspend
 	pop	x29, lr
 	mov	x0, #1
 	ret
 ENDPROC(__cpu_suspend_enter)
 	.ltorg

[...2x Nits fixed...]

quoted

With the last updates it seems fine by me, so:

Reviewed-by: Lorenzo Pieralisi <redacted>

Thanks!


James