[U-Boot] [PATCH 1/7] USB: Some cleanup prior to USB 3.0 interface addition
Vivek Gautam
gautamvivek1987 at gmail.com
Tue Apr 2 08:58:12 CEST 2013
Hi,
On Tue, Apr 2, 2013 at 12:16 PM, Marek Vasut <marex at denx.de> wrote:
> 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.
>
Ok, will amend this as required and update.
--
Thanks & Regards
Vivek
More information about the U-Boot
mailing list