* [PATCH] usb: fix unaligned access @ 2011-10-22 13:19 Fabian van der Werf 2011-10-22 20:20 ` Sascha Hauer 0 siblings, 1 reply; 10+ messages in thread From: Fabian van der Werf @ 2011-10-22 13:19 UTC (permalink / raw) To: barebox --- drivers/usb/core/usb.c | 12 +++++++----- drivers/usb/host/ehci-hcd.c | 9 +++++++-- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c index 7039a2c..369a393 100644 --- a/drivers/usb/core/usb.c +++ b/drivers/usb/core/usb.c @@ -50,6 +50,7 @@ #include <driver.h> #include <linux/ctype.h> #include <asm/byteorder.h> +#include <asm/unaligned.h> #include <xfuncs.h> #include <init.h> @@ -1071,6 +1072,7 @@ static int usb_hub_configure(struct usb_device *dev) struct usb_hub_status *hubsts; int i; struct usb_hub_device *hub; + unsigned short hub_chars; hub = xzalloc(sizeof (*hub)); dev->hub = hub; @@ -1100,8 +1102,8 @@ 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); + hub_chars = le16_to_cpu(get_unaligned(&descriptor->wHubCharacteristics)); + put_unaligned(hub_chars, &hub->desc.wHubCharacteristics); /* set the bitmap */ bitmap = (unsigned char *)&hub->desc.DeviceRemovable[0]; /* devices not removable by default */ @@ -1118,7 +1120,7 @@ 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) { + switch (hub_chars & HUB_CHAR_LPSM) { case 0x00: USB_HUB_PRINTF("ganged power switching\n"); break; @@ -1131,12 +1133,12 @@ static int usb_hub_configure(struct usb_device *dev) break; } - if (hub->desc.wHubCharacteristics & HUB_CHAR_COMPOUND) + if (hub_chars & 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 (hub_chars & 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 72f1c14..20c518a 100644 --- a/drivers/usb/host/ehci-hcd.c +++ b/drivers/usb/host/ehci-hcd.c @@ -33,6 +33,7 @@ #include <errno.h> #include <usb/ehci.h> #include <asm/mmu.h> +#include <asm/unaligned.h> #include "ehci.h" @@ -795,6 +796,7 @@ static int ehci_init(struct usb_host *host) struct ehci_priv *ehci = to_ehci(host); uint32_t reg; uint32_t cmd; + unsigned short hub_chars; ehci_halt(ehci); @@ -819,12 +821,15 @@ static int ehci_init(struct usb_host *host) reg = ehci_readl(&ehci->hccr->cr_hcsparams); descriptor.hub.bNbrPorts = HCS_N_PORTS(reg); + hub_chars = get_unaligned(&descriptor.hub.wHubCharacteristics); /* Port Indicators */ if (HCS_INDICATOR(reg)) - descriptor.hub.wHubCharacteristics |= 0x80; + hub_chars |= 0x80; /* Port Power Control */ if (HCS_PPC(reg)) - descriptor.hub.wHubCharacteristics |= 0x01; + hub_chars |= 0x01; + + put_unaligned(hub_chars, &descriptor.hub.wHubCharacteristics); /* Start the host controller. */ cmd = ehci_readl(&ehci->hcor->or_usbcmd); -- 1.7.0.4 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] usb: fix unaligned access 2011-10-22 13:19 [PATCH] usb: fix unaligned access Fabian van der Werf @ 2011-10-22 20:20 ` Sascha Hauer 2011-10-23 9:29 ` Fabian van der Werf 0 siblings, 1 reply; 10+ messages in thread From: Sascha Hauer @ 2011-10-22 20:20 UTC (permalink / raw) To: Fabian van der Werf; +Cc: barebox On Sat, Oct 22, 2011 at 03:19:53PM +0200, Fabian van der Werf wrote: > --- > drivers/usb/core/usb.c | 12 +++++++----- > drivers/usb/host/ehci-hcd.c | 9 +++++++-- > 2 files changed, 14 insertions(+), 7 deletions(-) Looks like a valid patch, I wonder that this never was a problem before. ARM should break here aswell I think. What architecture are you using? Sascha > > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c > index 7039a2c..369a393 100644 > --- a/drivers/usb/core/usb.c > +++ b/drivers/usb/core/usb.c > @@ -50,6 +50,7 @@ > #include <driver.h> > #include <linux/ctype.h> > #include <asm/byteorder.h> > +#include <asm/unaligned.h> > #include <xfuncs.h> > #include <init.h> > > @@ -1071,6 +1072,7 @@ static int usb_hub_configure(struct usb_device *dev) > struct usb_hub_status *hubsts; > int i; > struct usb_hub_device *hub; > + unsigned short hub_chars; > > hub = xzalloc(sizeof (*hub)); > dev->hub = hub; > @@ -1100,8 +1102,8 @@ 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); > + hub_chars = le16_to_cpu(get_unaligned(&descriptor->wHubCharacteristics)); > + put_unaligned(hub_chars, &hub->desc.wHubCharacteristics); > /* set the bitmap */ > bitmap = (unsigned char *)&hub->desc.DeviceRemovable[0]; > /* devices not removable by default */ > @@ -1118,7 +1120,7 @@ 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) { > + switch (hub_chars & HUB_CHAR_LPSM) { > case 0x00: > USB_HUB_PRINTF("ganged power switching\n"); > break; > @@ -1131,12 +1133,12 @@ static int usb_hub_configure(struct usb_device *dev) > break; > } > > - if (hub->desc.wHubCharacteristics & HUB_CHAR_COMPOUND) > + if (hub_chars & 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 (hub_chars & 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 72f1c14..20c518a 100644 > --- a/drivers/usb/host/ehci-hcd.c > +++ b/drivers/usb/host/ehci-hcd.c > @@ -33,6 +33,7 @@ > #include <errno.h> > #include <usb/ehci.h> > #include <asm/mmu.h> > +#include <asm/unaligned.h> > > #include "ehci.h" > > @@ -795,6 +796,7 @@ static int ehci_init(struct usb_host *host) > struct ehci_priv *ehci = to_ehci(host); > uint32_t reg; > uint32_t cmd; > + unsigned short hub_chars; > > ehci_halt(ehci); > > @@ -819,12 +821,15 @@ static int ehci_init(struct usb_host *host) > reg = ehci_readl(&ehci->hccr->cr_hcsparams); > descriptor.hub.bNbrPorts = HCS_N_PORTS(reg); > > + hub_chars = get_unaligned(&descriptor.hub.wHubCharacteristics); > /* Port Indicators */ > if (HCS_INDICATOR(reg)) > - descriptor.hub.wHubCharacteristics |= 0x80; > + hub_chars |= 0x80; > /* Port Power Control */ > if (HCS_PPC(reg)) > - descriptor.hub.wHubCharacteristics |= 0x01; > + hub_chars |= 0x01; > + > + put_unaligned(hub_chars, &descriptor.hub.wHubCharacteristics); > > /* Start the host controller. */ > cmd = ehci_readl(&ehci->hcor->or_usbcmd); > -- > 1.7.0.4 > > > _______________________________________________ > barebox mailing list > barebox@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/barebox > -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] usb: fix unaligned access 2011-10-22 20:20 ` Sascha Hauer @ 2011-10-23 9:29 ` Fabian van der Werf 2011-10-24 15:14 ` Antony Pavlov 0 siblings, 1 reply; 10+ messages in thread From: Fabian van der Werf @ 2011-10-23 9:29 UTC (permalink / raw) To: Sascha Hauer; +Cc: barebox > > Looks like a valid patch, I wonder that this never was a problem before. > ARM should break here aswell I think. What architecture are you using? > I am working with a pandaboard (arm cortex a9). The pandaboard has a usb ethernet controller, so I need usb to boot over tftp. I guess that most arm configurations don't need usb for booting, and in that case you won't run into this problem. Regards, Fabian > Sascha > >> >> diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c >> index 7039a2c..369a393 100644 >> --- a/drivers/usb/core/usb.c >> +++ b/drivers/usb/core/usb.c >> @@ -50,6 +50,7 @@ >> #include <driver.h> >> #include <linux/ctype.h> >> #include <asm/byteorder.h> >> +#include <asm/unaligned.h> >> #include <xfuncs.h> >> #include <init.h> >> >> @@ -1071,6 +1072,7 @@ static int usb_hub_configure(struct usb_device *dev) >> struct usb_hub_status *hubsts; >> int i; >> struct usb_hub_device *hub; >> + unsigned short hub_chars; >> >> hub = xzalloc(sizeof (*hub)); >> dev->hub = hub; >> @@ -1100,8 +1102,8 @@ 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); >> + hub_chars = le16_to_cpu(get_unaligned(&descriptor->wHubCharacteristics)); >> + put_unaligned(hub_chars, &hub->desc.wHubCharacteristics); >> /* set the bitmap */ >> bitmap = (unsigned char *)&hub->desc.DeviceRemovable[0]; >> /* devices not removable by default */ >> @@ -1118,7 +1120,7 @@ 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) { >> + switch (hub_chars & HUB_CHAR_LPSM) { >> case 0x00: >> USB_HUB_PRINTF("ganged power switching\n"); >> break; >> @@ -1131,12 +1133,12 @@ static int usb_hub_configure(struct usb_device *dev) >> break; >> } >> >> - if (hub->desc.wHubCharacteristics & HUB_CHAR_COMPOUND) >> + if (hub_chars & 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 (hub_chars & 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 72f1c14..20c518a 100644 >> --- a/drivers/usb/host/ehci-hcd.c >> +++ b/drivers/usb/host/ehci-hcd.c >> @@ -33,6 +33,7 @@ >> #include <errno.h> >> #include <usb/ehci.h> >> #include <asm/mmu.h> >> +#include <asm/unaligned.h> >> >> #include "ehci.h" >> >> @@ -795,6 +796,7 @@ static int ehci_init(struct usb_host *host) >> struct ehci_priv *ehci = to_ehci(host); >> uint32_t reg; >> uint32_t cmd; >> + unsigned short hub_chars; >> >> ehci_halt(ehci); >> >> @@ -819,12 +821,15 @@ static int ehci_init(struct usb_host *host) >> reg = ehci_readl(&ehci->hccr->cr_hcsparams); >> descriptor.hub.bNbrPorts = HCS_N_PORTS(reg); >> >> + hub_chars = get_unaligned(&descriptor.hub.wHubCharacteristics); >> /* Port Indicators */ >> if (HCS_INDICATOR(reg)) >> - descriptor.hub.wHubCharacteristics |= 0x80; >> + hub_chars |= 0x80; >> /* Port Power Control */ >> if (HCS_PPC(reg)) >> - descriptor.hub.wHubCharacteristics |= 0x01; >> + hub_chars |= 0x01; >> + >> + put_unaligned(hub_chars, &descriptor.hub.wHubCharacteristics); >> >> /* Start the host controller. */ >> cmd = ehci_readl(&ehci->hcor->or_usbcmd); >> -- >> 1.7.0.4 >> >> >> _______________________________________________ >> barebox mailing list >> barebox@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/barebox >> > > -- > Pengutronix e.K. | | > Industrial Linux Solutions | http://www.pengutronix.de/ | > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | > _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] usb: fix unaligned access 2011-10-23 9:29 ` Fabian van der Werf @ 2011-10-24 15:14 ` Antony Pavlov 2011-10-24 17:07 ` Sascha Hauer 0 siblings, 1 reply; 10+ messages in thread From: Antony Pavlov @ 2011-10-24 15:14 UTC (permalink / raw) To: Fabian van der Werf; +Cc: barebox On 23 October 2011 13:29, Fabian van der Werf <fvanderwerf@gmail.com> wrote: >> >> Looks like a valid patch, I wonder that this never was a problem before. >> ARM should break here aswell I think. What architecture are you using? >> > > I am working with a pandaboard (arm cortex a9). The pandaboard has a > usb ethernet controller, so I need usb to boot over tftp. I guess that > most arm configurations don't need usb for booting, and in that case > you won't run into this problem. I have DUB-E100 USB Ethernet (ID 2001:3c05) connected to Toshiba AC100 (NVidia Tegra2 --- Cortex A9); tftp works fine without usb unaligned access patch. -- Best regards, Antony Pavlov _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] usb: fix unaligned access 2011-10-24 15:14 ` Antony Pavlov @ 2011-10-24 17:07 ` Sascha Hauer 2011-10-24 18:37 ` Fabian van der Werf 0 siblings, 1 reply; 10+ messages in thread From: Sascha Hauer @ 2011-10-24 17:07 UTC (permalink / raw) To: Antony Pavlov; +Cc: barebox On Mon, Oct 24, 2011 at 07:14:04PM +0400, Antony Pavlov wrote: > On 23 October 2011 13:29, Fabian van der Werf <fvanderwerf@gmail.com> wrote: > >> > >> Looks like a valid patch, I wonder that this never was a problem before. > >> ARM should break here aswell I think. What architecture are you using? > >> > > > > I am working with a pandaboard (arm cortex a9). The pandaboard has a > > usb ethernet controller, so I need usb to boot over tftp. I guess that > > most arm configurations don't need usb for booting, and in that case > > you won't run into this problem. > > I have DUB-E100 USB Ethernet (ID 2001:3c05) connected to Toshiba AC100 > (NVidia Tegra2 --- Cortex A9); tftp works fine without usb unaligned > access patch. Works for me aswell, though the access definitely is unaligned. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] usb: fix unaligned access 2011-10-24 17:07 ` Sascha Hauer @ 2011-10-24 18:37 ` Fabian van der Werf 2011-10-24 19:02 ` Eric Bénard 0 siblings, 1 reply; 10+ messages in thread From: Fabian van der Werf @ 2011-10-24 18:37 UTC (permalink / raw) To: Sascha Hauer; +Cc: barebox On Mon, Oct 24, 2011 at 7:07 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote: > On Mon, Oct 24, 2011 at 07:14:04PM +0400, Antony Pavlov wrote: >> On 23 October 2011 13:29, Fabian van der Werf <fvanderwerf@gmail.com> wrote: >> >> >> >> Looks like a valid patch, I wonder that this never was a problem before. >> >> ARM should break here aswell I think. What architecture are you using? >> >> >> > >> > I am working with a pandaboard (arm cortex a9). The pandaboard has a >> > usb ethernet controller, so I need usb to boot over tftp. I guess that >> > most arm configurations don't need usb for booting, and in that case >> > you won't run into this problem. >> >> I have DUB-E100 USB Ethernet (ID 2001:3c05) connected to Toshiba AC100 >> (NVidia Tegra2 --- Cortex A9); tftp works fine without usb unaligned >> access patch. > > Works for me aswell, though the access definitely is unaligned. > Okay, I think it may be a compiler problem. The latest code sourcery compiler builds a barebox that breaks on usb. 2009q1-203 builds fine, however. In the usb code the compiler should be able to figure out that the access is unaligned from the packed structure. So I guess it should split up the access in multiple loads/stores. I will look into the binaries to confirm this. The latest compiler may be broken or maybe the default behaviour has changed because armv7 actually supports unaligned access. Fabian > Sascha > > > -- > Pengutronix e.K. | | > Industrial Linux Solutions | http://www.pengutronix.de/ | > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | > _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] usb: fix unaligned access 2011-10-24 18:37 ` Fabian van der Werf @ 2011-10-24 19:02 ` Eric Bénard 2011-10-24 19:30 ` Sascha Hauer 0 siblings, 1 reply; 10+ messages in thread From: Eric Bénard @ 2011-10-24 19:02 UTC (permalink / raw) To: barebox Hi, Le 24/10/2011 20:37, Fabian van der Werf a écrit : > > Okay, I think it may be a compiler problem. The latest code sourcery > compiler builds a barebox that breaks on usb. 2009q1-203 builds fine, > however. > > In the usb code the compiler should be able to figure out that the > access is unaligned from the packed structure. So I guess it should > split up the access in multiple loads/stores. I will look into the > binaries to confirm this. The latest compiler may be broken or maybe > the default behaviour has changed because armv7 actually supports > unaligned access. > can't this be the same problem described here with gcc 4.6 : https://bugs.launchpad.net/ubuntu/+source/linux/+bug/791552 solved by this patch : https://launchpadlibrarian.net/73908303/0001-USB-ehci-remove-structure-packing-from-ehci_def.patch with the following explanation : The kernel source marks ehci_regs as packed. gcc 4.6 treats all accesses to packed structures as unaligned and ends up reading the status register multiple times. Eric _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] usb: fix unaligned access 2011-10-24 19:02 ` Eric Bénard @ 2011-10-24 19:30 ` Sascha Hauer 2011-10-24 19:42 ` Eric Bénard 0 siblings, 1 reply; 10+ messages in thread From: Sascha Hauer @ 2011-10-24 19:30 UTC (permalink / raw) To: Eric Bénard; +Cc: barebox On Mon, Oct 24, 2011 at 09:02:53PM +0200, Eric Bénard wrote: > Hi, > > Le 24/10/2011 20:37, Fabian van der Werf a écrit : > > > >Okay, I think it may be a compiler problem. The latest code sourcery > >compiler builds a barebox that breaks on usb. 2009q1-203 builds fine, > >however. > > > >In the usb code the compiler should be able to figure out that the > >access is unaligned from the packed structure. So I guess it should > >split up the access in multiple loads/stores. I will look into the > >binaries to confirm this. The latest compiler may be broken or maybe > >the default behaviour has changed because armv7 actually supports > >unaligned access. > > > can't this be the same problem described here with gcc 4.6 : > https://bugs.launchpad.net/ubuntu/+source/linux/+bug/791552 > > solved by this patch : > https://launchpadlibrarian.net/73908303/0001-USB-ehci-remove-structure-packing-from-ehci_def.patch > > with the following explanation : > The kernel source marks ehci_regs as packed. gcc 4.6 treats all > accesses to packed structures as unaligned and ends up reading the > status register multiple times. If Fabians compiler would treat every access to packed structure members as unaligned everything would be fine. The problem seems to be that it doesn't treat it as unaligned. Let's wait for Fabians binary analysis. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] usb: fix unaligned access 2011-10-24 19:30 ` Sascha Hauer @ 2011-10-24 19:42 ` Eric Bénard 2011-10-25 18:36 ` Fabian van der Werf 0 siblings, 1 reply; 10+ messages in thread From: Eric Bénard @ 2011-10-24 19:42 UTC (permalink / raw) To: barebox@lists.infradead.org Le 24/10/2011 21:30, Sascha Hauer a écrit : > On Mon, Oct 24, 2011 at 09:02:53PM +0200, Eric Bénard wrote: >> Hi, >> >> Le 24/10/2011 20:37, Fabian van der Werf a écrit : >>> >>> Okay, I think it may be a compiler problem. The latest code sourcery >>> compiler builds a barebox that breaks on usb. 2009q1-203 builds fine, >>> however. >>> >>> In the usb code the compiler should be able to figure out that the >>> access is unaligned from the packed structure. So I guess it should >>> split up the access in multiple loads/stores. I will look into the >>> binaries to confirm this. The latest compiler may be broken or maybe >>> the default behaviour has changed because armv7 actually supports >>> unaligned access. >>> >> can't this be the same problem described here with gcc 4.6 : >> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/791552 >> >> solved by this patch : >> https://launchpadlibrarian.net/73908303/0001-USB-ehci-remove-structure-packing-from-ehci_def.patch >> >> with the following explanation : >> The kernel source marks ehci_regs as packed. gcc 4.6 treats all >> accesses to packed structures as unaligned and ends up reading the >> status register multiple times. > > If Fabians compiler would treat every access to packed structure members > as unaligned everything would be fine. The problem seems to be that it > doesn't treat it as unaligned. Let's wait for Fabians binary analysis. > maybe the complete explanation will explain better the problem as barebox seems to have the same readl as linux : http://gcc.gnu.org/ml/gcc/2011-02/msg00035.html Eric _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] usb: fix unaligned access 2011-10-24 19:42 ` Eric Bénard @ 2011-10-25 18:36 ` Fabian van der Werf 0 siblings, 0 replies; 10+ messages in thread From: Fabian van der Werf @ 2011-10-25 18:36 UTC (permalink / raw) To: Eric Bénard; +Cc: barebox@lists.infradead.org On Mon, Oct 24, 2011 at 9:42 PM, Eric Bénard <eric@eukrea.com> wrote: > Le 24/10/2011 21:30, Sascha Hauer a écrit : >> >> On Mon, Oct 24, 2011 at 09:02:53PM +0200, Eric Bénard wrote: >>> >>> Hi, >>> >>> Le 24/10/2011 20:37, Fabian van der Werf a écrit : >>>> >>>> Okay, I think it may be a compiler problem. The latest code sourcery >>>> compiler builds a barebox that breaks on usb. 2009q1-203 builds fine, >>>> however. >>>> >>>> In the usb code the compiler should be able to figure out that the >>>> access is unaligned from the packed structure. So I guess it should >>>> split up the access in multiple loads/stores. I will look into the >>>> binaries to confirm this. The latest compiler may be broken or maybe >>>> the default behaviour has changed because armv7 actually supports >>>> unaligned access. >>>> >>> can't this be the same problem described here with gcc 4.6 : >>> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/791552 >>> >>> solved by this patch : >>> >>> https://launchpadlibrarian.net/73908303/0001-USB-ehci-remove-structure-packing-from-ehci_def.patch >>> >>> with the following explanation : >>> The kernel source marks ehci_regs as packed. gcc 4.6 treats all >>> accesses to packed structures as unaligned and ends up reading the >>> status register multiple times. >> >> If Fabians compiler would treat every access to packed structure members >> as unaligned everything would be fine. The problem seems to be that it >> doesn't treat it as unaligned. Let's wait for Fabians binary analysis. >> > maybe the complete explanation will explain better the problem as barebox > seems to have the same readl as linux : > http://gcc.gnu.org/ml/gcc/2011-02/msg00035.html I have looked at the disassembly and the compiler does indeed emit an unaligned access. Considering the following code from ehci_init() /* Port Indicators */ if (HCS_INDICATOR(reg)) descriptor.hub.wHubCharacteristics |= 0x80; /* wHubCharacteristics is unaligned */ /* Port Power Control */ if (HCS_PPC(reg)) descriptor.hub.wHubCharacteristics |= 0x01; My older (gcc 4.3.3) compiler correctly uses byte loads/stores: 0x8f0171e4 <+148>: tst r1, #65536 ; 0x10000 0x8f0171e8 <+152>: and r3, r1, #15 0x8f0171ec <+156>: strb r3, [r0, #2] 0x8f0171f0 <+160>: beq 0x8f017210 <ehci_init+192> 0x8f0171f4 <+164>: ldrb r2, [r0, #4] 0x8f0171f8 <+168>: ldrb r3, [r0, #3] 0x8f0171fc <+172>: orr r3, r3, r2, lsl #8 0x8f017200 <+176>: orr r3, r3, #128 ; 0x80 0x8f017204 <+180>: strb r3, [r0, #3] 0x8f017208 <+184>: lsr r3, r3, #8 0x8f01720c <+188>: strb r3, [r0, #4] 0x8f017210 <+192>: tst r1, #16 0x8f017214 <+196>: beq 0x8f017238 <ehci_init+232> 0x8f017218 <+200>: ldr r3, [pc, #92] ; 0x8f01727c 0x8f01721c <+204>: ldrb r1, [r3, #4] 0x8f017220 <+208>: ldrb r2, [r3, #3] 0x8f017224 <+212>: orr r2, r2, r1, lsl #8 0x8f017228 <+216>: orr r2, r2, #1 0x8f01722c <+220>: strb r2, [r3, #3] 0x8f017230 <+224>: lsr r2, r2, #8 0x8f017234 <+228>: strb r2, [r3, #4] My newer compiler (gcc 4.5.2) uses unaligned halfword loads/stores: 0x8f0159d8 <+148>: tst r2, #65536 ; 0x10000 0x8f0159dc <+152>: and r1, r2, #15 0x8f0159e0 <+156>: strb r1, [r3, #2] 0x8f0159e4 <+160>: ldrhne r1, [r3, #3] 0x8f0159e8 <+164>: orrne r1, r1, #128 ; 0x80 0x8f0159ec <+168>: strhne r1, [r3, #3] 0x8f0159f0 <+172>: tst r2, #16 0x8f0159f4 <+176>: ldrhne r2, [r3, #3] 0x8f0159f8 <+180>: orrne r2, r2, #1 0x8f0159fc <+184>: strhne r2, [r3, #3] I found this post: http://communities.mentor.com/community/cs/archives/arm-gnu/msg04203.html. That mentions that unaligned access is the default for armv6 and armv7. There are two possible fixes: - disable alignment fault checking for armv6 and armv7 - build with the -mno-unaligned-access option Besides that, I think we should look whether the structure really needs to be packed. As far as I understand the code, the structure is not used for i/o access, so packing is not necessary. Regards, Fabian > > Eric > > _______________________________________________ > barebox mailing list > barebox@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/barebox > _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2011-10-25 18:36 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-10-22 13:19 [PATCH] usb: fix unaligned access Fabian van der Werf 2011-10-22 20:20 ` Sascha Hauer 2011-10-23 9:29 ` Fabian van der Werf 2011-10-24 15:14 ` Antony Pavlov 2011-10-24 17:07 ` Sascha Hauer 2011-10-24 18:37 ` Fabian van der Werf 2011-10-24 19:02 ` Eric Bénard 2011-10-24 19:30 ` Sascha Hauer 2011-10-24 19:42 ` Eric Bénard 2011-10-25 18:36 ` Fabian van der Werf
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.