| Message ID | 1346368414-993-1-git-send-email-dev@lynxeye.de |
|---|---|
| State | Superseded |
| Delegated to: | Marek Vasut |
| Headers | show |
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? > Signed-off-by: Lucas Stach <dev@lynxeye.de> > --- > common/usb_hub.c | 14 +++++++++----- > drivers/usb/host/ehci-hcd.c | 7 +++++-- > 2 Dateien geändert, 14 Zeilen hinzugefügt(+), 7 Zeilen entfernt(-) > > diff --git a/common/usb_hub.c b/common/usb_hub.c > index 53d939c..b8cd990 100644 > --- a/common/usb_hub.c > +++ b/common/usb_hub.c > @@ -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; > diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c > index bfea192..d90e94d 100644 > --- a/drivers/usb/host/ehci-hcd.c > +++ b/drivers/usb/host/ehci-hcd.c > @@ -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; > + put_unaligned(get_unaligned(&descriptor.hub.wHubCharacteristics) > + | 0x80, &descriptor.hub.wHubCharacteristics); > /* Port Power Control */ > if (HCS_PPC(reg)) > - descriptor.hub.wHubCharacteristics |= 0x01; > + put_unaligned(get_unaligned(&descriptor.hub.wHubCharacteristics) > + | 0x01, &descriptor.hub.wHubCharacteristics); > > /* Start the host controller. */ > cmd = ehci_readl(&ehcic[index].hcor->or_usbcmd); Best regards, Marek Vasut
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: 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 Thomas, Am Freitag, den 31.08.2012, 08:57 +0000 schrieb Langer Thomas (LQDE RD ST PON SW): > Hello Lucas, > > > @@ -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->... ? > Argh, stupid copypasta. Thanks for finding this. > Best Regards, > Thomas > >
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). > 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: > > 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. 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. $ 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
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
Hi Marek, (Apologies for the private mail) On Sat, 1 Sep 2012 00:16:43 +0200, Marek Vasut <marex@denx.de> wrote: > Dear Albert ARIBAUD, > > 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. 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. > > 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. ... 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. > Best regards, > Marek Vasut Amicalement,
Dear Albert ARIBAUD, > Hi Marek, > > On Sat, 1 Sep 2012 00:16:43 +0200, Marek Vasut <marex@denx.de> wrote: > > Dear Albert ARIBAUD, > > > > > 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. > > 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. But that'd mean that instead of fixing a compiler, we'd be doing a workaround in our code? > > > 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. > > ... 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. Yet still, in such case, valid compiler has to generate valid workaround code. > 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. 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 :-( > > Best regards, > > Marek Vasut > > Amicalement, Best regards, Marek Vasut
Dear Albert ARIBAUD, > Hi Marek, > > On Sat, 1 Sep 2012 16:34:09 +0200, Marek Vasut <marex@denx.de> wrote: > > Dear Albert ARIBAUD, > > > > > Hi Marek, > > > > > > On Sat, 1 Sep 2012 00:16:43 +0200, Marek Vasut <marex@denx.de> > > > > > > wrote: > > > > Dear Albert ARIBAUD, > > > > > > > > > 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. > > > > > > 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. > > > > 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. But then again, this is compiler bug we exposed, no need to hide it. I'm firm on this one. > 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. > 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? > > > > > 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. > > > > > > ... 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. > > > > 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. Does this work across architectures too? Like, on arm it's misaligned, on intel it isnt. > > > 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. > > > > 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?) http://www.intel.com/technology/usb/download/ehci-r10.pdf I think you're looking around 3.6 . > > Best regards, > > Marek Vasut > > Amicalement,
Hi Marek, On Sat, 1 Sep 2012 17:12:29 +0200, Marek Vasut <marex@denx.de> wrote: > > > > 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. > > > > 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. > > 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. > > 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. > > 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. > > 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 . 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. > > > Best regards, > > > Marek Vasut Amicalement,
Dear Albert ARIBAUD, In message <20120901182841.3ae8b35f@lilith> you wrote: > > 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. Full agreement. Best regards, Wolfgang Denk
Dear Albert ARIBAUD, > Hi Marek, > > On Sat, 1 Sep 2012 17:12:29 +0200, Marek Vasut <marex@denx.de> wrote: > > > > > 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. > > > > > > 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. Agreed > > > 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. > > > > 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. > > > > 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. > > > > 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 . > > 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. Nevermind, I took a second look and noticed how the structure looks. Anyway, I see USB is pioneering this new rule, nice :-) I now basically see the issue at hand, sorry for being a blind ass. Remind me next time to look first and talk afterwards. 361 struct usb_hub_descriptor { 362 unsigned char bLength; 363 unsigned char bDescriptorType; 364 unsigned char bNbrPorts; 365 unsigned short wHubCharacteristics; Let's apply this patch once (if?) WD pulls my USB tree. > > > > Best regards, > > > > Marek Vasut > > Amicalement,
diff --git a/common/usb_hub.c b/common/usb_hub.c index 53d939c..b8cd990 100644 --- a/common/usb_hub.c +++ b/common/usb_hub.c @@ -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; diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c index bfea192..d90e94d 100644 --- a/drivers/usb/host/ehci-hcd.c +++ b/drivers/usb/host/ehci-hcd.c @@ -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; + put_unaligned(get_unaligned(&descriptor.hub.wHubCharacteristics) + | 0x80, &descriptor.hub.wHubCharacteristics); /* Port Power Control */ if (HCS_PPC(reg)) - descriptor.hub.wHubCharacteristics |= 0x01; + put_unaligned(get_unaligned(&descriptor.hub.wHubCharacteristics) + | 0x01, &descriptor.hub.wHubCharacteristics); /* Start the host controller. */ cmd = ehci_readl(&ehcic[index].hcor->or_usbcmd);
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 <dev@lynxeye.de> --- common/usb_hub.c | 14 +++++++++----- drivers/usb/host/ehci-hcd.c | 7 +++++-- 2 Dateien geändert, 14 Zeilen hinzugefügt(+), 7 Zeilen entfernt(-)