| Message ID | 1502430811-21475-1-git-send-email-suneelglinux@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Marek Vasut |
| Headers | show |
Hi Suneel, On 10 August 2017 at 23:53, Suneel Garapati <suneelglinux@gmail.com> wrote: > usb tree and info commands may cause crash otherwise > > Signed-off-by: Suneel Garapati <suneelglinux@gmail.com> > --- > cmd/usb.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > Thank you for the patch - it certainly looks like a bug. Can you please expand the commit message a little? E.g. you have UCLASS_USB_EMUL below. > diff --git a/cmd/usb.c b/cmd/usb.c > index 992d414..81e1a7b 100644 > --- a/cmd/usb.c > +++ b/cmd/usb.c > @@ -415,7 +415,8 @@ static void usb_show_tree_graph(struct usb_device *dev, char *pre) > udev = dev_get_parent_priv(child); > > /* Ignore emulators, we only want real devices */ > - if (device_get_uclass_id(child) != UCLASS_USB_EMUL) { > + if (device_get_uclass_id(child) != > + (UCLASS_USB_EMUL | UCLASS_BLK)) { This seems odd to me. Do you mean to check that the child uclass is neither USB_EMUL nor BLK? Would it be possible to check that the parent is UCLASS_USB? That seems like a better condition to determine whether the child has USB parent data. > usb_show_tree_graph(udev, pre); > pre[index] = 0; > } > @@ -605,7 +606,8 @@ static void usb_show_info(struct usb_device *udev) > for (device_find_first_child(udev->dev, &child); > child; > device_find_next_child(&child)) { > - if (device_active(child)) { > + if (device_active(child) && > + (device_get_uclass_id(child) != UCLASS_BLK)) { > udev = dev_get_parent_priv(child); > usb_show_info(udev); > } > -- > 2.7.4 > Regards, Simon
Hi Simon, On Sun, Aug 13, 2017 at 2:37 PM, Simon Glass <sjg@chromium.org> wrote: > Hi Suneel, > > On 10 August 2017 at 23:53, Suneel Garapati <suneelglinux@gmail.com> wrote: >> usb tree and info commands may cause crash otherwise >> >> Signed-off-by: Suneel Garapati <suneelglinux@gmail.com> >> --- >> cmd/usb.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> > > Thank you for the patch - it certainly looks like a bug. Can you > please expand the commit message a little? E.g. you have > UCLASS_USB_EMUL below. I will change the description > >> diff --git a/cmd/usb.c b/cmd/usb.c >> index 992d414..81e1a7b 100644 >> --- a/cmd/usb.c >> +++ b/cmd/usb.c >> @@ -415,7 +415,8 @@ static void usb_show_tree_graph(struct usb_device *dev, char *pre) >> udev = dev_get_parent_priv(child); >> >> /* Ignore emulators, we only want real devices */ >> - if (device_get_uclass_id(child) != UCLASS_USB_EMUL) { >> + if (device_get_uclass_id(child) != >> + (UCLASS_USB_EMUL | UCLASS_BLK)) { > > This seems odd to me. Do you mean to check that the child uclass is > neither USB_EMUL nor BLK? > > Would it be possible to check that the parent is UCLASS_USB? That > seems like a better condition to determine whether the child has USB > parent data. It is possible to check parent uclass but would that ever fail? I assume, block device under usb storage device will always have parent as usb class. Also, tree is called on only usb class devices. Maybe I am missing something. Regards, Suneel > >> usb_show_tree_graph(udev, pre); >> pre[index] = 0; >> } >> @@ -605,7 +606,8 @@ static void usb_show_info(struct usb_device *udev) >> for (device_find_first_child(udev->dev, &child); >> child; >> device_find_next_child(&child)) { >> - if (device_active(child)) { >> + if (device_active(child) && >> + (device_get_uclass_id(child) != UCLASS_BLK)) { >> udev = dev_get_parent_priv(child); >> usb_show_info(udev); >> } >> -- >> 2.7.4 >> > > Regards, > Simon
Hi, Request for review on comments below. Regards, Suneel On Mon, Aug 14, 2017 at 8:06 PM, Suneel Garapati <suneelglinux@gmail.com> wrote: > Hi Simon, > > > On Sun, Aug 13, 2017 at 2:37 PM, Simon Glass <sjg@chromium.org> wrote: >> Hi Suneel, >> >> On 10 August 2017 at 23:53, Suneel Garapati <suneelglinux@gmail.com> wrote: >>> usb tree and info commands may cause crash otherwise >>> >>> Signed-off-by: Suneel Garapati <suneelglinux@gmail.com> >>> --- >>> cmd/usb.c | 6 ++++-- >>> 1 file changed, 4 insertions(+), 2 deletions(-) >>> >> >> Thank you for the patch - it certainly looks like a bug. Can you >> please expand the commit message a little? E.g. you have >> UCLASS_USB_EMUL below. > > I will change the description > >> >>> diff --git a/cmd/usb.c b/cmd/usb.c >>> index 992d414..81e1a7b 100644 >>> --- a/cmd/usb.c >>> +++ b/cmd/usb.c >>> @@ -415,7 +415,8 @@ static void usb_show_tree_graph(struct usb_device *dev, char *pre) >>> udev = dev_get_parent_priv(child); >>> >>> /* Ignore emulators, we only want real devices */ >>> - if (device_get_uclass_id(child) != UCLASS_USB_EMUL) { >>> + if (device_get_uclass_id(child) != >>> + (UCLASS_USB_EMUL | UCLASS_BLK)) { >> >> This seems odd to me. Do you mean to check that the child uclass is >> neither USB_EMUL nor BLK? >> >> Would it be possible to check that the parent is UCLASS_USB? That >> seems like a better condition to determine whether the child has USB >> parent data. > > It is possible to check parent uclass but would that ever fail? > I assume, block device under usb storage device will always have > parent as usb class. > Also, tree is called on only usb class devices. Maybe I am missing something. > > Regards, > Suneel > >> >>> usb_show_tree_graph(udev, pre); >>> pre[index] = 0; >>> } >>> @@ -605,7 +606,8 @@ static void usb_show_info(struct usb_device *udev) >>> for (device_find_first_child(udev->dev, &child); >>> child; >>> device_find_next_child(&child)) { >>> - if (device_active(child)) { >>> + if (device_active(child) && >>> + (device_get_uclass_id(child) != UCLASS_BLK)) { >>> udev = dev_get_parent_priv(child); >>> usb_show_info(udev); >>> } >>> -- >>> 2.7.4 >>> >> >> Regards, >> Simon
Hi Suneel, On 15 August 2017 at 11:06, Suneel Garapati <suneelglinux@gmail.com> wrote: > Hi Simon, > > > On Sun, Aug 13, 2017 at 2:37 PM, Simon Glass <sjg@chromium.org> wrote: >> Hi Suneel, >> >> On 10 August 2017 at 23:53, Suneel Garapati <suneelglinux@gmail.com> wrote: >>> usb tree and info commands may cause crash otherwise >>> >>> Signed-off-by: Suneel Garapati <suneelglinux@gmail.com> >>> --- >>> cmd/usb.c | 6 ++++-- >>> 1 file changed, 4 insertions(+), 2 deletions(-) >>> >> >> Thank you for the patch - it certainly looks like a bug. Can you >> please expand the commit message a little? E.g. you have >> UCLASS_USB_EMUL below. > > I will change the description > >> >>> diff --git a/cmd/usb.c b/cmd/usb.c >>> index 992d414..81e1a7b 100644 >>> --- a/cmd/usb.c >>> +++ b/cmd/usb.c >>> @@ -415,7 +415,8 @@ static void usb_show_tree_graph(struct usb_device *dev, char *pre) >>> udev = dev_get_parent_priv(child); >>> >>> /* Ignore emulators, we only want real devices */ >>> - if (device_get_uclass_id(child) != UCLASS_USB_EMUL) { >>> + if (device_get_uclass_id(child) != >>> + (UCLASS_USB_EMUL | UCLASS_BLK)) { >> >> This seems odd to me. Do you mean to check that the child uclass is >> neither USB_EMUL nor BLK? >> >> Would it be possible to check that the parent is UCLASS_USB? That >> seems like a better condition to determine whether the child has USB >> parent data. > > It is possible to check parent uclass but would that ever fail? How would it fail? The only device that does not have a parent is the root device, and we know it cannot be that since it will be UCLASS_ROOT. > I assume, block device under usb storage device will always have > parent as usb class. Yes that sounds right. > Also, tree is called on only usb class devices. Maybe I am missing something. Maybe, but I think it is more robust to check for the thing you want than the things you don't. If someone adds a new thing that can be a child then this code will need updating. > > Regards, > Suneel > >> >>> usb_show_tree_graph(udev, pre); >>> pre[index] = 0; >>> } >>> @@ -605,7 +606,8 @@ static void usb_show_info(struct usb_device *udev) >>> for (device_find_first_child(udev->dev, &child); >>> child; >>> device_find_next_child(&child)) { >>> - if (device_active(child)) { >>> + if (device_active(child) && >>> + (device_get_uclass_id(child) != UCLASS_BLK)) { >>> udev = dev_get_parent_priv(child); >>> usb_show_info(udev); >>> } >>> -- >>> 2.7.4 >>> Regards, Simon
Hi, On Thu, 10 Aug 2017 22:53:31 -0700 Suneel Garapati wrote: > usb tree and info commands may cause crash otherwise > > Signed-off-by: Suneel Garapati <suneelglinux@gmail.com> > --- > cmd/usb.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/cmd/usb.c b/cmd/usb.c > index 992d414..81e1a7b 100644 > --- a/cmd/usb.c > +++ b/cmd/usb.c > @@ -415,7 +415,8 @@ static void usb_show_tree_graph(struct usb_device *dev, char *pre) > udev = dev_get_parent_priv(child); > > /* Ignore emulators, we only want real devices */ > - if (device_get_uclass_id(child) != UCLASS_USB_EMUL) { > + if (device_get_uclass_id(child) != > + (UCLASS_USB_EMUL | UCLASS_BLK)) { > This should most probably be: > + if ((device_get_uclass_id(child) != UCLASS_USB_EMUL) && (device_get_uclass_id(child) != UCLASS_BLK)) { Lothar Waßmann
Hi Simon, On Thu, Aug 31, 2017 at 5:52 AM, Simon Glass <sjg@chromium.org> wrote: > Hi Suneel, > > On 15 August 2017 at 11:06, Suneel Garapati <suneelglinux@gmail.com> wrote: >> Hi Simon, >> >> >> On Sun, Aug 13, 2017 at 2:37 PM, Simon Glass <sjg@chromium.org> wrote: >>> Hi Suneel, >>> >>> On 10 August 2017 at 23:53, Suneel Garapati <suneelglinux@gmail.com> wrote: >>>> usb tree and info commands may cause crash otherwise >>>> >>>> Signed-off-by: Suneel Garapati <suneelglinux@gmail.com> >>>> --- >>>> cmd/usb.c | 6 ++++-- >>>> 1 file changed, 4 insertions(+), 2 deletions(-) >>>> >>> >>> Thank you for the patch - it certainly looks like a bug. Can you >>> please expand the commit message a little? E.g. you have >>> UCLASS_USB_EMUL below. >> >> I will change the description >> >>> >>>> diff --git a/cmd/usb.c b/cmd/usb.c >>>> index 992d414..81e1a7b 100644 >>>> --- a/cmd/usb.c >>>> +++ b/cmd/usb.c >>>> @@ -415,7 +415,8 @@ static void usb_show_tree_graph(struct usb_device *dev, char *pre) >>>> udev = dev_get_parent_priv(child); >>>> >>>> /* Ignore emulators, we only want real devices */ >>>> - if (device_get_uclass_id(child) != UCLASS_USB_EMUL) { >>>> + if (device_get_uclass_id(child) != >>>> + (UCLASS_USB_EMUL | UCLASS_BLK)) { >>> >>> This seems odd to me. Do you mean to check that the child uclass is >>> neither USB_EMUL nor BLK? >>> >>> Would it be possible to check that the parent is UCLASS_USB? That >>> seems like a better condition to determine whether the child has USB >>> parent data. >> >> It is possible to check parent uclass but would that ever fail? > > How would it fail? The only device that does not have a parent is the > root device, and we know it cannot be that since it will be > UCLASS_ROOT. > >> I assume, block device under usb storage device will always have >> parent as usb class. > > Yes that sounds right. > >> Also, tree is called on only usb class devices. Maybe I am missing something. > > Maybe, but I think it is more robust to check for the thing you want > than the things you don't. If someone adds a new thing that can be a > child then this code will need updating. I will add a check on parent uclass for USB in v1. Regards, Suneel > >> >> Regards, >> Suneel >> >>> >>>> usb_show_tree_graph(udev, pre); >>>> pre[index] = 0; >>>> } >>>> @@ -605,7 +606,8 @@ static void usb_show_info(struct usb_device *udev) >>>> for (device_find_first_child(udev->dev, &child); >>>> child; >>>> device_find_next_child(&child)) { >>>> - if (device_active(child)) { >>>> + if (device_active(child) && >>>> + (device_get_uclass_id(child) != UCLASS_BLK)) { >>>> udev = dev_get_parent_priv(child); >>>> usb_show_info(udev); >>>> } >>>> -- >>>> 2.7.4 >>>> > > Regards, > Simon
Hi, On Thu, Aug 31, 2017 at 11:30 PM, Lothar Waßmann <LW@karo-electronics.de> wrote: > Hi, > > On Thu, 10 Aug 2017 22:53:31 -0700 Suneel Garapati wrote: >> usb tree and info commands may cause crash otherwise >> >> Signed-off-by: Suneel Garapati <suneelglinux@gmail.com> >> --- >> cmd/usb.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/cmd/usb.c b/cmd/usb.c >> index 992d414..81e1a7b 100644 >> --- a/cmd/usb.c >> +++ b/cmd/usb.c >> @@ -415,7 +415,8 @@ static void usb_show_tree_graph(struct usb_device *dev, char *pre) >> udev = dev_get_parent_priv(child); >> >> /* Ignore emulators, we only want real devices */ >> - if (device_get_uclass_id(child) != UCLASS_USB_EMUL) { >> + if (device_get_uclass_id(child) != >> + (UCLASS_USB_EMUL | UCLASS_BLK)) { >> > This should most probably be: >> + if ((device_get_uclass_id(child) != UCLASS_USB_EMUL) && > (device_get_uclass_id(child) != UCLASS_BLK)) { Agree. Will make change in v1. Regards, Suneel > > > Lothar Waßmann
diff --git a/cmd/usb.c b/cmd/usb.c index 992d414..81e1a7b 100644 --- a/cmd/usb.c +++ b/cmd/usb.c @@ -415,7 +415,8 @@ static void usb_show_tree_graph(struct usb_device *dev, char *pre) udev = dev_get_parent_priv(child); /* Ignore emulators, we only want real devices */ - if (device_get_uclass_id(child) != UCLASS_USB_EMUL) { + if (device_get_uclass_id(child) != + (UCLASS_USB_EMUL | UCLASS_BLK)) { usb_show_tree_graph(udev, pre); pre[index] = 0; } @@ -605,7 +606,8 @@ static void usb_show_info(struct usb_device *udev) for (device_find_first_child(udev->dev, &child); child; device_find_next_child(&child)) { - if (device_active(child)) { + if (device_active(child) && + (device_get_uclass_id(child) != UCLASS_BLK)) { udev = dev_get_parent_priv(child); usb_show_info(udev); }
usb tree and info commands may cause crash otherwise Signed-off-by: Suneel Garapati <suneelglinux@gmail.com> --- cmd/usb.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)