Re: [U-Boot] [UBOOT] [PATCH] cmd: usb: ignore block devices under mass storage device
Hi, On Thu, Aug 31, 2017 at 11:30 PM, Lothar Waßmann 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 >> --- >> 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 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [UBOOT] [PATCH] cmd: usb: ignore block devices under mass storage device
Hi Simon, On Thu, Aug 31, 2017 at 5:52 AM, Simon Glass wrote: > Hi Suneel, > > On 15 August 2017 at 11:06, Suneel Garapati wrote: >> Hi Simon, >> >> >> On Sun, Aug 13, 2017 at 2:37 PM, Simon Glass wrote: >>> Hi Suneel, >>> >>> On 10 August 2017 at 23:53, Suneel Garapati wrote: usb tree and info commands may cause crash otherwise Signed-off-by: Suneel Garapati --- 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 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [UBOOT] [PATCH] cmd: usb: ignore block devices under mass storage device
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 > --- > 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 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [UBOOT] [PATCH] cmd: usb: ignore block devices under mass storage device
Hi Suneel, On 15 August 2017 at 11:06, Suneel Garapati wrote: > Hi Simon, > > > On Sun, Aug 13, 2017 at 2:37 PM, Simon Glass wrote: >> Hi Suneel, >> >> On 10 August 2017 at 23:53, Suneel Garapati wrote: >>> usb tree and info commands may cause crash otherwise >>> >>> Signed-off-by: Suneel Garapati >>> --- >>> 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 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [UBOOT] [PATCH] cmd: usb: ignore block devices under mass storage device
Hi, Request for review on comments below. Regards, Suneel On Mon, Aug 14, 2017 at 8:06 PM, Suneel Garapati wrote: > Hi Simon, > > > On Sun, Aug 13, 2017 at 2:37 PM, Simon Glass wrote: >> Hi Suneel, >> >> On 10 August 2017 at 23:53, Suneel Garapati wrote: >>> usb tree and info commands may cause crash otherwise >>> >>> Signed-off-by: Suneel Garapati >>> --- >>> 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 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [UBOOT] [PATCH] cmd: usb: ignore block devices under mass storage device
Hi Simon, On Sun, Aug 13, 2017 at 2:37 PM, Simon Glass wrote: > Hi Suneel, > > On 10 August 2017 at 23:53, Suneel Garapati wrote: >> usb tree and info commands may cause crash otherwise >> >> Signed-off-by: Suneel Garapati >> --- >> 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 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [UBOOT] [PATCH] cmd: usb: ignore block devices under mass storage device
Hi Suneel, On 10 August 2017 at 23:53, Suneel Garapati wrote: > usb tree and info commands may cause crash otherwise > > Signed-off-by: Suneel Garapati > --- > 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 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] [UBOOT] [PATCH] cmd: usb: ignore block devices under mass storage device
usb tree and info commands may cause crash otherwise Signed-off-by: Suneel Garapati --- 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)) { 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 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot