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

[Marek Vasut]

Dear 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 ?

Ah I see, yep, indexing the array with -1 is no good. You can add a safety-check 
there , but the -1 should never reach that place indeed.

Best regards,
Marek Vasut