[PATCH v2] libertas: Fix alignment issues in libertas core

[Colin McCabe]

On Wed, Jan 28, 2009 at 3:47 PM, Erwin Authried <eauth at softsys.co.at> wrote:
> Am Mittwoch, den 28.01.2009, 14:48 -0800 schrieb Colin McCabe:
>> The argument for specifying struct layout explicitly is pretty clear.
>> If we relied on gcc to get it right "by accident," we would find that
>> gcc upgrades and minor code changes would break the driver in
>> mysterious ways.
> you are probably right, it's a bit fragile, and there's no warranty
> that different/new architectures have the same rules for alignment of
> structures.
>
>>
>> I'm surprised that gcc generates poorer code when the layout is
>> specified explicitly, since after all, it should be the same layout.
>> Have you tried using __attribute__ ((aligned (8))) or similar?
>>
> it's not surprising because with the packed attribute gcc doesn't make
> any assumptions about the the alignment of the start of the structure.
> For example, if you compare the code for the following simple structure:
>
> struct a {
>  int i;
> };
>
> struct b {
>  int i;
> } __attribute__ ((packed));

That's very insightful.

I wonder if there is an __attribute__ or pragma that would force gcc
to do the right thing here.
Can you put __attribute__((aligned)) on a member of a packed struct? I
might have to test that out later...

Colin


>
> the following code is generated when "i" is accessed via a pointer to
> the structure:
>
> * without packed attribute:
>        ldr     r0, [r0, #0]
>
> * with the packed attribute:
>        ldrb    r3, [r0, #0]
>        ldrb    r2, [r0, #1]
>        ldrb    r1, [r0, #2]
>        orr     r3, r3, r2, asl #8
>        ldrb    r0, [r0, #3]
>        orr     r3, r3, r1, asl #16
>        orr     r0, r3, r0, asl #24
>
> As you see, there are seven(!) instructions instead of just one for
> reading a 32-bit variable when the packed attribute is used!
>
> -Erwin
>
>> regards,
>> Colin
>>
>>
>> On Wed, Jan 28, 2009 at 10:47 AM, Erwin Authried <eauth at softsys.co.at> wrote:
>> > Am Mittwoch, den 28.01.2009, 07:49 -0800 schrieb Andrey Yurovsky:
>> >> On Wed, Jan 28, 2009 at 12:34 AM, Erwin Authried <eauth at softsys.co.at> wrote:
>> >> > is it really necessary to pack all structures in hostcmd.h? The "packed"
>> >> > attribute makes the code larger and slower. I have changed the order in
>> >> > the lbs_private structure in dev.h a little bit so that resp_buf is on a
>> >> > 32-bit boundary. Then, I have removed most of the packed attributes in
>> >> > hostcmd.h (where all 16 or 32-bit Variables are on 16/32 bit boundaries
>> >> > anyway). The libertas module is a bit smaller, and I haven't seen any
>> >> > problem.
>> >>
>> >> Thanks Erwin.  What CPU or CPUs have you tested your changes on?  In
>> >> our case, the Blackfin (and, reportedly, the AVR32) needed us to
>> >> guarantee alignment whereas ARM and x86 worked fine as things were
>> >> before.  Also although it's less efficient, we can now add commands
>> >> more safely if we use the packed attribute consistently rather than
>> >> counting bytes, and there's a good chance that more host command
>> >> structures will be added in the near future.
>> >
>> > I have tested with ARM7TDMI (AT91FR40162), there is no aligment trap.
>> > The cpu simply ignores the lowest address bits and produces wrong
>> > assignments when the alignment isn't correct.
>> >
>> > -Erwin
>> >
>> >>
>> >> > Am Montag, den 12.01.2009, 13:14 -0800 schrieb Andrey Yurovsky:
>> >> >> Data structures that come over the wire from the WLAN firmware must be packed.
>> >> >> This fixes alignment problems on the blackfin architecture and, reportedly, on
>> >> >> the AVR32.
>> >> >>
>> >> >> This is a replacement for the previous version of this patch which had also
>> >> >> explicitly used get_unaligned_ macros.  As Johannes Berg pointed out, these
>> >> >> macros were unnecessary.
>> >> >>
>> > --
>> > Dipl.-Ing. Erwin Authried
>> > Softwareentwicklung und Systemdesign
>> >
>> >
>> > _______________________________________________
>> > libertas-dev mailing list
>> > libertas-dev at lists.infradead.org
>> > http://lists.infradead.org/mailman/listinfo/libertas-dev
>> >
>
>