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

login
register
mail settings
Submitter Vivek Gautam
Date March 27, 2013, 9:28 a.m.
Message ID <1364376543-7526-2-git-send-email-gautam.vivek@samsung.com>
Download mbox | patch
Permalink /patch/231628/
State Awaiting Upstream
Delegated to: Marek Vasut
Headers show

Comments

Vivek Gautam - March 27, 2013, 9:28 a.m.
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(-)
Marek Vasut - March 28, 2013, 2:26 p.m.
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.

>  			}
>  			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) {
> -				usb_max_devs++;
> -		}
> +				    &usb_dev_desc[usb_max_devs]) == 1)
> +					usb_max_devs++;
>  			}
> +


What is this change here doing?

>  		}
>  		/* if storage device */
>  		if (usb_max_devs == USB_MAX_STOR_DEV) {

[..]

Best regards,
Marek Vasut
Vivek Gautam - April 2, 2013, 5:54 a.m.
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];

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.

>
>>               }
>>               /* 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
Vivek Gautam - April 2, 2013, 5:56 a.m.
On Tue, Apr 2, 2013 at 11:24 AM, Vivek Gautam <gautamvivek1987@gmail.com> wrote:
> 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];
>
> 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
Marek Vasut - April 2, 2013, 6:26 a.m.
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@lists.denx.de
> > http://lists.denx.de/mailman/listinfo/u-boot

Best regards,
Marek Vasut
Vivek Gautam - April 2, 2013, 6:37 a.m.
On Tue, Apr 2, 2013 at 11:56 AM, Marek Vasut <marex@denx.de> wrote:
> 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 ?

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 ?

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

Alright, will amend this. :-)

>
>> >> -                             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) {
[..]
Marek Vasut - April 2, 2013, 6:46 a.m.
Dear Vivek Gautam,

> On Tue, Apr 2, 2013 at 11:56 AM, Marek Vasut <marex@denx.de> wrote:
> > 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 ?
> 
> 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
Vivek Gautam - April 2, 2013, 6:58 a.m.
Hi,


On Tue, Apr 2, 2013 at 12:16 PM, Marek Vasut <marex@denx.de> wrote:
> Dear Vivek Gautam,
>
>> On Tue, Apr 2, 2013 at 11:56 AM, Marek Vasut <marex@denx.de> wrote:
>> > 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 ?
>>
>> 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.

Patch

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++;
 			}
 			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) {
-				usb_max_devs++;
-		}
+				    &usb_dev_desc[usb_max_devs]) == 1)
+					usb_max_devs++;
 			}
+
 		}
 		/* if storage device */
 		if (usb_max_devs == USB_MAX_STOR_DEV) {
@@ -511,7 +511,7 @@  static int usb_stor_BBB_comdat(ccb *srb, struct us_data *us)
 	dir_in = US_DIRECTION(srb->cmd[0]);
 
 #ifdef BBB_COMDAT_TRACE
-	printf("dir %d lun %d cmdlen %d cmd %p datalen %d pdata %p\n",
+	printf("dir %d lun %d cmdlen %d cmd %p datalen %lu pdata %p\n",
 		dir_in, srb->lun, srb->cmdlen, srb->cmd, srb->datalen,
 		srb->pdata);
 	if (srb->cmdlen) {
@@ -1218,6 +1218,7 @@  int usb_storage_probe(struct usb_device *dev, unsigned int ifnum,
 {
 	struct usb_interface *iface;
 	int i;
+	struct usb_endpoint_descriptor *ep_desc;
 	unsigned int flags = 0;
 
 	int protocol = 0;
@@ -1300,24 +1301,25 @@  int usb_storage_probe(struct usb_device *dev, unsigned int ifnum,
 	 * We will ignore any others.
 	 */
 	for (i = 0; i < iface->desc.bNumEndpoints; i++) {
+		ep_desc = &iface->ep_desc[i];
 		/* is it an BULK endpoint? */
-		if ((iface->ep_desc[i].bmAttributes &
+		if ((ep_desc->bmAttributes &
 		     USB_ENDPOINT_XFERTYPE_MASK) == USB_ENDPOINT_XFER_BULK) {
-			if (iface->ep_desc[i].bEndpointAddress & USB_DIR_IN)
-				ss->ep_in = iface->ep_desc[i].bEndpointAddress &
-					USB_ENDPOINT_NUMBER_MASK;
+			if (ep_desc->bEndpointAddress & USB_DIR_IN)
+				ss->ep_in = ep_desc->bEndpointAddress &
+						USB_ENDPOINT_NUMBER_MASK;
 			else
 				ss->ep_out =
-					iface->ep_desc[i].bEndpointAddress &
+					ep_desc->bEndpointAddress &
 					USB_ENDPOINT_NUMBER_MASK;
 		}
 
 		/* is it an interrupt endpoint? */
-		if ((iface->ep_desc[i].bmAttributes &
-		    USB_ENDPOINT_XFERTYPE_MASK) == USB_ENDPOINT_XFER_INT) {
-			ss->ep_int = iface->ep_desc[i].bEndpointAddress &
-				USB_ENDPOINT_NUMBER_MASK;
-			ss->irqinterval = iface->ep_desc[i].bInterval;
+		if ((ep_desc->bmAttributes &
+		     USB_ENDPOINT_XFERTYPE_MASK) == USB_ENDPOINT_XFER_INT) {
+			ss->ep_int = ep_desc->bEndpointAddress &
+						USB_ENDPOINT_NUMBER_MASK;
+			ss->irqinterval = ep_desc->bInterval;
 		}
 	}
 	USB_STOR_PRINTF("Endpoints In %d Out %d Int %d\n",
diff --git a/include/usb_defs.h b/include/usb_defs.h
index 9502544..0c78d9d 100644
--- a/include/usb_defs.h
+++ b/include/usb_defs.h
@@ -234,7 +234,7 @@ 
 #define HUB_CHAR_OCPM               0x0018
 
 /*
- *Hub Status & Hub Change bit masks
+ * Hub Status & Hub Change bit masks
  */
 #define HUB_STATUS_LOCAL_POWER	0x0001
 #define HUB_STATUS_OVERCURRENT	0x0002