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