[U-Boot] [PATCH] net: fec_mxc: Fix timeouts during tftp transfer

[U-Boot] [PATCH] net: fec_mxc: Fix timeouts during tftp transfer

[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);
 
 	/*
 	 * Check if any critical events have happened
-- 
1.8.1.2

[U-Boot] [PATCH] net: fec_mxc: Fix timeouts during tftp transfer

[Hector Palacios]

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

[U-Boot] [PATCH] net: fec_mxc: Fix timeouts during tftp transfer

[Marek Vasut]

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

[U-Boot] [PATCH] net: fec_mxc: Fix timeouts during tftp transfer

[Benoît Thébaudeau]

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

[U-Boot] [PATCH] net: fec_mxc: Fix timeouts during tftp transfer

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Brain off-line, please wait.

[U-Boot] [PATCH] net: fec_mxc: Fix timeouts during tftp transfer

[Benoît Thébaudeau]

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

[U-Boot] [PATCH] net: fec_mxc: Fix timeouts during tftp transfer

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Well, the way I see it, logic is only a way of being ignorant by num-
bers.                                 - Terry Pratchett, _Small Gods_

[U-Boot] [PATCH] net: fec_mxc: Fix timeouts during tftp transfer

[Fabio Estevam]

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