[U-Boot] [PATCH] Align linebuf to avoid misaligned aliases of it

[U-Boot] [PATCH] Align linebuf to avoid misaligned aliases of it

[Albert Aribaud]

Commit  64419e47518bbba059c80b77558f93ad4804145c aliases
the uint16_t usp and uint32_t uip variables in print_buffer()
to uint8_t variable linebuf without aligning it to an uint32_t
address, thus causing data aborts on ARM when doing md.l on
32-bit wide area (and probably 16-bit wide as well).

Aligning linebuf fixes the issue.

Signed-off-by: Albert Aribaud <albert.aribaud@free.fr>
---
 lib/display_options.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/lib/display_options.c b/lib/display_options.c
index 20319e6..c544777 100644
--- a/lib/display_options.c
+++ b/lib/display_options.c
@@ -101,7 +101,8 @@ void print_size(unsigned long long size, const char *s)
 #define DEFAULT_LINE_LENGTH_BYTES (16)
 int print_buffer (ulong addr, void* data, uint width, uint count, uint linelen)
 {
-	uint8_t linebuf[MAX_LINE_LENGTH_BYTES + 1];
+	uint8_t linebuf[MAX_LINE_LENGTH_BYTES + 1]
+		__attribute__((__aligned__(sizeof(uint32_t))));
 	uint32_t *uip = (void*)linebuf;
 	uint16_t *usp = (void*)linebuf;
 	uint8_t *ucp = (void*)linebuf;
-- 
1.7.1

[U-Boot] [PATCH] Align linebuf to avoid misaligned aliases of it

[Mike Frysinger]

On Sat, Aug 14, 2010 at 4:11 AM, Albert Aribaud wrote:
> Commit ?64419e47518bbba059c80b77558f93ad4804145c aliases
> the uint16_t usp and uint32_t uip variables in print_buffer()
> to uint8_t variable linebuf without aligning it to an uint32_t
> address, thus causing data aborts on ARM when doing md.l on
> 32-bit wide area (and probably 16-bit wide as well).

hmm, i guess i overlooked that issue.  usually i'm pretty good at this
stuff.  sorry about that.

> Aligning linebuf fixes the issue.

i dont believe there are issues with gcc stack aligning for native
sizes ... just above that

> ?int print_buffer (ulong addr, void* data, uint width, uint count, uint linelen)
> ?{
> - ? ? ? uint8_t linebuf[MAX_LINE_LENGTH_BYTES + 1];
> + ? ? ? uint8_t linebuf[MAX_LINE_LENGTH_BYTES + 1]
> + ? ? ? ? ? ? ? __attribute__((__aligned__(sizeof(uint32_t))));

__aligned(sizeof(uint32_t))
-mike

[U-Boot] [PATCH] Align linebuf to avoid misaligned aliases of it

[Albert ARIBAUD]

Le 14/08/2010 10:25, Mike Frysinger a ?crit :

>>   int print_buffer (ulong addr, void* data, uint width, uint count, uint linelen)
>>   {
>> -       uint8_t linebuf[MAX_LINE_LENGTH_BYTES + 1];
>> +       uint8_t linebuf[MAX_LINE_LENGTH_BYTES + 1]
>> +               __attribute__((__aligned__(sizeof(uint32_t))));
>
> __aligned(sizeof(uint32_t))
> -mike

I based the __aligned__ on what's in post/cpu/ppc4xx/cache.c. What is 
the difference between __aligned and __aligned__?

Amicalement,
-- 
Albert.

[U-Boot] [PATCH] Align linebuf to avoid misaligned aliases of it

[Mike Frysinger]

On Sat, Aug 14, 2010 at 4:33 AM, Albert ARIBAUD wrote:
> Le 14/08/2010 10:25, Mike Frysinger a ?crit :
>>> ?int print_buffer (ulong addr, void* data, uint width, uint count, uint
>>> linelen)
>>> ?{
>>> - ? ? ? uint8_t linebuf[MAX_LINE_LENGTH_BYTES + 1];
>>> + ? ? ? uint8_t linebuf[MAX_LINE_LENGTH_BYTES + 1]
>>> + ? ? ? ? ? ? ? __attribute__((__aligned__(sizeof(uint32_t))));
>>
>> __aligned(sizeof(uint32_t))
>
> I based the __aligned__ on what's in post/cpu/ppc4xx/cache.c. What is the
> difference between __aligned and __aligned__?

i dont mean "__attribute__((__aligned(sizeof(uint32_t))))", i mean
"__aligned(sizeof(uint32_t))".  the linux compiler headers take care
of expanding it to an attribute.
-mike

[U-Boot] [PATCH] Align linebuf to avoid misaligned aliases of it

[Albert ARIBAUD]

Le 14/08/2010 19:42, Mike Frysinger a ?crit :
> On Sat, Aug 14, 2010 at 4:33 AM, Albert ARIBAUD wrote:
>> Le 14/08/2010 10:25, Mike Frysinger a ?crit :
>>>>   int print_buffer (ulong addr, void* data, uint width, uint count, uint
>>>> linelen)
>>>>   {
>>>> -       uint8_t linebuf[MAX_LINE_LENGTH_BYTES + 1];
>>>> +       uint8_t linebuf[MAX_LINE_LENGTH_BYTES + 1]
>>>> +               __attribute__((__aligned__(sizeof(uint32_t))));
>>>
>>> __aligned(sizeof(uint32_t))
>>
>> I based the __aligned__ on what's in post/cpu/ppc4xx/cache.c. What is the
>> difference between __aligned and __aligned__?
>
> i dont mean "__attribute__((__aligned(sizeof(uint32_t))))", i mean
> "__aligned(sizeof(uint32_t))".  the linux compiler headers take care
> of expanding it to an attribute.
> -mike

The __aligned(...) syntax fails to compile with the ELDK 4.2 arm toolchain.

Amicalement,
-- 
Albert.

[U-Boot] [PATCH] Align linebuf to avoid misaligned aliases of it

[Mike Frysinger]

On Wed, Aug 18, 2010 at 8:49 AM, Albert ARIBAUD wrote:
> Le 14/08/2010 19:42, Mike Frysinger a ?crit :
>> On Sat, Aug 14, 2010 at 4:33 AM, Albert ARIBAUD wrote:
>>> Le 14/08/2010 10:25, Mike Frysinger a ?crit :
>>>>> ?int print_buffer (ulong addr, void* data, uint width, uint count, uint
>>>>> linelen)
>>>>> ?{
>>>>> - ? ? ? uint8_t linebuf[MAX_LINE_LENGTH_BYTES + 1];
>>>>> + ? ? ? uint8_t linebuf[MAX_LINE_LENGTH_BYTES + 1]
>>>>> + ? ? ? ? ? ? ? __attribute__((__aligned__(sizeof(uint32_t))));
>>>>
>>>> __aligned(sizeof(uint32_t))
>>>
>>> I based the __aligned__ on what's in post/cpu/ppc4xx/cache.c. What is the
>>> difference between __aligned and __aligned__?
>>
>> i dont mean "__attribute__((__aligned(sizeof(uint32_t))))", i mean
>> "__aligned(sizeof(uint32_t))". ?the linux compiler headers take care
>> of expanding it to an attribute.
>
> The __aligned(...) syntax fails to compile with the ELDK 4.2 arm toolchain.

you need to include linux/compiler.h first ... but i would have
thought this be a header already included globally.  maybe that's a
new topic to start.
-mike

[U-Boot] [PATCH] Align linebuf to avoid misaligned aliases of it

[Albert ARIBAUD]

Le 18/08/2010 18:46, Mike Frysinger a ?crit :
> On Wed, Aug 18, 2010 at 8:49 AM, Albert ARIBAUD wrote:
>> Le 14/08/2010 19:42, Mike Frysinger a ?crit :
>>> On Sat, Aug 14, 2010 at 4:33 AM, Albert ARIBAUD wrote:
>>>> Le 14/08/2010 10:25, Mike Frysinger a ?crit :
>>>>>>   int print_buffer (ulong addr, void* data, uint width, uint count, uint
>>>>>> linelen)
>>>>>>   {
>>>>>> -       uint8_t linebuf[MAX_LINE_LENGTH_BYTES + 1];
>>>>>> +       uint8_t linebuf[MAX_LINE_LENGTH_BYTES + 1]
>>>>>> +               __attribute__((__aligned__(sizeof(uint32_t))));
>>>>>
>>>>> __aligned(sizeof(uint32_t))
>>>>
>>>> I based the __aligned__ on what's in post/cpu/ppc4xx/cache.c. What is the
>>>> difference between __aligned and __aligned__?
>>>
>>> i dont mean "__attribute__((__aligned(sizeof(uint32_t))))", i mean
>>> "__aligned(sizeof(uint32_t))".  the linux compiler headers take care
>>> of expanding it to an attribute.
>>
>> The __aligned(...) syntax fails to compile with the ELDK 4.2 arm toolchain.
>
> you need to include linux/compiler.h first ... but i would have
> thought this be a header already included globally.  maybe that's a
> new topic to start.
> -mike

I don't understand why I should introduce a dependency on linux. What is 
the benefit?

Amicalement,
-- 
Albert.

[U-Boot] [PATCH] Align linebuf to avoid misaligned aliases of it

[Mike Frysinger]

On Wed, Aug 18, 2010 at 1:46 PM, Albert ARIBAUD wrote:
> Le 18/08/2010 18:46, Mike Frysinger a ?crit :
>> you need to include linux/compiler.h first ... but i would have
>> thought this be a header already included globally. ?maybe that's a
>> new topic to start.
>
> I don't understand why I should introduce a dependency on linux. What is the
> benefit?

u-boot already includes the header in the source tree, not to mention
the fact that this file is already including linux/ctype.h
-mike

[U-Boot] [PATCH] Align linebuf to avoid misaligned aliases of it

[Albert ARIBAUD]

Le 18/08/2010 19:54, Mike Frysinger a ?crit :
> On Wed, Aug 18, 2010 at 1:46 PM, Albert ARIBAUD wrote:
>> Le 18/08/2010 18:46, Mike Frysinger a ?crit :
>>> you need to include linux/compiler.h first ... but i would have
>>> thought this be a header already included globally.  maybe that's a
>>> new topic to start.
>>
>> I don't understand why I should introduce a dependency on linux. What is the
>> benefit?
>
> u-boot already includes the header in the source tree, not to mention
> the fact that this file is already including linux/ctype.h
> -mike

But absolutely no .h or .c file uses __aligned whereas 
__attribute__((__aligned__())) is used in several places, right?

Amicalement,
-- 
Albert.

[U-Boot] [PATCH] Align linebuf to avoid misaligned aliases of it

[Mike Frysinger]

On Wednesday, August 18, 2010 16:36:39 Albert ARIBAUD wrote:
> Le 18/08/2010 19:54, Mike Frysinger a ?crit :
> > On Wed, Aug 18, 2010 at 1:46 PM, Albert ARIBAUD wrote:
> >> Le 18/08/2010 18:46, Mike Frysinger a ?crit :
> >>> you need to include linux/compiler.h first ... but i would have
> >>> thought this be a header already included globally.  maybe that's a
> >>> new topic to start.
> >> 
> >> I don't understand why I should introduce a dependency on linux. What is
> >> the benefit?
> > 
> > u-boot already includes the header in the source tree, not to mention
> > the fact that this file is already including linux/ctype.h
> 
> But absolutely no .h or .c file uses __aligned whereas
> __attribute__((__aligned__())) is used in several places, right?

that would seem to be the case currently
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20100819/dd69ca2f/attachment.pgp 

[U-Boot] [PATCH] Align linebuf to avoid misaligned aliases of it

[Albert ARIBAUD]

Le 19/08/2010 07:58, Mike Frysinger a ?crit :
> On Wednesday, August 18, 2010 16:36:39 Albert ARIBAUD wrote:
>> Le 18/08/2010 19:54, Mike Frysinger a ?crit :
>>> On Wed, Aug 18, 2010 at 1:46 PM, Albert ARIBAUD wrote:
>>>> Le 18/08/2010 18:46, Mike Frysinger a ?crit :
>>>>> you need to include linux/compiler.h first ... but i would have
>>>>> thought this be a header already included globally.  maybe that's a
>>>>> new topic to start.
>>>>
>>>> I don't understand why I should introduce a dependency on linux. What is
>>>> the benefit?
>>>
>>> u-boot already includes the header in the source tree, not to mention
>>> the fact that this file is already including linux/ctype.h
>>
>> But absolutely no .h or .c file uses __aligned whereas
>> __attribute__((__aligned__())) is used in several places, right?
>
> that would seem to be the case currently
> -mike

In that case, I'll stick with the currently used syntax for this patch.

If someone feels the need to switch over to __aligned, I think issuing a 
separate global patch which changes all occurrences makes more sense 
than using both forms in the source code.

Amicalement,
-- 
Albert.