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

From: Marek Vasut <marex@denx.de>
To: u-boot@lists.denx.de
Subject: [U-Boot] [PATCH 1/7] USB: Some cleanup prior to USB 3.0 interface addition
Date: Tue, 2 Apr 2013 08:26:50 +0200	[thread overview]
Message-ID: <201304020826.50300.marex@denx.de> (raw)
In-Reply-To: <CAFp+6iHVHfepxs42dNXyH2To8x08+y5Ny6XXz1RwKq9zwEwSeQ@mail.gmail.com>

Dear Vivek Gautam,

> Hi Marek,
> 
> On Thu, Mar 28, 2013 at 7:56 PM, Marek Vasut <marex@denx.de> wrote:
> > Dear Vivek Gautam,
> > 
> >> Some cleanup in usb framework, nothing much on feature side.
> >> 
> >> Signed-off-by: Vikas C Sajjan <vikas.sajjan@samsung.com>
> >> Signed-off-by: Vivek Gautam <gautam.vivek@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 ?

> >>                       }
> >>                       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.

> >> -                             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) {
> > 
> > [..]
> > 
> > Best regards,
> > Marek Vasut
> > _______________________________________________
> > U-Boot mailing list
> > U-Boot at lists.denx.de
> > http://lists.denx.de/mailman/listinfo/u-boot

Best regards,
Marek Vasut