[U-Boot] [PATCH 2/2] usb: gadget: Use unaligned access for wMaxPacketSize

[Vivek Gautam]

Hi,


On Mon, May 13, 2013 at 4:20 PM, Kyungmin Park <kmpark at infradead.org> wrote:
> On Mon, May 13, 2013 at 7:23 PM, Vivek Gautam <gautam.vivek at samsung.com>wrote:
>
>> Use get_unaligned() while fetching wMaxPacketSize to avoid
>> voilating any alignment rules.

typo here s/voilating/violating

>>
>
> It's another story, can we get performance gain with unaligned access
> feature? In case of kernel, we got some gains.

This change was necessary since we get data abort if we try to fetch
wMaxPacketSize
which violates the natural alignment rules being a member of "struct
usb_endpoint_descriptor".

I am not sure of the performance gains with this. However, if you can
guide me how to
measure that, we can get the numbers if any.

>
> Anyway, good job!

Thanks !

>
> Acked-by: Kyungmin Park <kyungmin.park at samsung.com>
>
>>
>> Signed-off-by: Vivek Gautam <gautam.vivek at samsung.com>
>> Cc: Lukasz Majewski <l.majewski at samsung.com>
>> Cc: Piotr Wilczek <p.wilczek at samsung.com>
>> Cc: Kyungmin Park <kyungmin.park at samsung.com>
>> Cc: Lukasz Dalek <luk0104 at gmail.com>
>> Cc: Marek Vasut <marex at denx.de>
>> ---
>>
>> Just did a build test on u-boot-usb/master branch.
>> Need to be tested further.
>>
>> Based on u-boot-usb/next.
>>
>>  drivers/usb/gadget/f_mass_storage.c |    3 ++-
>>  drivers/usb/gadget/pxa25x_udc.c     |   13 +++++++------
>>  2 files changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/f_mass_storage.c
>> b/drivers/usb/gadget/f_mass_storage.c
>> index c28866f..45bc132 100644
>> --- a/drivers/usb/gadget/f_mass_storage.c
>> +++ b/drivers/usb/gadget/f_mass_storage.c
>> @@ -2261,7 +2261,8 @@ reset:
>>         if (rc)
>>                 goto reset;
>>         fsg->bulk_out_enabled = 1;
>> -       common->bulk_out_maxpacket = le16_to_cpu(d->wMaxPacketSize);
>> +       common->bulk_out_maxpacket =
>> +
>> le16_to_cpu(get_unaligned(&d->wMaxPacketSize));
>>         clear_bit(IGNORE_BULK_OUT, &fsg->atomic_bitflags);
>>
>>         /* Allocate the requests */
>> diff --git a/drivers/usb/gadget/pxa25x_udc.c
>> b/drivers/usb/gadget/pxa25x_udc.c
>> index 9ce98f0..085503d 100644
>> --- a/drivers/usb/gadget/pxa25x_udc.c
>> +++ b/drivers/usb/gadget/pxa25x_udc.c
>> @@ -314,7 +314,8 @@ static int pxa25x_ep_enable(struct usb_ep *_ep,
>>         if (!_ep || !desc || ep->desc || _ep->name == ep0name
>>                         || desc->bDescriptorType != USB_DT_ENDPOINT
>>                         || ep->bEndpointAddress != desc->bEndpointAddress
>> -                       || ep->fifo_size <
>> le16_to_cpu(desc->wMaxPacketSize)) {
>> +                       || ep->fifo_size <
>> +
>>  le16_to_cpu(get_unaligned(&desc->wMaxPacketSize))) {
>>                 printf("%s, bad ep or descriptor\n", __func__);
>>                 return -EINVAL;
>>         }
>> @@ -329,9 +330,9 @@ static int pxa25x_ep_enable(struct usb_ep *_ep,
>>
>>         /* hardware _could_ do smaller, but driver doesn't */
>>         if ((desc->bmAttributes == USB_ENDPOINT_XFER_BULK
>> -                               && le16_to_cpu(desc->wMaxPacketSize)
>> +                       &&
>> le16_to_cpu(get_unaligned(&desc->wMaxPacketSize))
>>                                                 != BULK_FIFO_SIZE)
>> -                       || !desc->wMaxPacketSize) {
>> +                       || !get_unaligned(&desc->wMaxPacketSize)) {
>>                 printf("%s, bad %s maxpacket\n", __func__, _ep->name);
>>                 return -ERANGE;
>>         }
>> @@ -345,7 +346,7 @@ static int pxa25x_ep_enable(struct usb_ep *_ep,
>>         ep->desc = desc;
>>         ep->stopped = 0;
>>         ep->pio_irqs = 0;
>> -       ep->ep.maxpacket = le16_to_cpu(desc->wMaxPacketSize);
>> +       ep->ep.maxpacket =
>> le16_to_cpu(get_unaligned(&desc->wMaxPacketSize));
>>
>>         /* flush fifo (mostly for OUT buffers) */
>>         pxa25x_ep_fifo_flush(_ep);
>> @@ -485,7 +486,7 @@ write_fifo(struct pxa25x_ep *ep, struct pxa25x_request
>> *req)
>>  {
>>         unsigned max;
>>
>> -       max = le16_to_cpu(ep->desc->wMaxPacketSize);
>> +       max = le16_to_cpu(get_unaligned(&ep->desc->wMaxPacketSize));
>>         do {
>>                 unsigned count;
>>                 int is_last, is_short;
>> @@ -766,7 +767,7 @@ pxa25x_ep_queue(struct usb_ep *_ep, struct usb_request
>> *_req, gfp_t gfp_flags)
>>          */
>>         if (unlikely(ep->bmAttributes == USB_ENDPOINT_XFER_ISOC
>>                         && req->req.length >
>> -                       le16_to_cpu(ep->desc->wMaxPacketSize)))
>> +
>> le16_to_cpu(get_unaligned(&ep->desc->wMaxPacketSize))))
>>                 return -EMSGSIZE;
>>
>>         debug_cond(NOISY, "%s queue req %p, len %d buf %p\n",
>> --
>> 1.7.6.5
>>
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot at lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot
>>
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>



-- 
Best Regards
Vivek