Dear Vivek Gautam,
> On Tue, Apr 2, 2013 at 11:56 AM, Marek Vasut <ma...@denx.de> wrote:
> > Dear Vivek Gautam,
> >
> >> Hi Marek,
> >>
> >> On Thu, Mar 28, 2013 at 7:56 PM, Marek Vasut <ma...@denx.de> wrote:
> >> > Dear Vivek Gautam,
> >> >
> >> >> Some cleanup in usb framework, nothing much on feature side.
> >> >>
> >> >> Signed-off-by: Vikas C Sajjan <vikas.saj...@samsung.com>
> >> >> Signed-off-by: Vivek Gautam <gautam.vi...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
