cmd: usb: ignore block devices under mass storage device

PATCH New Suneel Garapati 3 participants 8 messages Aug 11, 2017 – Sep 1, 2017

lore

Share & tools

lore

Suneel Garapati

author

2017-08-11 05:53 UTC #0 lore raw

PATCH 1/1 cmd: usb: ignore block devices under mass storage device

usb tree and info commands may cause crash otherwise

Signed-off-by Suneel Garapati <[email protected]>

---
 cmd/usb.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

cmd/usb.c +4 -2

index 992d414..81e1a7b 100644
@@ -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);
 		}

Signature · 1 line

2.7.4

Suneel Garapati

author

2017-08-15 03:06 UTC #2 lore raw

PATCH 1/1 cmd: usb: ignore block devices under mass storage device

Hi Simon,

Quoted 11 lines · Simon Glass

Hi Suneel,
Quoted 7 lines · Suneel Garapati
usb tree and info commands may cause crash otherwise
Signed-off-by Suneel Garapati <[email protected]>
---
 cmd/usb.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Quoted 3 lines

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

Quoted 16 lines

cmd/usb.c static void usb_show_tree_graph(struct usb_device *dev, char *pre) · line 419

Quoted 12 lines

cmd/usb.c +2 -1

index 992d414..81e1a7b 100644
@@ -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?

Quoted 3 lines

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

Quoted 20 lines

Quoted 15 lines

cmd/usb.c +2 -2

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

Quoted 2 lines

2.7.4

Regards,
Simon

Suneel Garapati

author

2017-08-28 18:37 UTC #3 lore raw

PATCH 1/1 cmd: usb: ignore block devices under mass storage device

Hi,

Request for review on comments below.

Regards,
Suneel

Quoted 69 lines · Suneel Garapati

Hi Simon,
Quoted 11 lines · Simon Glass
Hi Suneel,
Quoted 7 lines · Suneel Garapati
usb tree and info commands may cause crash otherwise
Signed-off-by Suneel Garapati <[email protected]>
---
 cmd/usb.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
Quoted 3 lines
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
Quoted 16 lines
cmd/usb.c static void usb_show_tree_graph(struct usb_device *dev, char *pre) · line 419
Quoted 12 lines
cmd/usb.c +2 -1
index 992d414..81e1a7b 100644
@@ -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?
Quoted 3 lines
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
Quoted 20 lines
Quoted 15 lines
cmd/usb.c +2 -2
                        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);
                }
--
Quoted 2 lines
2.7.4
Regards,
Simon

Simon Glass

2017-08-31 12:52 UTC #4 lore raw

PATCH 1/1 cmd: usb: ignore block devices under mass storage device

In reply to Suneel Garapati

Hi Suneel,

Quoted 37 lines · Suneel Garapati

Hi Simon,
Quoted 11 lines · Simon Glass
Hi Suneel,
Quoted 7 lines · Suneel Garapati
usb tree and info commands may cause crash otherwise
Signed-off-by Suneel Garapati <[email protected]>
---
 cmd/usb.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
Quoted 3 lines
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
Quoted 16 lines
cmd/usb.c static void usb_show_tree_graph(struct usb_device *dev, char *pre) · line 419
Quoted 12 lines
cmd/usb.c +2 -1
index 992d414..81e1a7b 100644
@@ -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?

Quoted 5 lines

Quoted 3 lines
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.

Quoted 2 lines

I assume, block device under usb storage device will always have parent as usb class.

Yes that sounds right.

Quoted 1 line

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.

Quoted 5 lines

Regards,
Suneel
Quoted 1 line

Quoted 16 lines

Quoted 8 lines

cmd/usb.c +0 -1

                        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)) {

Quoted 8 lines

cmd/usb.c +2 -1

+               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

author

2017-09-01 07:34 UTC #5 lore raw

PATCH 1/1 cmd: usb: ignore block devices under mass storage device

Hi Simon,

Quoted 53 lines · Simon Glass

Hi Suneel,
Quoted 37 lines · Suneel Garapati
Hi Simon,
Quoted 11 lines · Simon Glass
Hi Suneel,
Quoted 7 lines · Suneel Garapati
usb tree and info commands may cause crash otherwise
Signed-off-by Suneel Garapati <[email protected]>
---
 cmd/usb.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
Quoted 3 lines
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
Quoted 16 lines
cmd/usb.c static void usb_show_tree_graph(struct usb_device *dev, char *pre) · line 419
Quoted 12 lines
cmd/usb.c +2 -1
index 992d414..81e1a7b 100644
@@ -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?
Quoted 5 lines
Quoted 3 lines
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.
Quoted 2 lines
I assume, block device under usb storage device will always have parent as usb class.

Quoted 7 lines

Yes that sounds right.
Quoted 1 line
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

Quoted 25 lines

Quoted 6 lines

Regards,
Suneel
Quoted 1 line

Quoted 16 lines

Quoted 8 lines

cmd/usb.c +0 -1

                        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)) {

Quoted 8 lines

cmd/usb.c +2 -1

+               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

Lothar Waßmann

2017-09-01 06:30 UTC #6 lore raw

PATCH 1/1 cmd: usb: ignore block devices under mass storage device

In reply to Suneel Garapati

Hi,

Quoted 12 lines · 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 <[email protected]>
---
 cmd/usb.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
cmd/usb.c +0 -0
index 992d414..81e1a7b 100644

cmd/usb.c static void usb_show_tree_graph(struct usb_device *dev, char *pre) · line 420

Quoted 7 lines

cmd/usb.c +2 -1

@@ -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:

cmd/usb.c static void usb_show_tree_graph(struct usb_device *dev, char *pre) · line 421

Quoted 1 line

cmd/usb.c +1 -0

+		if ((device_get_uclass_id(child) != UCLASS_USB_EMUL) &&

			(device_get_uclass_id(child) != UCLASS_BLK)) {

Lothar WaÃŸmann

Suneel Garapati

author

2017-09-01 07:35 UTC #7 lore raw

PATCH 1/1 cmd: usb: ignore block devices under mass storage device

Hi,

Quoted 17 lines · Lothar Waßmann

Hi,
Quoted 14 lines · 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 <[email protected]>
---
 cmd/usb.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
cmd/usb.c +0 -0
index 992d414..81e1a7b 100644
@@ -415,7 +415,8 @@ static void usb_show_tree_graph(struct usb_device *dev, char *pre)
              udev = dev_get_parent_priv(child);
 

Quoted 8 lines

cmd/usb.c static void usb_show_tree_graph(struct usb_device *dev, char *pre)
Quoted 5 lines
cmd/usb.c +2 -1
              /* 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:
cmd/usb.c static void usb_show_tree_graph(struct usb_device *dev, char *pre) · line 421
Quoted 1 line
cmd/usb.c +1 -0
+             if ((device_get_uclass_id(child) != UCLASS_USB_EMUL) &&
                        (device_get_uclass_id(child) != UCLASS_BLK)) {

Agree. Will make change in v1.

Regards,
Suneel

Quoted 3 lines

Lothar Waßmann