On Tue, Apr 2, 2013 at 11:24 AM, Vivek Gautam <gautamvivek1...@gmail.com> wrote:
> 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];
>
> One more thing actually i could notice, "ifno = -1" in this else section.
> Is this what we want it here ?
>
>>
>>> }
>>> 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.
>
>>> - 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.
Please ignore above comment. It was meant for change prior to this, above.
Will remove this extra line here.
>
>>
>>> }
>>> /* if storage device */
>>> if (usb_max_devs == USB_MAX_STOR_DEV) {
>>
>> [..]
>>
>> Best regards,
>> Marek Vasut
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot@lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot
>
>
>
> --
> Thanks & Regards
> Vivek
--
Thanks & Regards
Vivek
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
