[U-Boot] cmd: usb: ignore block devices under mass storage device

Submitted by Suneel Garapati on Aug. 11, 2017, 5:53 a.m.

Details

Message ID 1502430811-21475-1-git-send-email-suneelglinux@gmail.com
State New
Delegated to: Marek Vasut
Headers show

Commit Message

Suneel Garapati Aug. 11, 2017, 5:53 a.m.
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(-)

Comments

Simon Glass Aug. 13, 2017, 9:37 p.m.
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
Suneel Garapati Aug. 15, 2017, 3:06 a.m.
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

Patch hide | download patch | download mbox

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);
 		}