mail archive of the barebox mailing list
 help / color / mirror / Atom feed
* [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	[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

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

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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox