[U-Boot] [PATCH] powerpc/lib: fix unsafe register handling in wait_ticks

[U-Boot] [PATCH] powerpc/lib: fix unsafe register handling in wait_ticks

[Mats Kärrman]

If watchdog is enabled, the arch/powerpc/lib/ticks.S::wait_ticks() function
calls the function specified by the WATCHDOG_RESET macro.
The wait_ticks function depends on the registers r0, r6 and r7 being
preserved however that is not guaranteed, e.g. if the reset function is a
C function this will probably overwrite r0 and cause an endless loop.

The following patch changes to using r14+r15 instead of r6+r7 (to resemble
what would have been generated by a C compiler) and saves all necessary
registers on the stack.

The patch has been tested on a custom MPC5125 based machine using the 512x
powerpc architecture.

Signed-off-by: Mats Karrman <mats.karrman@tritech.se>
Cc: Wolfgang Denk <wd@denx.de>
Acked-by: Joakim Tjernlund <joakim.tjernlund@transmode.se>
---
 arch/powerpc/lib/ticks.S |   17 +++++++++++------
 1 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/lib/ticks.S b/arch/powerpc/lib/ticks.S
index 1781039..63114bb 100644
--- a/arch/powerpc/lib/ticks.S
+++ b/arch/powerpc/lib/ticks.S
@@ -50,19 +50,24 @@ wait_ticks:
 	stwu	r1, -16(r1)
 	mflr	r0		/* save link register */
 	stw	r0, 20(r1)	/* Use r0 or GDB will be unhappy */
-	mr	r7, r3		/* save tick count */
+	stw	r14, 12(r1)	/* save used registers */
+	stw	r15, 8(r1)
+	mr	r14, r3		/* save tick count */
 	bl	get_ticks	/* Get start time */
 
 	/* Calculate end time */
-	addc	r7, r4, r7	/* Compute end time lower */
-	addze	r6, r3		/*     and end time upper */
+	addc	r14, r4, r14	/* Compute end time lower */
+	addze	r15, r3		/*     and end time upper */
 
 	WATCHDOG_RESET		/* Trigger watchdog, if needed */
 1:	bl	get_ticks	/* Get current time */
-	subfc	r4, r4, r7	/* Subtract current time from end time */
-	subfe.	r3, r3, r6
+	subfc	r4, r4, r14	/* Subtract current time from end time */
+	subfe.	r3, r3, r15
 	bge	1b		/* Loop until time expired */
 
-	mtlr	r0		/* restore link register */
+	lwz	r15, 8(r1)	/* restore saved registers */
+	lwz	r14, 12(r1)
+	lwz	r0, 20(r1)
 	addi	r1,r1,16
+	mtlr	r0
 	blr
-- 
1.7.1

[U-Boot] [PATCH] powerpc/lib: fix unsafe register handling in wait_ticks

[Stefan Roese]

On 01/27/2013 06:03 PM, Mats K?rrman wrote:
> If watchdog is enabled, the arch/powerpc/lib/ticks.S::wait_ticks() function
> calls the function specified by the WATCHDOG_RESET macro.
> The wait_ticks function depends on the registers r0, r6 and r7 being
> preserved however that is not guaranteed, e.g. if the reset function is a
> C function this will probably overwrite r0 and cause an endless loop.
> 
> The following patch changes to using r14+r15 instead of r6+r7 (to resemble
> what would have been generated by a C compiler) and saves all necessary
> registers on the stack.
> 
> The patch has been tested on a custom MPC5125 based machine using the 512x
> powerpc architecture.
> 
> Signed-off-by: Mats Karrman <mats.karrman@tritech.se>
> Cc: Wolfgang Denk <wd@denx.de>
> Acked-by: Joakim Tjernlund <joakim.tjernlund@transmode.se>

Thanks for this fix. I just reproduced this problem on another MPC5200
based board (a4m2k) while implementing watchdog support. And this patch
fixes the problem (hangup in wait_ticks).

So:

Tested-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

[U-Boot] [PATCH] powerpc/lib: fix unsafe register handling in wait_ticks

[Joakim Tjernlund]

> 
> On 01/27/2013 06:03 PM, Mats K?rrman wrote:
> > If watchdog is enabled, the arch/powerpc/lib/ticks.S::wait_ticks() 
function
> > calls the function specified by the WATCHDOG_RESET macro.
> > The wait_ticks function depends on the registers r0, r6 and r7 being
> > preserved however that is not guaranteed, e.g. if the reset function 
is a
> > C function this will probably overwrite r0 and cause an endless loop.
> > 
> > The following patch changes to using r14+r15 instead of r6+r7 (to 
resemble
> > what would have been generated by a C compiler) and saves all 
necessary
> > registers on the stack.
> > 
> > The patch has been tested on a custom MPC5125 based machine using the 
512x
> > powerpc architecture.
> > 
> > Signed-off-by: Mats Karrman <mats.karrman@tritech.se>
> > Cc: Wolfgang Denk <wd@denx.de>
> > Acked-by: Joakim Tjernlund <joakim.tjernlund@transmode.se>
> 
> Thanks for this fix. I just reproduced this problem on another MPC5200
> based board (a4m2k) while implementing watchdog support. And this patch
> fixes the problem (hangup in wait_ticks).
> 
> So:
> 
> Tested-by: Stefan Roese <sr@denx.de>

If someone wants to play with a C only impl. I THINK
it this is a good starting point:

#define WATCHDOG_RESET

/*static inline */unsigned long long get_ticks(void)
{
        union xx {
                unsigned long long r3_r4;
                struct yx {
                        unsigned long r3;
                        unsigned long r4;
                } xy;
        } res;
        register unsigned long tmp;

        do {
                asm ("mftbu %0\n\t"
                     "mftb %1\n\t"
                     "mftbu %2"
                     : "=r"(res.xy.r3), "=r"(res.xy.r4), "=r"(tmp));
        } while (res.xy.r3 != tmp);
        return res.r3_r4;
}

wait_ticks(unsigned long wt)
{
        unsigned long long ticks_end;

        ticks_end = get_ticks() + wt;
        WATCHDOG_RESET;
        do {

        } while((signed long long)ticks_end - (signed long 
long)get_ticks() >= 0);
}

[U-Boot] powerpc/lib: fix unsafe register handling in wait_ticks

[Tom Rini]

On Sun, Jan 27, 2013 at 07:03:44AM -0000, =?utf-8?b?TWF0cyBLw6Rycm1hbiA8TWF0cy5LYXJybWFuQHRyaXRlY2guc2U+?= wrote:

> If watchdog is enabled, the arch/powerpc/lib/ticks.S::wait_ticks() function
> calls the function specified by the WATCHDOG_RESET macro.
> The wait_ticks function depends on the registers r0, r6 and r7 being
> preserved however that is not guaranteed, e.g. if the reset function is a
> C function this will probably overwrite r0 and cause an endless loop.
> 
> The following patch changes to using r14+r15 instead of r6+r7 (to resemble
> what would have been generated by a C compiler) and saves all necessary
> registers on the stack.
> 
> The patch has been tested on a custom MPC5125 based machine using the 512x
> powerpc architecture.
> 
> Signed-off-by: Mats Karrman <mats.karrman@tritech.se>
> Cc: Wolfgang Denk <wd@denx.de>
> Acked-by: Joakim Tjernlund <joakim.tjernlund@transmode.se>
> Tested-by: Stefan Roese <sr@denx.de>

This does not cleanly apply to u-boot/master, can you please resend
based on top of tree?  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130408/109da4e3/attachment.pgp>