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

[Erwin Authried]

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));

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
> >