| Message ID | 1379293803-30650-1-git-send-email-festevam@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Stefano Babic |
| Headers | show |
On 09/16/2013 03:10 AM, Fabio Estevam wrote: > From: Fabio Estevam <fabio.estevam@freescale.com> > > Performing tftp transfers on mx28 results in random timeouts. > > Hector Palacios and Robert Hodaszi analyzed the root cause being related to the > alignment of the 'buff' buffer inside fec_recv(). > > GCC versions such as 4.4/4.5 are more likely to exhibit such problem. > > Use ALLOC_CACHE_ALIGN_BUFFER() for making the proper alignment of buffer. > > Reported-by: Hector Palacios <hector.palacios@digi.com> > Tested-by: Oliver Metz <oliver@freetz.org> > Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com> > --- > drivers/net/fec_mxc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c > index 690e572..b423ff6 100644 > --- a/drivers/net/fec_mxc.c > +++ b/drivers/net/fec_mxc.c > @@ -794,7 +794,7 @@ static int fec_recv(struct eth_device *dev) > uint16_t bd_status; > uint32_t addr, size, end; > int i; > - uchar buff[FEC_MAX_PKT_SIZE] __aligned(ARCH_DMA_MINALIGN); > + ALLOC_CACHE_ALIGN_BUFFER(uchar, buff, FEC_MAX_PKT_SIZE); > > /* > * Check if any critical events have happened > Tested-by: Hector Palacios <hector.palacios@digi.com> Best regards, -- Hector Palacios
Dear Fabio Estevam, > From: Fabio Estevam <fabio.estevam@freescale.com> > > Performing tftp transfers on mx28 results in random timeouts. > > Hector Palacios and Robert Hodaszi analyzed the root cause being related to > the alignment of the 'buff' buffer inside fec_recv(). > > GCC versions such as 4.4/4.5 are more likely to exhibit such problem. > > Use ALLOC_CACHE_ALIGN_BUFFER() for making the proper alignment of buffer. > > Reported-by: Hector Palacios <hector.palacios@digi.com> > Tested-by: Oliver Metz <oliver@freetz.org> > Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com> > --- > drivers/net/fec_mxc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c > index 690e572..b423ff6 100644 > --- a/drivers/net/fec_mxc.c > +++ b/drivers/net/fec_mxc.c > @@ -794,7 +794,7 @@ static int fec_recv(struct eth_device *dev) > uint16_t bd_status; > uint32_t addr, size, end; > int i; > - uchar buff[FEC_MAX_PKT_SIZE] __aligned(ARCH_DMA_MINALIGN); > + ALLOC_CACHE_ALIGN_BUFFER(uchar, buff, FEC_MAX_PKT_SIZE); OK, it's gonna be safer this way, still what's the root cause of the issue? FEC_MAX_PKT_SIZE is 1536 (which is aligned to 32b and even 64b) __aligned(ARCH_DMA_MINALIGN) aligns the stuff to 32b or 64b depending on CPU So how can the above not properly align the buffer? Is that a compiler bug then? btw. using ALLOC_CACHE_ALIGN_BUFFER will be safer were someone to change FEC_MAX_PKT_SIZE, the buffer would still be safe for cache flush/inval ops. Best regards, Marek Vasut
Hi Marek, On Tuesday, September 17, 2013 5:00:58 AM, Marek Vasut wrote: > Dear Fabio Estevam, > > > From: Fabio Estevam <fabio.estevam@freescale.com> > > > > Performing tftp transfers on mx28 results in random timeouts. > > > > Hector Palacios and Robert Hodaszi analyzed the root cause being related to > > the alignment of the 'buff' buffer inside fec_recv(). > > > > GCC versions such as 4.4/4.5 are more likely to exhibit such problem. > > > > Use ALLOC_CACHE_ALIGN_BUFFER() for making the proper alignment of buffer. > > > > Reported-by: Hector Palacios <hector.palacios@digi.com> > > Tested-by: Oliver Metz <oliver@freetz.org> > > Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com> > > --- > > drivers/net/fec_mxc.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c > > index 690e572..b423ff6 100644 > > --- a/drivers/net/fec_mxc.c > > +++ b/drivers/net/fec_mxc.c > > @@ -794,7 +794,7 @@ static int fec_recv(struct eth_device *dev) > > uint16_t bd_status; > > uint32_t addr, size, end; > > int i; > > - uchar buff[FEC_MAX_PKT_SIZE] __aligned(ARCH_DMA_MINALIGN); > > + ALLOC_CACHE_ALIGN_BUFFER(uchar, buff, FEC_MAX_PKT_SIZE); > > OK, it's gonna be safer this way, still what's the root cause of the issue? > > FEC_MAX_PKT_SIZE is 1536 (which is aligned to 32b and even 64b) > __aligned(ARCH_DMA_MINALIGN) aligns the stuff to 32b or 64b depending on CPU > > So how can the above not properly align the buffer? Is that a compiler bug > then? > > btw. using ALLOC_CACHE_ALIGN_BUFFER will be safer were someone to change > FEC_MAX_PKT_SIZE, the buffer would still be safe for cache flush/inval ops. I have encountered the same kind of alignment issue recently on something else. It looks like GCC for ARM just silently ignores the aligned attribute for auto (stacked) variables. Best regards, Benoît
Dear Benoît Thébaudeau, In message <1135126743.1842859.1379415574013.JavaMail.zimbra@advansee.com> you wrote: > > > So how can the above not properly align the buffer? Is that a compiler bug > > then? > > > > btw. using ALLOC_CACHE_ALIGN_BUFFER will be safer were someone to change > > FEC_MAX_PKT_SIZE, the buffer would still be safe for cache flush/inval ops. > > I have encountered the same kind of alignment issue recently on something else. > It looks like GCC for ARM just silently ignores the aligned attribute for auto > (stacked) variables. I would really like to see the generated code from such a system, so we verify if this is indeed true, or if something else is causing such issues. Even if the suggested patch fixes the current problem, it leaves a bad feeling as it's only based on speculation about the causes. Best regards, Wolfgang Denk
Dear Wolfgang Denk, On Tuesday, September 17, 2013 1:47:02 PM, Wolfgang Denk wrote: > Dear Benoît Thébaudeau, > > In message <1135126743.1842859.1379415574013.JavaMail.zimbra@advansee.com> > you wrote: > > > > > So how can the above not properly align the buffer? Is that a compiler > > > bug > > > then? > > > > > > btw. using ALLOC_CACHE_ALIGN_BUFFER will be safer were someone to change > > > FEC_MAX_PKT_SIZE, the buffer would still be safe for cache flush/inval > > > ops. > > > > I have encountered the same kind of alignment issue recently on something > > else. > > It looks like GCC for ARM just silently ignores the aligned attribute for > > auto > > (stacked) variables. > > I would really like to see the generated code from such a system, so > we verify if this is indeed true, or if something else is causing such > issues. > > Even if the suggested patch fixes the current problem, it leaves a bad > feeling as it's only based on speculation about the causes. Here is a basic alignment test that I have run on ARM: --- #include <stdio.h> void foo(void *var) { printf("var=0x%.8x\n", (int)var); } int main(void) { unsigned char var[1536] __attribute__((__aligned__(64))); unsigned int i; for (i = 0; i < 10; i++) foo(&var); return 0; } --- I have built it using: $ cross-gcc align.c -o align With GCC 4.5.4, the kind of output that I get is 'var=0x7ee1a6b8' (i.e. not aligned as requested). The generated asm is: --- 0000849c <main>: 849c: e92d4800 push {fp, lr} 84a0: e28db004 add fp, sp, #4 84a4: e24ddc06 sub sp, sp, #1536 ; 0x600 84a8: e24dd008 sub sp, sp, #8 84ac: e3a03000 mov r3, #0 84b0: e50b3008 str r3, [fp, #-8] 84b4: ea000007 b 84d8 <main+0x3c> 84b8: e24b3c06 sub r3, fp, #1536 ; 0x600 84bc: e2433004 sub r3, r3, #4 84c0: e2433008 sub r3, r3, #8 84c4: e1a00003 mov r0, r3 84c8: ebffffe7 bl 846c <foo> 84cc: e51b3008 ldr r3, [fp, #-8] 84d0: e2833001 add r3, r3, #1 84d4: e50b3008 str r3, [fp, #-8] 84d8: e51b3008 ldr r3, [fp, #-8] 84dc: e3530009 cmp r3, #9 84e0: 9afffff4 bls 84b8 <main+0x1c> 84e4: e3a03000 mov r3, #0 84e8: e1a00003 mov r0, r3 84ec: e24bd004 sub sp, fp, #4 84f0: e8bd8800 pop {fp, pc} --- With GCC 4.6.2, the kind of output that I get is 'var=0x7e808680' (i.e. aligned as requested). The generated asm is: --- 000083a4 <main>: 83a4: e92d4810 push {r4, fp, lr} 83a8: e28db008 add fp, sp, #8 83ac: e24dd00c sub sp, sp, #12 83b0: e24ddd19 sub sp, sp, #1600 ; 0x640 83b4: e1a0300d mov r3, sp 83b8: e283303f add r3, r3, #63 ; 0x3f 83bc: e1a03323 lsr r3, r3, #6 83c0: e1a04303 lsl r4, r3, #6 83c4: e3a03000 mov r3, #0 83c8: e50b3010 str r3, [fp, #-16] 83cc: ea000004 b 83e4 <main+0x40> 83d0: e1a00004 mov r0, r4 83d4: ebffffe6 bl 8374 <foo> 83d8: e51b3010 ldr r3, [fp, #-16] 83dc: e2833001 add r3, r3, #1 83e0: e50b3010 str r3, [fp, #-16] 83e4: e51b3010 ldr r3, [fp, #-16] 83e8: e3530009 cmp r3, #9 83ec: 9afffff7 bls 83d0 <main+0x2c> 83f0: e3a03000 mov r3, #0 83f4: e1a00003 mov r0, r3 83f8: e24bd008 sub sp, fp, #8 83fc: e8bd8810 pop {r4, fp, pc} --- I did not succeed to duplicate the issue with GCC 4.6.2, while GCC 4.5.4 almost always produces an unexpected alignment for auto variables. Best regards, Benoît
Dear Benoît, In message <16119766.1853628.1379428952428.JavaMail.zimbra@advansee.com> you wrote: > > Here is a basic alignment test that I have run on ARM: ... > I did not succeed to duplicate the issue with GCC 4.6.2, while GCC 4.5.4 almost > always produces an unexpected alignment for auto variables. Thanks a lot, I highly appreciate your efforts. Yes, this is really a good indication that we are fighting against a compiler bug. Best regards, Wolfgang Denk
Hi Wolfgang, On Tue, Sep 17, 2013 at 4:27 PM, Wolfgang Denk <wd@denx.de> wrote: > Dear Benoît, > > In message <16119766.1853628.1379428952428.JavaMail.zimbra@advansee.com> you wrote: >> >> Here is a basic alignment test that I have run on ARM: > ... >> I did not succeed to duplicate the issue with GCC 4.6.2, while GCC 4.5.4 almost >> always produces an unexpected alignment for auto variables. > > Thanks a lot, I highly appreciate your efforts. Yes, this is really a > good indication that we are fighting against a compiler bug. Yes, very good analysis. Will send a v2 containig Benoît's report in the commit log. Regards, Fabio Estevam
diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c index 690e572..b423ff6 100644 --- a/drivers/net/fec_mxc.c +++ b/drivers/net/fec_mxc.c @@ -794,7 +794,7 @@ static int fec_recv(struct eth_device *dev) uint16_t bd_status; uint32_t addr, size, end; int i; - uchar buff[FEC_MAX_PKT_SIZE] __aligned(ARCH_DMA_MINALIGN); + ALLOC_CACHE_ALIGN_BUFFER(uchar, buff, FEC_MAX_PKT_SIZE); /* * Check if any critical events have happened