| Message ID | 1364376543-7526-2-git-send-email-gautam.vivek@samsung.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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) { [..]
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
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.
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