[U-Boot] [PATCH] usb: do explicit unaligned accesses

[Marek Vasut <marex@denx.de>]

Dear Albert ARIBAUD,

> Hi Marek,
> 
> On Fri, 31 Aug 2012 18:15:30 +0200, Marek Vasut <marex@denx.de> wrote:
> > Dear Albert ARIBAUD,
> > 
> > > Hi Marek,
> > > 
> > > On Fri, 31 Aug 2012 01:29:05 +0200, Marek Vasut <marex@denx.de>
> > > 
> > > wrote:
> > > > Dear Lucas Stach,
> > > > 
> > > > > usb_hub_descriptor has to be packed as it's used for
> > > > > communication with the device. Member wHubCharacteristics
> > > > > violates the natural alignment rules.
> > > > > 
> > > > > Use explicit unaligned access functions for this member.
> > > > > Fixes ARMv7 traping while using USB.
> > > > 
> > > > Shouldn't a properly configured compiler prevent such behavior?
> > > 
> > > This has been discussed before. As a summary:
> > Argh, I must have missed this.
> > 
> > > 1. A properly configured compiler can pad a structure so that the
> > > fields always start at an aligned address (assuming the structure
> > > base address is itself aligned). But that alters the structure, and
> > > here there must be no alterations to the structure.
> > 
> > There can be ... the compiler can even reorder the structures (that
> > happened in Linux kernel, as I'm on a train now with shitty internet
> > connection,  I can't find it ... search LWN for that please).
> 
> As a compiler bug, this can happen indeed. And we had our share of
> compiler alignment bugs ourselves. But a conforming compiler should not
> reorder fields.

I'm no compiler expert ... so I won't argue here

> > > 2. A properly (and differently) configured compiler can
> > > automatically generate native unaligned accesses to unaligned
> > > fields. This is acceptable on armv6+ architectures, has a
> > > performance penalty on earlier architectures, and does not
> > > necessarily work depending on the hardware configuration.
> > 
> > Certainly, agreed.
> > 
> > > 3. A properly (and differently yet) configured compiler can
> > > automatically generate non-native unaligned accesses to unaligned
> > > fields. This is acceptable on all architectures, has a performance
> > > penalty on pre-armV6 architectures for all misaligned accesses,
> > > whether voluntary or not.
> > 
> > Correct, I'd vote for this solution -- let the compiler handle such
> > unaligned cases.
> > 
> > > The conclusion of the discussion is as follows -- or to be more
> > > exact, following this discussion, this is my stance as the U-Boot
> 
> > > custodian:
> Someone please hold Wolfgang back while I correct myself: "as the *ARM*
> U-Boot custodian..."
> 
> :)

%^)

> > > i) All the code intended to run 'close to' U-Boot (i.e., U-Boot code
> > > itself and application code) is controlled enough that we should be
> > > able to know and limit which code requires misaligned access (such
> > > as here for this USB structure field);
> > > 
> > > ii) On some ARM architectures, and possibly some non-ARM
> > > architectures as well, native misaligned access incur a performance
> > > hit, and may even simply be impossible or forbidden by a hardware
> > > or system design decision.
> > > 
> > > iii) Thus, U-Boot should follow a strict policy of using native
> > > aligned accesses only, possibly enforcing misaligned native access
> > > prevention in hardware, and of explicitly emulating misaligned
> > > accesses when they cannot be avoided.
> > 
> > I do agree with i) and ii), but why not just let compiler handle the
> > unaligned access for us? The compiler can optimize across the whole
> > code, not only locally over one access, therefore it might be able to
> > punt some of the unaligned accesses altogether if the code allows it.
> 
> I think you are talking about lumping small-sized accesses together
> into a bigger access possibly aligned.

This is exactly what I mean.

> If I am correct, then I don't
> think this is related to misaligned accesses.

Why won't it be? Such access can in the end turn out to be aligned, therefore 
leveraging all the penalty.

> If I am not correct, can
> you please detail what you meant?
> 
> > Besides, right now, the code is much more readable. So I really don't
> > like adding some strange macros to force crazy aligned access if the
> > compiler can do it for us and can do it better.
> 
> I personally would let the compiler do it too, but I prefer it to be
> clearly indicated to the reader of the code when an access is
> known to be misaligned.

I'd enable enable the Alignment trapping in the CPU and die on an unaligned 
access at runtime -- to indicate the user that he should fix his bloody 
compiler.

> > $ ARCH=arm CROSS_COMPILE=arm-linux-gnueabi- ./MAKEALL m28evk
> > Configuring for m28evk board...
> > 
> >    text    data     bss     dec     hex filename
> >  
> >  415964    7688  288740  712392   adec8 ./u-boot
> >  
> >   11754     788      12   12554    310a ./spl/u-boot-spl
> > 
> > --------------------- SUMMARY ----------------------------
> > Boards compiled: 1
> > ----------------------------------------------------------
> > $ patch -Np1 -i /tmp/\[PATCH\ v2\]\ usb_do\ explicit\ unaligned\
> > accesses.mbox patching file common/usb_hub.c
> > patching file drivers/usb/host/ehci-hcd.c
> > Hunk #2 succeeded at 867 with fuzz 1 (offset -10 lines).
> > $ ARCH=arm CROSS_COMPILE=arm-linux-gnueabi- ./MAKEALL m28evk
> > Configuring for m28evk board...
> > 
> >    text    data     bss     dec     hex filename
> >  
> >  415968    7688  288736  712392   adec8 ./u-boot
> >  
> >   11754     788      12   12554    310a ./spl/u-boot-spl
> > 
> > --------------------- SUMMARY ----------------------------
> > Boards compiled: 1
> > ----------------------------------------------------------
> > 
> > Notice the text section grew a bit too, I dunno why, does anyone care
> > enough to clarify please?
> > 
> > > Hope this clarifies.
> > > 
> > > Amicalement,
> > 
> > Best regards,
> > Marek Vasut
> 
> Amicalement,

Best regards,
Marek Vasut