Re: [U-Boot] imx: get_ticks in syscounter.c get miscompiled by GCC 6
Hi, On Wed, Mar 7, 2018 at 11:27 PM, Tom Riniwrote: > On Wed, Mar 07, 2018 at 10:42:44AM -0300, Fabio Estevam wrote: >> Patch looks good. Make sure to add your Signed-off-by line, then you >> can send it via git send-email. > > Yes please, thanks! I've sent it to you with my signed-of-by. thanks, -- yashi ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] imx: get_ticks in syscounter.c get miscompiled by GCC 6
On Wed, Mar 07, 2018 at 10:42:44AM -0300, Fabio Estevam wrote: > Hi Yasushi , > > On Wed, Mar 7, 2018 at 2:57 AM, Yasushi SHOJIwrote: > > > Do you guys really want to put volatile on all of these now? > > We are at rc4 and Tom is planing to cut the release > > March 12th. > > This can be done at a later step. Yes. And it should be a little bit manual too. For example, using your regex (thanks!) I see we have some powerpc code that's doing asm("eieio") and that should be eieio() (which is in turn an asm volatile ...), as well as some sync;isync or just sync/isync that should be sync();isync(); or similar. And people that know x86 might have some useful comments there too. > > I'm attaching a tentative patch to fix only syscounter.c. > > If it looks good, I'l resend it by git-send-email. > > Patch looks good. Make sure to add your Signed-off-by line, then you > can send it via git send-email. Yes please, thanks! -- Tom signature.asc Description: PGP signature ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] imx: get_ticks in syscounter.c get miscompiled by GCC 6
Hi Yasushi , On Wed, Mar 7, 2018 at 2:57 AM, Yasushi SHOJIwrote: > Do you guys really want to put volatile on all of these now? > We are at rc4 and Tom is planing to cut the release > March 12th. This can be done at a later step. > I'm attaching a tentative patch to fix only syscounter.c. > If it looks good, I'l resend it by git-send-email. Patch looks good. Make sure to add your Signed-off-by line, then you can send it via git send-email. Thanks ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] imx: get_ticks in syscounter.c get miscompiled by GCC 6
Hi, On Tue, Mar 6, 2018 at 10:11 PM, Fabio Estevamwrote: > On Tue, Mar 6, 2018 at 9:31 AM, Lothar Waßmann > wrote: > >> Without the 'volatile' attribute the compiler is entitled to move the >> asm code around or optimize it out. >> So, your patch is the correct fix independent from the gcc version >> used. > > Yes, but then it would be better to fix all the places where asm is > used without volatile. That'd be a quite big patch. A quick grep shows me that there is over 100 asm() without volatile. git grep -e '\basm *(' | grep -v -e volatile -e nop | wc -l 153 Do you guys really want to put volatile on all of these now? We are at rc4 and Tom is planing to cut the release March 12th. I'm attaching a tentative patch to fix only syscounter.c. If it looks good, I'l resend it by git-send-email. Best, -- yashi From 8772a9b5b532da4c1d0656ffa21f324e72d369e8 Mon Sep 17 00:00:00 2001 From: Yasushi SHOJI Date: Wed, 7 Mar 2018 14:38:05 +0900 Subject: [PATCH] imx: syscounter: make sure asm is volatile Without the volatile attribute, compilers are entitled to optimize out the same asm(). In the case of __udelay() in syscounter.c, it calls `get_ticks()` twice, one for the starting time and the second in the loop to check the current time. When compilers inline `get_ticks()` they see the same `mrrc` instructions and optimize out the second one. This leads to infinite loop since we don't get updated value from the system counter. Here is a portion of the disassembly of __udelay: 88: 428b cmp r3, r1 8a: f8ce 20a4 str.w r2, [lr, #164] ; 0xa4 8e: bf08 it eq 90: 4282 cmpeq r2, r0 92: f8ce 30a0 str.w r3, [lr, #160] ; 0xa0 96: d3f7 bcc.n 88 <__udelay+0x88> 98: e8bd 8cf0 ldmia.w sp!, {r4, r5, r6, r7, sl, fp, pc} Note that final jump / loop at 96 to 88, we don't have any `mrrc`. With a volatile attribute, the above changes to this: 8a: ec53 2f0e mrrc 15, 0, r2, r3, cr14 8e: 42ab cmp r3, r5 90: f8c1 20a4 str.w r2, [r1, #164] ; 0xa4 94: bf08 it eq 96: 42a2 cmpeq r2, r4 98: f8c1 30a0 str.w r3, [r1, #160] ; 0xa0 9c: d3f5 bcc.n 8a <__udelay+0x8a> 9e: e8bd 8cf0 ldmia.w sp!, {r4, r5, r6, r7, sl, fp, pc} a2: bf00 nop I'm advised to put volatile on all asm(), so this commit also adds it to the asm() in timer_init(). --- arch/arm/mach-imx/syscounter.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/arch/arm/mach-imx/syscounter.c b/arch/arm/mach-imx/syscounter.c index 9290918dca..1d4ebfe343 100644 --- a/arch/arm/mach-imx/syscounter.c +++ b/arch/arm/mach-imx/syscounter.c @@ -62,7 +62,7 @@ int timer_init(void) unsigned long val, freq; freq = CONFIG_SC_TIMER_CLK; - asm("mcr p15, 0, %0, c14, c0, 0" : : "r" (freq)); + asm volatile("mcr p15, 0, %0, c14, c0, 0" : : "r" (freq)); writel(freq, >cntfid0); @@ -82,7 +82,7 @@ unsigned long long get_ticks(void) { unsigned long long now; - asm("mrrc p15, 0, %Q0, %R0, c14" : "=r" (now)); + asm volatile("mrrc p15, 0, %Q0, %R0, c14" : "=r" (now)); gd->arch.tbl = (unsigned long)(now & 0x); gd->arch.tbu = (unsigned long)(now >> 32); -- 2.16.2 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] imx: get_ticks in syscounter.c get miscompiled by GCC 6
On Tue, Mar 6, 2018 at 9:31 AM, Lothar Waßmannwrote: > Without the 'volatile' attribute the compiler is entitled to move the > asm code around or optimize it out. > So, your patch is the correct fix independent from the gcc version > used. Yes, but then it would be better to fix all the places where asm is used without volatile. ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] imx: get_ticks in syscounter.c get miscompiled by GCC 6
Hi, On Tue, 6 Mar 2018 15:06:08 +0900 Yasushi SHOJI wrote: > Hi, > > It seems to me that both GCC 6.3 and 6.4 mis-compiles > s/mis-compiles/optimizes/ Without the 'volatile' attribute the compiler is entitled to move the asm code around or optimize it out. So, your patch is the correct fix independent from the gcc version used. > arch/arm/mach-imx/syscounter.c. > > I'm attaching two files, bad.txt is the original syscounter.c and > good.txt is the one > with the following patch. > > diff --git a/arch/arm/mach-imx/syscounter.c b/arch/arm/mach-imx/syscounter.c > index 9290918dca..30ed0109a2 100644 > --- a/arch/arm/mach-imx/syscounter.c > +++ b/arch/arm/mach-imx/syscounter.c > @@ -82,7 +82,7 @@ unsigned long long get_ticks(void) > { > unsigned long long now; > > - asm("mrrc p15, 0, %Q0, %R0, c14" : "=r" (now)); > + asm volatile("mrrc p15, 0, %Q0, %R0, c14" : "=r" (now)); > > gd->arch.tbl = (unsigned long)(now & 0x); > gd->arch.tbu = (unsigned long)(now >> 32); > Lothar Waßmann ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot