[U-Boot] OMAP4 u-boot broken (was [PATCH] usb: align usb_endpoint_descriptor to 16-bit boundary)

[Tom Rini]

On Tue, Dec 13, 2011 at 1:15 PM, Stefan Kristiansson
<stefan.kristiansson at 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:
> 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.

I'm working on option one now.

-- 
Tom