[U-Boot] [PATCH] usb: align usb_endpoint_descriptor to 16-bit boundary
The usb_endpoint_descriptor struct is 7 bytes large and is defined as an array (ep_desc[USB_MAXENDPOINTS]) in the usb_interface struct in include/usb.h
This fact will result in that every odd index in that array will start at an uneven address, this in turn makes accesses to u16 wMaxPacketSize unaligned. Such accesses are illegal on the OpenRISC architecture (as well as other architectures) and will render a bus error.
Setting the aligned(2) attribute on usb_endpoint_descriptor will force wMaxPacketSize to a 16-bit boundary.
Signed-off-by: Stefan Kristiansson stefan.kristiansson@saunalahti.fi --- include/usbdescriptors.h | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/include/usbdescriptors.h b/include/usbdescriptors.h index 2dec3b9..392fcf5 100644 --- a/include/usbdescriptors.h +++ b/include/usbdescriptors.h @@ -199,7 +199,7 @@ struct usb_endpoint_descriptor { u8 bmAttributes; u16 wMaxPacketSize; u8 bInterval; -} __attribute__ ((packed)); +} __attribute__ ((packed)) __attribute__ ((aligned(2)));
struct usb_interface_descriptor { u8 bLength;
Hi,
2011/10/20 Stefan Kristiansson stefan.kristiansson@saunalahti.fi:
The usb_endpoint_descriptor struct is 7 bytes large and is defined as an array (ep_desc[USB_MAXENDPOINTS]) in the usb_interface struct in include/usb.h
This fact will result in that every odd index in that array will start at an uneven address, this in turn makes accesses to u16 wMaxPacketSize unaligned. Such accesses are illegal on the OpenRISC architecture (as well as other architectures) and will render a bus error.
Setting the aligned(2) attribute on usb_endpoint_descriptor will force wMaxPacketSize to a 16-bit boundary.
Signed-off-by: Stefan Kristiansson stefan.kristiansson@saunalahti.fi
Applied to u-boot-usb.
Kind regards,
Remy
Hi Kristiansson,
On Friday 21 October 2011 12:55 AM, Stefan Kristiansson wrote:
The usb_endpoint_descriptor struct is 7 bytes large and is defined as an array (ep_desc[USB_MAXENDPOINTS]) in the usb_interface struct in include/usb.h
This fact will result in that every odd index in that array will start at an uneven address, this in turn makes accesses to u16 wMaxPacketSize unaligned. Such accesses are illegal on the OpenRISC architecture (as well as other architectures) and will render a bus error.
Setting the aligned(2) attribute on usb_endpoint_descriptor will force wMaxPacketSize to a 16-bit boundary.
Signed-off-by: Stefan Kristianssonstefan.kristiansson@saunalahti.fi
OMAP4 U-Boot is broken in the mainline. U-Boot wouldn't boot up on any OMAP4 platforms. I suspect this will be the case with any ARM platform that has enabled USB tty code. I git-bisected the issue to this patch. I did some analysis on it and here are my findings.
aligned(2) indeed makes the sizeof(struct usb_endpoint_descriptor) == 8. But that doesn't seem to be enough. struct acm_config_desc embeds fields of type usb_endpoint_descriptor and has the attribute 'packed'. As a result, these usb_endpoint_descriptor structures in acm_config_desc may have odd addresses and consequently wMaxPacketSize also has odd address. As far as I can see, this is not a new issue. But your patch fortunately or unfortunately brought it out. Here is how:
When 'usb_endpoint_descriptor' didn't have the 'aligned' attribute, compiler took extra care while accessing wMaxPacketSize. It did it using two byte reads and combining them to make a 16-bit half-word. When aligned was added compiler replaced it with a 'ldrh' (load half-word) instruction that resulted in an abort due the odd address.
Now, I am not sure how to solve this problem. I tried the following and the boot issue is gone.
diff --git a/drivers/serial/usbtty.c b/drivers/serial/usbtty.c index e2e87fe..2a961e9 100644 --- a/drivers/serial/usbtty.c +++ b/drivers/serial/usbtty.c @@ -151,7 +151,7 @@ struct acm_config_desc { /* Slave Interface */ struct usb_interface_descriptor data_class_interface; struct usb_endpoint_descriptor data_endpoints[NUM_ENDPOINTS-1]; -} __attribute__((packed)); +};
static struct acm_config_desc acm_configuration_descriptors[NUM_CONFIGS] = { {
I am not a USB expert, so not sure whether this works. Does that structure really have to be 'packed'? It will be great if USB experts can look into this and come up with a permanent solution at the earliest because quite a few platforms will be broken until then.
include/usbdescriptors.h | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/include/usbdescriptors.h b/include/usbdescriptors.h index 2dec3b9..392fcf5 100644 --- a/include/usbdescriptors.h +++ b/include/usbdescriptors.h @@ -199,7 +199,7 @@ struct usb_endpoint_descriptor { u8 bmAttributes; u16 wMaxPacketSize; u8 bInterval; -} __attribute__ ((packed)); +} __attribute__ ((packed)) __attribute__ ((aligned(2)));
struct usb_interface_descriptor { u8 bLength;
Hi Aneesh, On Tue, Dec 13, 2011 at 06:59:45PM +0530, Aneesh V wrote:
OMAP4 U-Boot is broken in the mainline. U-Boot wouldn't boot up on any OMAP4 platforms. I suspect this will be the case with any ARM platform that has enabled USB tty code. I git-bisected the issue to this patch. I did some analysis on it and here are my findings.
aligned(2) indeed makes the sizeof(struct usb_endpoint_descriptor) == 8. But that doesn't seem to be enough. struct acm_config_desc embeds fields of type usb_endpoint_descriptor and has the attribute 'packed'. As a result, these usb_endpoint_descriptor structures in acm_config_desc may have odd addresses and consequently wMaxPacketSize also has odd address. As far as I can see, this is not a new issue. But your patch fortunately or unfortunately brought it out. Here is how:
When 'usb_endpoint_descriptor' didn't have the 'aligned' attribute, compiler took extra care while accessing wMaxPacketSize. It did it using two byte reads and combining them to make a 16-bit half-word. When aligned was added compiler replaced it with a 'ldrh' (load half-word) instruction that resulted in an abort due the odd address.
How unpleasent, nice that you were able to pinpoint where and how it fails however.
Now, I am not sure how to solve this problem. I tried the following and the boot issue is gone.
diff --git a/drivers/serial/usbtty.c b/drivers/serial/usbtty.c index e2e87fe..2a961e9 100644 --- a/drivers/serial/usbtty.c +++ b/drivers/serial/usbtty.c @@ -151,7 +151,7 @@ struct acm_config_desc { /* Slave Interface */ struct usb_interface_descriptor data_class_interface; struct usb_endpoint_descriptor data_endpoints[NUM_ENDPOINTS-1]; -} __attribute__((packed)); +};
static struct acm_config_desc acm_configuration_descriptors[NUM_CONFIGS] = { {
I am not a USB expert, so not sure whether this works. Does that structure really have to be 'packed'? It will be great if USB experts can look into this and come up with a permanent solution at the earliest because quite a few platforms will be broken until then.
I am neither a USB expert, just the moron that apperantly broke a lot of OMAP4 boards.
The way I see it there are 3 options at hand here: 1) revert my patch and look over the places where wMaxPacketSize is used and wrap them in get/set_unaligned() (at least in non-arch specific code) 2) go forward with your approach 3) stop using usb_endpoint_descriptor in arrays
I think option nr 1 is the safest one here, but yes, input from USB experts would be great.
Stefan
Copying Wolfgang as he seems to be the author of this module.
On Wednesday 14 December 2011 01:45 AM, Stefan Kristiansson wrote:
Hi Aneesh, On Tue, Dec 13, 2011 at 06:59:45PM +0530, Aneesh V wrote:
OMAP4 U-Boot is broken in the mainline. U-Boot wouldn't boot up on any OMAP4 platforms. I suspect this will be the case with any ARM platform that has enabled USB tty code. I git-bisected the issue to this patch. I did some analysis on it and here are my findings.
aligned(2) indeed makes the sizeof(struct usb_endpoint_descriptor) == 8. But that doesn't seem to be enough. struct acm_config_desc embeds fields of type usb_endpoint_descriptor and has the attribute 'packed'. As a result, these usb_endpoint_descriptor structures in acm_config_desc may have odd addresses and consequently wMaxPacketSize also has odd address. As far as I can see, this is not a new issue. But your patch fortunately or unfortunately brought it out. Here is how:
When 'usb_endpoint_descriptor' didn't have the 'aligned' attribute, compiler took extra care while accessing wMaxPacketSize. It did it using two byte reads and combining them to make a 16-bit half-word. When aligned was added compiler replaced it with a 'ldrh' (load half-word) instruction that resulted in an abort due the odd address.
How unpleasent, nice that you were able to pinpoint where and how it fails however.
Now, I am not sure how to solve this problem. I tried the following and the boot issue is gone.
diff --git a/drivers/serial/usbtty.c b/drivers/serial/usbtty.c index e2e87fe..2a961e9 100644 --- a/drivers/serial/usbtty.c +++ b/drivers/serial/usbtty.c @@ -151,7 +151,7 @@ struct acm_config_desc { /* Slave Interface */ struct usb_interface_descriptor data_class_interface; struct usb_endpoint_descriptor data_endpoints[NUM_ENDPOINTS-1]; -} __attribute__((packed)); +};
static struct acm_config_desc acm_configuration_descriptors[NUM_CONFIGS] = { {
I am not a USB expert, so not sure whether this works. Does that structure really have to be 'packed'? It will be great if USB experts can look into this and come up with a permanent solution at the earliest because quite a few platforms will be broken until then.
I am neither a USB expert, just the moron that apperantly broke a lot of OMAP4 boards.
The way I see it there are 3 options at hand here:
- revert my patch and look over the places where wMaxPacketSize is used
and wrap them in get/set_unaligned() (at least in non-arch specific code) 2) go forward with your approach 3) stop using usb_endpoint_descriptor in arrays
I think option nr 1 is the safest one here, but yes, input from USB experts would be great.
Wolfgang,
What's your recommendation to solve this problem. Does the ACM structure really have to be 'packed'? The packed attribute is resulting in some of its fields to be at odd address, which in turn generates alignment faults on access. My original mail has more details.
This is now affecting any ARM board that has USB_TTY enabled.
best regards, Aneesh
On Wed, Dec 14, 2011 at 3:24 AM, Aneesh V aneesh@ti.com wrote:
Copying Wolfgang as he seems to be the author of this module.
On Wednesday 14 December 2011 01:45 AM, Stefan Kristiansson wrote:
Hi Aneesh, On Tue, Dec 13, 2011 at 06:59:45PM +0530, Aneesh V wrote:
OMAP4 U-Boot is broken in the mainline. U-Boot wouldn't boot up on any OMAP4 platforms. I suspect this will be the case with any ARM platform that has enabled USB tty code. I git-bisected the issue to this patch. I did some analysis on it and here are my findings.
aligned(2) indeed makes the sizeof(struct usb_endpoint_descriptor) == 8. But that doesn't seem to be enough. struct acm_config_desc embeds fields of type usb_endpoint_descriptor and has the attribute 'packed'. As a result, these usb_endpoint_descriptor structures in acm_config_desc may have odd addresses and consequently wMaxPacketSize also has odd address. As far as I can see, this is not a new issue. But your patch fortunately or unfortunately brought it out. Here is how:
When 'usb_endpoint_descriptor' didn't have the 'aligned' attribute, compiler took extra care while accessing wMaxPacketSize. It did it using two byte reads and combining them to make a 16-bit half-word. When aligned was added compiler replaced it with a 'ldrh' (load half-word) instruction that resulted in an abort due the odd address.
How unpleasent, nice that you were able to pinpoint where and how it fails however.
Now, I am not sure how to solve this problem. I tried the following and the boot issue is gone.
diff --git a/drivers/serial/usbtty.c b/drivers/serial/usbtty.c index e2e87fe..2a961e9 100644 --- a/drivers/serial/usbtty.c +++ b/drivers/serial/usbtty.c @@ -151,7 +151,7 @@ struct acm_config_desc { /* Slave Interface */ struct usb_interface_descriptor data_class_interface; struct usb_endpoint_descriptor data_endpoints[NUM_ENDPOINTS-1]; -} __attribute__((packed)); +};
static struct acm_config_desc acm_configuration_descriptors[NUM_CONFIGS] = { {
I am not a USB expert, so not sure whether this works. Does that structure really have to be 'packed'? It will be great if USB experts can look into this and come up with a permanent solution at the earliest because quite a few platforms will be broken until then.
I am neither a USB expert, just the moron that apperantly broke a lot of OMAP4 boards.
The way I see it there are 3 options at hand here:
- revert my patch and look over the places where wMaxPacketSize is used
and wrap them in get/set_unaligned() (at least in non-arch specific code) 2) go forward with your approach 3) stop using usb_endpoint_descriptor in arrays
I think option nr 1 is the safest one here, but yes, input from USB experts would be great.
Wolfgang,
What's your recommendation to solve this problem. Does the ACM structure really have to be 'packed'? The packed attribute is resulting in some of its fields to be at odd address, which in turn generates alignment faults on access. My original mail has more details.
This is now affecting any ARM board that has USB_TTY enabled.
Adding Wolfgang this time :)
On Tue, Dec 13, 2011 at 1:15 PM, Stefan Kristiansson stefan.kristiansson@saunalahti.fi wrote:
Hi Aneesh, On Tue, Dec 13, 2011 at 06:59:45PM +0530, Aneesh V wrote:
OMAP4 U-Boot is broken in the mainline. U-Boot wouldn't boot up on any OMAP4 platforms. I suspect this will be the case with any ARM platform that has enabled USB tty code. I git-bisected the issue to this patch. I did some analysis on it and here are my findings.
aligned(2) indeed makes the sizeof(struct usb_endpoint_descriptor) == 8. But that doesn't seem to be enough. struct acm_config_desc embeds fields of type usb_endpoint_descriptor and has the attribute 'packed'. As a result, these usb_endpoint_descriptor structures in acm_config_desc may have odd addresses and consequently wMaxPacketSize also has odd address. As far as I can see, this is not a new issue. But your patch fortunately or unfortunately brought it out. Here is how:
When 'usb_endpoint_descriptor' didn't have the 'aligned' attribute, compiler took extra care while accessing wMaxPacketSize. It did it using two byte reads and combining them to make a 16-bit half-word. When aligned was added compiler replaced it with a 'ldrh' (load half-word) instruction that resulted in an abort due the odd address.
How unpleasent, nice that you were able to pinpoint where and how it fails however.
Now, I am not sure how to solve this problem. I tried the following and the boot issue is gone.
diff --git a/drivers/serial/usbtty.c b/drivers/serial/usbtty.c index e2e87fe..2a961e9 100644 --- a/drivers/serial/usbtty.c +++ b/drivers/serial/usbtty.c @@ -151,7 +151,7 @@ struct acm_config_desc { /* Slave Interface */ struct usb_interface_descriptor data_class_interface; struct usb_endpoint_descriptor data_endpoints[NUM_ENDPOINTS-1]; -} __attribute__((packed)); +};
static struct acm_config_desc acm_configuration_descriptors[NUM_CONFIGS] = { {
I am not a USB expert, so not sure whether this works. Does that structure really have to be 'packed'? It will be great if USB experts can look into this and come up with a permanent solution at the earliest because quite a few platforms will be broken until then.
I am neither a USB expert, just the moron that apperantly broke a lot of OMAP4 boards.
The way I see it there are 3 options at hand here:
- revert my patch and look over the places where wMaxPacketSize is used
and wrap them in get/set_unaligned() (at least in non-arch specific code) 2) go forward with your approach 3) stop using usb_endpoint_descriptor in arrays
I think option nr 1 is the safest one here, but yes, input from USB experts would be great.
I'm working on option one now.
On 12/13/2011 02:29 PM, Aneesh V wrote:
Hi Kristiansson,
On Friday 21 October 2011 12:55 AM, Stefan Kristiansson wrote:
The usb_endpoint_descriptor struct is 7 bytes large and is defined as an array (ep_desc[USB_MAXENDPOINTS]) in the usb_interface struct in include/usb.h
This fact will result in that every odd index in that array will start at an uneven address, this in turn makes accesses to u16 wMaxPacketSize unaligned. Such accesses are illegal on the OpenRISC architecture (as well as other architectures) and will render a bus error.
Setting the aligned(2) attribute on usb_endpoint_descriptor will force wMaxPacketSize to a 16-bit boundary.
Signed-off-by: Stefan Kristianssonstefan.kristiansson@saunalahti.fi
OMAP4 U-Boot is broken in the mainline. U-Boot wouldn't boot up on any OMAP4 platforms. I suspect this will be the case with any ARM platform that has enabled USB tty code. I git-bisected the issue to this patch. I did some analysis on it and here are my findings.
I can confirm this on my OMAP3 platform with CONFIG_USB_TTY set. I can get it to boot again by either reverting this patch or unsetting CONFIG_USB_TTY. Looking forward to the fix.
-Michael
MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler Registergericht: Amtsgericht Stuttgart, HRB 271090 Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner, Erhard Meier
participants (5)
-
Aneesh V -
Michael Jones -
Remy Bohmer -
Stefan Kristiansson -
Tom Rini