usb: do explicit unaligned accesses

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.

Signed-off-by Lucas Stach <[email protected]>

---
 common/usb_hub.c            | 14 +++++++++-----
 drivers/usb/host/ehci-hcd.c |  7 +++++--

2 Dateien ge?ndert, 14 Zeilen hinzugef?gt(+), 7 Zeilen entfernt(-)

common/usb_hub.c +9 -5

index 53d939c..b8cd990 100644
@@ -43,6 +43,7 @@
 #include <common.h>
 #include <command.h>
 #include <asm/processor.h>
+#include <asm/unaligned.h>
 #include <linux/ctype.h>
 #include <asm/byteorder.h>
 #include <asm/unaligned.h>

@@ -269,6 +270,7 @@ static int usb_hub_configure(struct usb_device *dev)
 	int i;
 	ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buffer, USB_BUFSIZ);
 	unsigned char *bitmap;
+	short hubCharacteristics;
 	struct usb_hub_descriptor *descriptor;
 	struct usb_hub_device *hub;
 #ifdef USB_HUB_DEBUG

@@ -304,8 +306,9 @@ static int usb_hub_configure(struct usb_device *dev)
 	}
 	memcpy((unsigned char *)&hub->desc, buffer, descriptor->bLength);
 	/* adjust 16bit values */
-	hub->desc.wHubCharacteristics =
-				le16_to_cpu(descriptor->wHubCharacteristics);
+	put_unaligned(le16_to_cpu(get_unaligned(
+			&descriptor->wHubCharacteristics)),
+			&descriptor->wHubCharacteristics);
 	/* set the bitmap */
 	bitmap = (unsigned char *)&hub->desc.DeviceRemovable[0];
 	/* devices not removable by default */

@@ -322,7 +325,8 @@ static int usb_hub_configure(struct usb_device *dev)
 	dev->maxchild = descriptor->bNbrPorts;
 	USB_HUB_PRINTF("%d ports detected\n", dev->maxchild);
 
-	switch (hub->desc.wHubCharacteristics & HUB_CHAR_LPSM) {
+	hubCharacteristics = get_unaligned(&hub->desc.wHubCharacteristics);
+	switch (hubCharacteristics & HUB_CHAR_LPSM) {
 	case 0x00:
 		USB_HUB_PRINTF("ganged power switching\n");
 		break;

@@ -335,12 +339,12 @@ static int usb_hub_configure(struct usb_device *dev)
 		break;
 	}
 
-	if (hub->desc.wHubCharacteristics & HUB_CHAR_COMPOUND)
+	if (hubCharacteristics & HUB_CHAR_COMPOUND)
 		USB_HUB_PRINTF("part of a compound device\n");
 	else
 		USB_HUB_PRINTF("standalone hub\n");
 
-	switch (hub->desc.wHubCharacteristics & HUB_CHAR_OCPM) {
+	switch (hubCharacteristics & HUB_CHAR_OCPM) {
 	case 0x00:
 		USB_HUB_PRINTF("global over-current protection\n");
 		break;

drivers/usb/host/ehci-hcd.c +1 -1

index bfea192..d90e94d 100644
@@ -22,6 +22,7 @@
  */
 #include <common.h>
 #include <asm/byteorder.h>
+#include <asm/unaligned.h>
 #include <usb.h>
 #include <asm/io.h>
 #include <malloc.h>

@@ -876,10 +877,12 @@ int usb_lowlevel_init(int index, void **controller)
 	debug("Register %x NbrPorts %d\n", reg, descriptor.hub.bNbrPorts);
 	/* Port Indicators */
 	if (HCS_INDICATOR(reg))
-		descriptor.hub.wHubCharacteristics |= 0x80;

Dear Lucas Stach,

Quoted 6 lines

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?

Hi Marek,

Quoted 6 lines · Marek Vasut

Dear Lucas Stach,

Quoted 4 lines

usb_hub_descriptor has to be packed as it's used for communication with the device. Member wHubCharacteristics violates the natural alignment rules.

Quoted 4 lines

Quoted 2 lines

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:

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.

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.

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.

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:

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.

Hope this clarifies.

Amicalement,

Hi Marek,

Quoted 26 lines · Marek Vasut

Dear Albert ARIBAUD,

Quoted 11 lines

Hi Marek,

On Fri, 31 Aug 2012 01:29:05 +0200, Marek Vasut <[email protected]>

Quoted 6 lines · wrote:

Dear Lucas Stach,

Quoted 4 lines

usb_hub_descriptor has to be packed as it's used for communication with the device. Member wHubCharacteristics violates the natural alignment rules.

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

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.

Quoted 4 lines

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.

Quoted 3 lines

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.

Quoted 17 lines

Quoted 5 lines

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.

Quoted 5 lines

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.

Quoted 4 lines

Quoted 4 lines

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

:)

Quoted 15 lines

Quoted 14 lines

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.

Quoted 4 lines

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.

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

If I am not correct, can
you please detail what you meant?

Quoted 3 lines

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.

Quoted 32 lines

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

Quoted 6 lines

Quoted 3 lines

Hope this clarifies.

Amicalement,

Best regards,
Marek Vasut

Amicalement,

Dear Albert ARIBAUD,

Quoted 2 lines

I think you are talking about lumping small-sized accesses together into a bigger access possibly aligned.

This is exactly what I mean.

Quoted 2 lines

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.

Quoted 7 lines

If I am not correct, can
you please detail what you meant?

Quoted 3 lines

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.

Quoted 3 lines

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.

Best regards,
Marek Vasut

Hi Marek,

Quoted 5 lines · Marek Vasut

Dear Albert ARIBAUD,

Quoted 2 lines

I think you are talking about lumping small-sized accesses together into a bigger access possibly aligned.

Quoted 7 lines

This is exactly what I mean.

Quoted 2 lines

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.

I have not expressed myself clearly. Yes, access lumping is related to access alignment. What I meant is: disallowing misaligned native accesses will not prevent access lumping. Misalignment restrictions do indeed restrict how such lumpings will happen, but it does not prevent lumping per se.

One place where lumping and misalignement prevention did clash was raised in the previous discussion: a 7+1 bytes function-local char array was allocated on a non-aligned address (which is possible and normal because it is a char) and was initialized with some content. The compiler lumped the initialization as two misaligned 32-byte native accesses, despite misaligned native accesses being forbidden by compiler command line options. This was a compiler bug.

Quoted 7 lines

Quoted 7 lines

If I am not correct, can
you please detail what you meant?

Quoted 3 lines

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.

Quoted 7 lines

Quoted 3 lines

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.

... or fix his bloody structure, or fix his bloody f...ixing pointer arithmetic, or... but I do agree with the trapping, and that's my plan.

However other architectures may need, or choose, another stance on alignments, and it is best if they don't have to discover intended misaligned accesses the hard way.

Thus my opinion that any misaligned
access which cannot be fixed should not be sliently left for the compiler to handle, but should (also) be clearly marked as such, if only for humans to notice.

Quoted 2 lines

Best regards,
Marek Vasut

Amicalement,

Hi Marek,

Quoted 34 lines · Marek Vasut

Dear Albert ARIBAUD,

Quoted 24 lines

Hi Marek,

On Sat, 1 Sep 2012 00:16:43 +0200, Marek Vasut <[email protected]>

Quoted 5 lines · wrote:

Dear Albert ARIBAUD,

Quoted 2 lines

I think you are talking about lumping small-sized accesses together into a bigger access possibly aligned.

Quoted 7 lines

This is exactly what I mean.

Quoted 2 lines

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.

I have not expressed myself clearly. Yes, access lumping is related to access alignment. What I meant is: disallowing misaligned native accesses will not prevent access lumping. Misalignment restrictions do indeed restrict how such lumpings will happen, but it does not prevent lumping per se.

Quoted 7 lines

One place where lumping and misalignement prevention did clash was raised in the previous discussion: a 7+1 bytes function-local char array was allocated on a non-aligned address (which is possible and normal because it is a char) and was initialized with some content. The compiler lumped the initialization as two misaligned 32-byte native accesses, despite misaligned native accesses being forbidden by compiler command line options. This was a compiler bug.

Quoted 2 lines

But that'd mean that instead of fixing a compiler, we'd be doing a workaround in our code?

Not exactly.

First, in this instance, a fix to the compiler has been at least requested, if not already applied (I would need to check this). The fix causes the compiler to still generate misaligned 32-bit accesses *if* misaligned native accesses are allowed, and to use only allowed accesses otherwise.

Second, I do not ask U-Boot contributors to mark code as explicitly unaligned when the misaligned access is caused by a compiler or code error; I ask them to mark code as unaligned when the misaligned access is *unavoidable* because the HW or some standard imposes it.

Here, the specification from which the USB struc is derived imposes a misaligned field. This, rather than any compiler bug, makes the misaligned access *unavoidable*. And because it is, I ask that it be marked so, by the explicit use of unaligned accessors.

Quoted 24 lines

Quoted 23 lines

Quoted 8 lines

Quoted 8 lines

If I am not correct, can
you please detail what you meant?

Quoted 4 lines

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.

Quoted 7 lines

Quoted 3 lines

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.

... or fix his bloody structure, or fix his bloody f...ixing pointer arithmetic, or... but I do agree with the trapping, and that's my plan.

However other architectures may need, or choose, another stance on alignments, and it is best if they don't have to discover intended misaligned accesses the hard way.

Quoted 2 lines

Yet still, in such case, valid compiler has to generate valid workaround code.

Yes. However, letting the compiler generate workarounds may end up letting it generate workarounds for misaligned accesses caused by errors or bugs also. Marking the code explicitly helps telling which is which too.

Quoted 5 lines

Quoted 4 lines

Thus my opinion that any misaligned
access which cannot be fixed should not be sliently left for the compiler to handle, but should (also) be clearly marked as such, if only for humans to notice.

Quoted 3 lines

I can't say I agree here ... since it's a really ad-hoc solution. I can't say I see any real benefit other than that it's hiding possible compiler bugs :-(

Here it is barely an ad hoc solution, as the alternative would be fixing the hardware or worse, spec (can someone tell us where this misaligned struct field originates from exactly, hw or USB spec?)

Quoted 2 lines

Best regards,
Marek Vasut

Amicalement,

Quoted 14 lines

Quoted 11 lines

Quoted 8 lines

One place where lumping and misalignement prevention did clash was raised in the previous discussion: a 7+1 bytes function-local char array was allocated on a non-aligned address (which is possible and normal because it is a char) and was initialized with some content. The compiler lumped the initialization as two misaligned 32-byte native accesses, despite misaligned native accesses being forbidden by compiler command line options. This was a compiler bug.

But that'd mean that instead of fixing a compiler, we'd be doing a workaround in our code?

Not exactly.

Quoted 5 lines

First, in this instance, a fix to the compiler has been at least requested, if not already applied (I would need to check this). The fix causes the compiler to still generate misaligned 32-bit accesses *if* misaligned native accesses are allowed, and to use only allowed accesses otherwise.

But then again, this is compiler bug we exposed, no need to hide it. I'm firm on this one.

Quoted 4 lines

Second, I do not ask U-Boot contributors to mark code as explicitly unaligned when the misaligned access is caused by a compiler or code error; I ask them to mark code as unaligned when the misaligned access is *unavoidable* because the HW or some standard imposes it.

I see, I'm starting to see your point. Maybe because I've missed the previous discussion.

Quoted 4 lines

Here, the specification from which the USB struc is derived imposes a misaligned field. This, rather than any compiler bug, makes the misaligned access *unavoidable*. And because it is, I ask that it be marked so, by the explicit use of unaligned accessors.

Still, this is unaligned only on ARM, not on maybe some other arches, right?

Quoted 4 lines

Yes. However, letting the compiler generate workarounds may end up letting it generate workarounds for misaligned accesses caused by errors or bugs also. Marking the code explicitly helps telling which is which too.

Does this work across architectures too? Like, on arm it's misaligned, on intel it isnt.

Quoted 3 lines

Here it is barely an ad hoc solution, as the alternative would be fixing the hardware or worse, spec (can someone tell us where this misaligned struct field originates from exactly, hw or USB spec?)

http://www.intel.com/technology/usb/download/ehci-r10.pdf I think you're looking around 3.6 .

Quoted 2 lines

Quoted 2 lines

Best regards,
Marek Vasut

Each architecture has its own capabilites regarding native misaligned accesses... This is why I consider that as a general rule U-Boot should always align its data properly, because (hopefully) all architectures can do aligned native accesses; OTOH, if we accept misaligned code on the grounds that 'it works on such and suh arches' or that 'any normal arch should be able to handle misaligned accesses some way' or 'no one in their right mind would physically forbit misaligned accesses', then we're just giving Murphy a chance to kick us at some point.

Consider this an application of Postel's principle: we liberally accept architectures that maybe allow misaligned accesses and maybe handle them well; and we conservatively do not do such accesses unless we have no other choice.

Hi Marek,

Quoted 14 lines · Marek Vasut

Quoted 14 lines

Quoted 11 lines

Quoted 8 lines

One place where lumping and misalignement prevention did clash was raised in the previous discussion: a 7+1 bytes function-local char array was allocated on a non-aligned address (which is possible and normal because it is a char) and was initialized with some content. The compiler lumped the initialization as two misaligned 32-byte native accesses, despite misaligned native accesses being forbidden by compiler command line options. This was a compiler bug.

But that'd mean that instead of fixing a compiler, we'd be doing a workaround in our code?

Not exactly.

Quoted 8 lines

Quoted 5 lines

First, in this instance, a fix to the compiler has been at least requested, if not already applied (I would need to check this). The fix causes the compiler to still generate misaligned 32-bit accesses *if* misaligned native accesses are allowed, and to use only allowed accesses otherwise.

But then again, this is compiler bug we exposed, no need to hide it. I'm firm on this one.

I guess I was not clear: this issue with an 8-char local array was *not* in U-Boot. So we exposed nothing there, and none of the discussion led to any conclusion that we should hide anything under the carpet. Actually, if/when we meet a compiler issue, my suggestion is always to explicitly and lavishly comment the 'fix', whatever it is, with warnings such as /* CAUTION! BRAINDEAD COMPILER! There is an issue with compiler X versions Y and up where [...] */. And keep an eye on the compiler.

Quoted 7 lines

Quoted 4 lines

Second, I do not ask U-Boot contributors to mark code as explicitly unaligned when the misaligned access is caused by a compiler or code error; I ask them to mark code as unaligned when the misaligned access is *unavoidable* because the HW or some standard imposes it.

I see, I'm starting to see your point. Maybe because I've missed the previous discussion.

I think so.

Quoted 7 lines

Quoted 4 lines

Here, the specification from which the USB struc is derived imposes a misaligned field. This, rather than any compiler bug, makes the misaligned access *unavoidable*. And because it is, I ask that it be marked so, by the explicit use of unaligned accessors.

Still, this is unaligned only on ARM, not on maybe some other arches, right?

The notion of 'misaligned' (rather than unaligned) is pretty much
architecture-independent: the fact that a 32-bit int is not on a 4-byte boundary makes it misaligned. Now, depending on arches, this misalignment may be irrelevant because the arch can do native misaligned accesses with little or no penalty, or may be a worry because the arch can (be made to) do native misaligned accesses but at a performance cost, or it may be a blocker because the arch cannot do native misaligned accesses.

So maybe on some other arches misalignment is 'not a problem', or maybe it is 'a serious issue'. Anyway, for ARM is ranges from one ed to the other, and for any arch, on a given system the seriousness of the issue may be set by the designers of the system.

Quoted 7 lines

Quoted 4 lines

Yes. However, letting the compiler generate workarounds may end up letting it generate workarounds for misaligned accesses caused by errors or bugs also. Marking the code explicitly helps telling which is which too.

Does this work across architectures too? Like, on arm it's misaligned, on intel it isnt.

Each architecture has its own capabilites regarding native misaligned accesses... This is why I consider that as a general rule U-Boot should always align its data properly, because (hopefully) all architectures can do aligned native accesses; OTOH, if we accept misaligned code on the grounds that 'it works on such and suh arches' or that 'any normal arch should be able to handle misaligned accesses some way' or 'no one in their right mind would physically forbit misaligned accesses', then we're just giving Murphy a chance to kick us at some point.

Consider this an application of Postel's principle: we liberally accept architectures that maybe allow misaligned accesses and maybe handle them well; and we conservatively do not do such accesses unless we have no other choice.

Quoted 4 lines

Quoted 3 lines

Here it is barely an ad hoc solution, as the alternative would be fixing the hardware or worse, spec (can someone tell us where this misaligned struct field originates from exactly, hw or USB spec?)

Quoted 2 lines

http://www.intel.com/technology/usb/download/ehci-r10.pdf I think you're looking around 3.6 .

I see section 3.6 (Queue Head) but I don't readily see the link between the header and the patch we're discussiong (but I'm not an USB expert either). Can you connect e few more dots for me? Thanks in advance.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Best regards,
Marek Vasut

Amicalement,

Hello Lucas,

common/usb_hub.c static int usb_hub_configure(struct usb_device *dev) · line 311

Quoted 9 lines

common/usb_hub.c +3 -2

@@ -304,8 +306,9 @@ static int usb_hub_configure(struct usb_device *dev)
 	}
 	memcpy((unsigned char *)&hub->desc, buffer, descriptor->bLength);
 	/* adjust 16bit values */
-	hub->desc.wHubCharacteristics =
-				le16_to_cpu(descriptor->wHubCharacteristics);
+	put_unaligned(le16_to_cpu(get_unaligned(
+			&descriptor->wHubCharacteristics)),
+			&descriptor->wHubCharacteristics);

I think, the second parameter should be &hub->... ?

Best Regards,
Thomas