[U-Boot] [PATCH 1/7] USB: Some cleanup prior to USB 3.0 interface addition

[Vivek Gautam]

On Tue, Apr 2, 2013 at 11:56 AM, Marek Vasut <marex at denx.de> wrote:
> Dear Vivek Gautam,
>
>> Hi Marek,
>>
>> On Thu, Mar 28, 2013 at 7:56 PM, Marek Vasut <marex at denx.de> wrote:
>> > Dear Vivek Gautam,
>> >
>> >> Some cleanup in usb framework, nothing much on feature side.
>> >>
>> >> Signed-off-by: Vikas C Sajjan <vikas.sajjan at samsung.com>
>> >> Signed-off-by: Vivek Gautam <gautam.vivek at samsung.com>
>> >> ---
>> >>
>> >>  common/usb.c         |   18 ++++++++++--------
>> >>  common/usb_storage.c |   30 ++++++++++++++++--------------
>> >>  include/usb_defs.h   |    2 +-
>> >>  3 files changed, 27 insertions(+), 23 deletions(-)
>> >>
>> >> diff --git a/common/usb.c b/common/usb.c
>> >> index 6fc0fc1..40c1547 100644
>> >> --- a/common/usb.c
>> >> +++ b/common/usb.c
>> >> @@ -360,6 +360,7 @@ static int usb_parse_config(struct usb_device *dev,
>> >>
>> >>       int index, ifno, epno, curr_if_num;
>> >>       int i;
>> >>       u16 ep_wMaxPacketSize;
>> >>
>> >> +     struct usb_interface *if_desc = NULL;
>> >>
>> >>       ifno = -1;
>> >>       epno = -1;
>> >>
>> >> @@ -387,23 +388,24 @@ static int usb_parse_config(struct usb_device
>> >> *dev,
>> >>
>> >>                            &buffer[index])->bInterfaceNumber !=
>> >>                            curr_if_num) {
>> >>
>> >>                               /* this is a new interface, copy new desc
>> >>                               */ ifno = dev->config.no_of_if;
>> >>
>> >> +                             if_desc = &dev->config.if_desc[ifno];
>> >>
>> >>                               dev->config.no_of_if++;
>> >>
>> >> -                             memcpy(&dev->config.if_desc[ifno],
>> >> -                                     &buffer[index], buffer[index]);
>> >> -                             dev->config.if_desc[ifno].no_of_ep = 0;
>> >> -                             dev->config.if_desc[ifno].num_altsetting =
>> >> 1; +                             memcpy(if_desc, &buffer[index],
>> >> buffer[index]); +                             if_desc->no_of_ep = 0;
>> >> +                             if_desc->num_altsetting = 1;
>> >>
>> >>                               curr_if_num =
>> >>
>> >> -                                  dev-
>> >>
>> >>config.if_desc[ifno].desc.bInterfaceNumber;
>> >>
>> >> +                                  if_desc->desc.bInterfaceNumber;
>> >>
>> >>                       } else {
>> >>
>> >>                               /* found alternate setting for the
>> >>                               interface */
>> >>
>> >> -
>> >> dev->config.if_desc[ifno].num_altsetting++; +
>> >>   if_desc->num_altsetting++;
>> >
>> > This will crash as if_desc is set in the if() branch above and it will be
>> > NULL in the else{} branch.
>>
>> True, will add here
>> if_desc = &dev->config.if_desc[ifno];
>
> Yea ;-)
>
>> One more thing actually i could notice, "ifno = -1" in this else section.
>> Is this what we want it here ?
>
> Maybe I don't see it ?

ifno is initailized to -1.
Now when descriptor is 'Interface type' (case USB_DT_INTERFACE) ifno
is update in if part only as below:
ifno = dev->config.no_of_if;

This would not be reaching else part, right ?

>
>> >>                       }
>> >>                       break;
>> >>
>> >>               case USB_DT_ENDPOINT:
>> >>                       epno = dev->config.if_desc[ifno].no_of_ep;
>> >>
>> >> +                     if_desc = &dev->config.if_desc[ifno];
>> >>
>> >>                       /* found an endpoint */
>> >>
>> >> -                     dev->config.if_desc[ifno].no_of_ep++;
>> >> -                     memcpy(&dev->config.if_desc[ifno].ep_desc[epno],
>> >> +                     if_desc->no_of_ep++;
>> >> +                     memcpy(&if_desc->ep_desc[epno],
>> >>
>> >>                               &buffer[index], buffer[index]);
>> >>
>> >>                       ep_wMaxPacketSize = get_unaligned(&dev->config.\
>> >>
>> >>                                                       if_desc[ifno].\
>> >>
>> >> diff --git a/common/usb_storage.c b/common/usb_storage.c
>> >> index fb322b4..475c218 100644
>> >> --- a/common/usb_storage.c
>> >> +++ b/common/usb_storage.c
>> >> @@ -278,10 +278,10 @@ int usb_stor_scan(int mode)
>> >>
>> >>                            lun++) {
>> >>
>> >>                               usb_dev_desc[usb_max_devs].lun = lun;
>> >>                               if (usb_stor_get_info(dev,
>> >>                               &usb_stor[start],
>> >>
>> >> -
>> >
>> > &usb_dev_desc[usb_max_devs]) == 1) {
>>
>> Do we really need this brace here? We just have one statement under this
>> if.
>
> If the condition is multi-line, then we use brace, yes.

Alright, will amend this. :-)

>
>> >> -                             usb_max_devs++;
>> >> -             }
>> >> +                                 &usb_dev_desc[usb_max_devs]) == 1)
>> >> +                                     usb_max_devs++;
>> >>
>> >>                       }
>> >>
>> >> +
>> >
>> > What is this change here doing?
>>
>> There were these unaligned braces for if part, which we removed.
>>
>> >>               }
>> >>               /* if storage device */
>> >>               if (usb_max_devs == USB_MAX_STOR_DEV) {
[..]



-- 
Thanks & Regards
Vivek