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

[Tom Rini]

On Wed, Dec 14, 2011 at 3:24 AM, Aneesh V <aneesh at 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:
>> 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.
>
>
> 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 :)

-- 
Tom