cmd: usb: ignore blk, emulation devices in usb tree/info display

PATCH Superseded Suneel Garapati 3 participants 11 messages Oct 19, 2017

Share & tools

Suneel Garapati

author

2017-10-19 01:22 UTC #0 lore raw

PATCH v6 1/1 cmd: usb: ignore blk, emulation devices in usb tree/info display

usb tree/info commands iterate over all usb uclass devices recursively. blk uclass child devices are created for mass storage, treating these as usb uclass devices and referencing usb config interface descriptors cause crash. To fix, ignore blk and usb_emul uclass devices(sandbox) in usb_show_info and usb_tree_graph. also avoid addition of preamble for blk uclass child devices, otherwise tree dump gets messed up.

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

Reviewed-by Bin Meng <[email protected]>

Tested-by Bin Meng <[email protected]>

Reviewed-by Simon Glass <[email protected]>

---
Changes v6:
- re-write commit message with detail
Changes v5:
- add usb_emul to ignore list in usb_show_info
- modify description
Changes v4:
- do not set preamble if child is block device for mass storage
Changes v3:
- remove 'check on parent uclass' in description
Changes v2:
- remove check on parent uclass
Changes v1:
- add separate check on blk uclass
- modify description
- add separate check on parent uclass as usb

 cmd/usb.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

cmd/usb.c +19 -3

index d95bcf5..907debe 100644
@@ -349,6 +349,16 @@ static void usb_show_tree_graph(struct usb_device *dev, char *pre)
 	printf(" %s", pre);
 #ifdef CONFIG_DM_USB
 	has_child = device_has_active_children(dev->dev);
+	if (device_get_uclass_id(dev->dev) == UCLASS_MASS_STORAGE) {
+		struct udevice *child;
+
+		for (device_find_first_child(dev->dev, &child);
+		     child;
+		     device_find_next_child(&child)) {
+			if (device_get_uclass_id(child) == UCLASS_BLK)
+				has_child = 0;
+		}
+	}
 #else
 	/* check if the device has connected children */
 	int i;

@@ -414,8 +424,12 @@ 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) {
+		/*
+		 * Ignore emulators and block child devices, we only want
+		 * real devices
+		 */
+		if ((device_get_uclass_id(child) != UCLASS_USB_EMUL) &&
+		    (device_get_uclass_id(child) != UCLASS_BLK)) {
 			usb_show_tree_graph(udev, pre);
 			pre[index] = 0;
 		}

@@ -605,7 +619,9 @@ 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_USB_EMUL) &&
+		    (device_get_uclass_id(child) != UCLASS_BLK)) {
 			udev = dev_get_parent_priv(child);
 			usb_show_info(udev);
 		}

Signature · 1 line

2.7.4

Marek Vasut

2017-10-19 01:39 UTC #1 lore raw

PATCH v6 1/1 cmd: usb: ignore blk, emulation devices in usb tree/info display

Quoted 5 lines · Suneel Garapati

usb tree/info commands iterate over all usb uclass devices recursively. blk uclass child devices are created for mass storage, treating these as usb uclass devices and referencing usb config interface descriptors cause crash. To fix, ignore blk and usb_emul uclass devices(sandbox)

^^^^^^^ what's this about ? USB_EMUL devices can be
enables elsewhere too, right ?

Anyway, shouldn't you rather filter for positive matches (usb uclass devices etc) , rather than filtering out a few negative matches (blk etc) which might break in the future ?

Quoted 3 lines

in usb_show_info and usb_tree_graph.
also avoid addition of preamble for blk uclass child devices, otherwise tree dump gets messed up.

Also, sentences start with capital letter. This should be in a separate patch if it's a separate change ...

Quoted 72 lines

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

Reviewed-by Bin Meng <[email protected]>

Tested-by Bin Meng <[email protected]>

Reviewed-by Simon Glass <[email protected]>

 cmd/usb.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

cmd/usb.c +19 -3

index d95bcf5..907debe 100644
@@ -349,6 +349,16 @@ static void usb_show_tree_graph(struct usb_device *dev, char *pre)
 	printf(" %s", pre);
 #ifdef CONFIG_DM_USB
 	has_child = device_has_active_children(dev->dev);
+	if (device_get_uclass_id(dev->dev) == UCLASS_MASS_STORAGE) {
+		struct udevice *child;
+
+		for (device_find_first_child(dev->dev, &child);
+		     child;
+		     device_find_next_child(&child)) {
+			if (device_get_uclass_id(child) == UCLASS_BLK)
+				has_child = 0;
+		}
+	}
 #else
 	/* check if the device has connected children */
 	int i;

@@ -414,8 +424,12 @@ 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) {
+		/*
+		 * Ignore emulators and block child devices, we only want
+		 * real devices
+		 */
+		if ((device_get_uclass_id(child) != UCLASS_USB_EMUL) &&
+		    (device_get_uclass_id(child) != UCLASS_BLK)) {
 			usb_show_tree_graph(udev, pre);
 			pre[index] = 0;
 		}

@@ -605,7 +619,9 @@ 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_USB_EMUL) &&
+		    (device_get_uclass_id(child) != UCLASS_BLK)) {
 			udev = dev_get_parent_priv(child);
 			usb_show_info(udev);
 		}

Signature · 2 lines

Best regards,
Marek Vasut

Suneel Garapati

author

2017-10-19 03:24 UTC #2 lore raw

PATCH v6 1/1 cmd: usb: ignore blk, emulation devices in usb tree/info display

Quoted 8 lines · Marek Vasut

Quoted 5 lines · Suneel Garapati
usb tree/info commands iterate over all usb uclass devices recursively. blk uclass child devices are created for mass storage, treating these as usb uclass devices and referencing usb config interface descriptors cause crash. To fix, ignore blk and usb_emul uclass devices(sandbox)
^^^^^^^ what's this about ? USB_EMUL devices can be
enables elsewhere too, right ?

Only disabled during the tree/info dump.

Quoted 4 lines

Anyway, shouldn't you rather filter for positive matches (usb uclass devices etc) , rather than filtering out a few negative matches (blk etc) which might break in the future ?

usb_for_each_root_dev does that but we dont have uclass_find_first_child_device to call on UCLASS_USB like uclass_find_first_device.
So, device_find_first_child and check on uclass id is performed.

Quoted 7 lines

Quoted 4 lines
in usb_show_info and usb_tree_graph.
also avoid addition of preamble for blk uclass child devices, otherwise tree dump gets messed up.
Also, sentences start with capital letter. This should be in a separate patch if it's a separate change ...

Ignoring preamble and device should go together, hence cannot be separate change.

Regards,
Suneel

Quoted 78 lines

Quoted 63 lines

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

Reviewed-by Bin Meng <[email protected]>

Tested-by Bin Meng <[email protected]>

Reviewed-by Simon Glass <[email protected]>

 cmd/usb.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

cmd/usb.c +16 -2

index d95bcf5..907debe 100644
@@ -349,6 +349,16 @@ static void usb_show_tree_graph(struct usb_device *dev, char *pre)
      printf(" %s", pre);
 #ifdef CONFIG_DM_USB
      has_child = device_has_active_children(dev->dev);
+     if (device_get_uclass_id(dev->dev) == UCLASS_MASS_STORAGE) {
+             struct udevice *child;
+
+             for (device_find_first_child(dev->dev, &child);
+                  child;
+                  device_find_next_child(&child)) {
+                     if (device_get_uclass_id(child) == UCLASS_BLK)
+                             has_child = 0;
+             }
+     }
 #else
      /* check if the device has connected children */
      int i;

@@ -414,8 +424,12 @@ 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) {
+             /*
+              * Ignore emulators and block child devices, we only want
+              * real devices
+              */
+             if ((device_get_uclass_id(child) != UCLASS_USB_EMUL) &&
+                 (device_get_uclass_id(child) != UCLASS_BLK)) {
                      usb_show_tree_graph(udev, pre);
                      pre[index] = 0;
              }

cmd/usb.c static void usb_show_info(struct usb_device *udev) · line 627

Quoted 10 lines

cmd/usb.c +3 -1

@@ -605,7 +619,9 @@ 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_USB_EMUL) &&
+                 (device_get_uclass_id(child) != UCLASS_BLK)) {
                      udev = dev_get_parent_priv(child);
                      usb_show_info(udev);
              }

--
Best regards,
Marek Vasut

Marek Vasut

2017-10-19 07:33 UTC #3 lore raw

PATCH v6 1/1 cmd: usb: ignore blk, emulation devices in usb tree/info display

Quoted 10 lines · Suneel Garapati

Quoted 8 lines · Marek Vasut
Quoted 5 lines · Suneel Garapati
usb tree/info commands iterate over all usb uclass devices recursively. blk uclass child devices are created for mass storage, treating these as usb uclass devices and referencing usb config interface descriptors cause crash. To fix, ignore blk and usb_emul uclass devices(sandbox)
^^^^^^^ what's this about ? USB_EMUL devices can be
enables elsewhere too, right ?
Only disabled during the tree/info dump.

I don't understand this answer. Can USB_EMUL devices be enabled on any other machine than sandbox or not ? I presume it can ...

Quoted 6 lines

Quoted 3 lines
Anyway, shouldn't you rather filter for positive matches (usb uclass devices etc) , rather than filtering out a few negative matches (blk etc) which might break in the future ?
usb_for_each_root_dev does that but we dont have uclass_find_first_child_device to call on UCLASS_USB like uclass_find_first_device.
So, device_find_first_child and check on uclass id is performed.

I mean, rather than doing
(device_get_uclass_id(child) != UCLASS_USB_xxx &&
device_get_uclass_id(child) != UCLASS_USB_yyy)
dump

(device_get_uclass_id(child) == UCLASS_USB_nnn)
dump

for nnn being only the relevant USB classes for which we actually want to dump.

Does that work ?

Quoted 89 lines

Quoted 6 lines

Quoted 3 lines
in usb_show_info and usb_tree_graph.
also avoid addition of preamble for blk uclass child devices, otherwise tree dump gets messed up.
Also, sentences start with capital letter. This should be in a separate patch if it's a separate change ...

Ignoring preamble and device should go together, hence cannot be separate change.

Regards,
Suneel

Quoted 78 lines

Quoted 63 lines

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

Reviewed-by Bin Meng <[email protected]>

Tested-by Bin Meng <[email protected]>

Reviewed-by Simon Glass <[email protected]>

 cmd/usb.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

cmd/usb.c +16 -2

index d95bcf5..907debe 100644
@@ -349,6 +349,16 @@ static void usb_show_tree_graph(struct usb_device *dev, char *pre)
      printf(" %s", pre);
 #ifdef CONFIG_DM_USB
      has_child = device_has_active_children(dev->dev);
+     if (device_get_uclass_id(dev->dev) == UCLASS_MASS_STORAGE) {
+             struct udevice *child;
+
+             for (device_find_first_child(dev->dev, &child);
+                  child;
+                  device_find_next_child(&child)) {
+                     if (device_get_uclass_id(child) == UCLASS_BLK)
+                             has_child = 0;
+             }
+     }
 #else
      /* check if the device has connected children */
      int i;

@@ -414,8 +424,12 @@ 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) {
+             /*
+              * Ignore emulators and block child devices, we only want
+              * real devices
+              */
+             if ((device_get_uclass_id(child) != UCLASS_USB_EMUL) &&
+                 (device_get_uclass_id(child) != UCLASS_BLK)) {
                      usb_show_tree_graph(udev, pre);
                      pre[index] = 0;
              }

cmd/usb.c static void usb_show_info(struct usb_device *udev) · line 627

Quoted 10 lines

cmd/usb.c +3 -1

@@ -605,7 +619,9 @@ 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_USB_EMUL) &&
+                 (device_get_uclass_id(child) != UCLASS_BLK)) {
                      udev = dev_get_parent_priv(child);
                      usb_show_info(udev);
              }

--
Best regards,
Marek Vasut

Signature · 2 lines

Best regards,
Marek Vasut

Suneel Garapati

author

2017-10-19 08:03 UTC #4 lore raw

PATCH v6 1/1 cmd: usb: ignore blk, emulation devices in usb tree/info display

Quoted 8 lines · Marek Vasut

Quoted 7 lines · Suneel Garapati
Quoted 6 lines · Marek Vasut
Quoted 5 lines · Suneel Garapati
usb tree/info commands iterate over all usb uclass devices recursively. blk uclass child devices are created for mass storage, treating these as usb uclass devices and referencing usb config interface descriptors cause crash. To fix, ignore blk and usb_emul uclass devices(sandbox)

Quoted 6 lines

Quoted 3 lines
Quoted 2 lines
^^^^^^^ what's this about ? USB_EMUL devices can be
enables elsewhere too, right ?
Only disabled during the tree/info dump.
I don't understand this answer. Can USB_EMUL devices be enabled on any other machine than sandbox or not ? I presume it can ...

drivers/usb/emul/Kconfig - usb_emul depends on sandbox.
I dont see any machine config using it.
I believe USB_EMUL devices were being ignored before this patch, not sure if the expectation has changed now.
Anyway, with the change to call only usb_xxx classes this should go away in description.

Quoted 15 lines

Quoted 7 lines
Quoted 4 lines
Anyway, shouldn't you rather filter for positive matches (usb uclass devices etc) , rather than filtering out a few negative matches (blk etc) which might break in the future ?
usb_for_each_root_dev does that but we dont have uclass_find_first_child_device to call on UCLASS_USB like uclass_find_first_device.
So, device_find_first_child and check on uclass id is performed.
I mean, rather than doing
(device_get_uclass_id(child) != UCLASS_USB_xxx &&
device_get_uclass_id(child) != UCLASS_USB_yyy)
dump
do

Quoted 7 lines

(device_get_uclass_id(child) == UCLASS_USB_nnn)
dump
for nnn being only the relevant USB classes for which we actually want to dump.
Does that work ?

May work, I will test and send v7

Regards,
Suneel

Quoted 95 lines

Quoted 5 lines

Quoted 5 lines
Quoted 4 lines
in usb_show_info and usb_tree_graph.
also avoid addition of preamble for blk uclass child devices, otherwise tree dump gets messed up.

Quoted 85 lines

Quoted 2 lines

Also, sentences start with capital letter. This should be in a separate patch if it's a separate change ...

Ignoring preamble and device should go together, hence cannot be separate change.

Regards,
Suneel

Quoted 78 lines

Quoted 63 lines

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

Reviewed-by Bin Meng <[email protected]>

Tested-by Bin Meng <[email protected]>

Reviewed-by Simon Glass <[email protected]>

 cmd/usb.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

cmd/usb.c +16 -2

index d95bcf5..907debe 100644
@@ -349,6 +349,16 @@ static void usb_show_tree_graph(struct usb_device *dev, char *pre)
      printf(" %s", pre);
 #ifdef CONFIG_DM_USB
      has_child = device_has_active_children(dev->dev);
+     if (device_get_uclass_id(dev->dev) == UCLASS_MASS_STORAGE) {
+             struct udevice *child;
+
+             for (device_find_first_child(dev->dev, &child);
+                  child;
+                  device_find_next_child(&child)) {
+                     if (device_get_uclass_id(child) == UCLASS_BLK)
+                             has_child = 0;
+             }
+     }
 #else
      /* check if the device has connected children */
      int i;

@@ -414,8 +424,12 @@ 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) {
+             /*
+              * Ignore emulators and block child devices, we only want
+              * real devices
+              */
+             if ((device_get_uclass_id(child) != UCLASS_USB_EMUL) &&
+                 (device_get_uclass_id(child) != UCLASS_BLK)) {
                      usb_show_tree_graph(udev, pre);
                      pre[index] = 0;
              }

cmd/usb.c static void usb_show_info(struct usb_device *udev) · line 627

Quoted 10 lines

cmd/usb.c +3 -1

@@ -605,7 +619,9 @@ 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_USB_EMUL) &&
+                 (device_get_uclass_id(child) != UCLASS_BLK)) {
                      udev = dev_get_parent_priv(child);
                      usb_show_info(udev);
              }

--
Best regards,
Marek Vasut

Bin Meng

2017-10-19 08:37 UTC #5 lore raw

PATCH v6 1/1 cmd: usb: ignore blk, emulation devices in usb tree/info display

In reply to Marek Vasut

Hi Marek,

Quoted 8 lines · Marek Vasut

Quoted 7 lines · Suneel Garapati
Quoted 6 lines · Marek Vasut
Quoted 5 lines · Suneel Garapati
usb tree/info commands iterate over all usb uclass devices recursively. blk uclass child devices are created for mass storage, treating these as usb uclass devices and referencing usb config interface descriptors cause crash. To fix, ignore blk and usb_emul uclass devices(sandbox)

Quoted 7 lines

Quoted 3 lines
Quoted 2 lines
^^^^^^^ what's this about ? USB_EMUL devices can be
enables elsewhere too, right ?
Only disabled during the tree/info dump.
I don't understand this answer. Can USB_EMUL devices be enabled on any other machine than sandbox or not ? I presume it can ...

No, it cannot.

Quoted 14 lines

Quoted 6 lines
Quoted 3 lines
Anyway, shouldn't you rather filter for positive matches (usb uclass devices etc) , rather than filtering out a few negative matches (blk etc) which might break in the future ?
usb_for_each_root_dev does that but we dont have uclass_find_first_child_device to call on UCLASS_USB like uclass_find_first_device.
So, device_find_first_child and check on uclass id is performed.
I mean, rather than doing
(device_get_uclass_id(child) != UCLASS_USB_xxx &&
device_get_uclass_id(child) != UCLASS_USB_yyy)
dump
do

Quoted 8 lines

(device_get_uclass_id(child) == UCLASS_USB_nnn)
dump
for nnn being only the relevant USB classes for which we actually want to dump.
Does that work ?

No, I don't think you can enumerate all USB devices here. It can be any uclass device.

Quoted 9 lines

Quoted 9 lines
Quoted 6 lines
Quoted 3 lines
in usb_show_info and usb_tree_graph.
also avoid addition of preamble for blk uclass child devices, otherwise tree dump gets messed up.
Also, sentences start with capital letter. This should be in a separate patch if it's a separate change ...
Ignoring preamble and device should go together, hence cannot be separate change.

[snip]

Regards,
Bin

Marek Vasut

2017-10-19 08:43 UTC #6 lore raw

PATCH v6 1/1 cmd: usb: ignore blk, emulation devices in usb tree/info display

Quoted 1 line · Bin Meng

Hi Marek,

Hi,

Quoted 11 lines · Marek Vasut

Quoted 10 lines · Suneel Garapati
Quoted 9 lines · Marek Vasut
Quoted 8 lines · Suneel Garapati
Quoted 5 lines
usb tree/info commands iterate over all usb uclass devices recursively. blk uclass child devices are created for mass storage, treating these as usb uclass devices and referencing usb config interface descriptors cause crash. To fix, ignore blk and usb_emul uclass devices(sandbox)
^^^^^^^ what's this about ? USB_EMUL devices can be
enables elsewhere too, right ?

Quoted 7 lines

Quoted 5 lines
Quoted 1 line
Only disabled during the tree/info dump.
I don't understand this answer. Can USB_EMUL devices be enabled on any other machine than sandbox or not ? I presume it can ...
No, it cannot.

Why ? Because of the Kconfig thing ? That can easily change and then this breaks ...

Quoted 17 lines

Quoted 17 lines
Quoted 6 lines
Quoted 3 lines
Anyway, shouldn't you rather filter for positive matches (usb uclass devices etc) , rather than filtering out a few negative matches (blk etc) which might break in the future ?
usb_for_each_root_dev does that but we dont have uclass_find_first_child_device to call on UCLASS_USB like uclass_find_first_device.
So, device_find_first_child and check on uclass id is performed.
I mean, rather than doing
(device_get_uclass_id(child) != UCLASS_USB_xxx &&
device_get_uclass_id(child) != UCLASS_USB_yyy)
dump
do
(device_get_uclass_id(child) == UCLASS_USB_nnn)
dump

Quoted 8 lines

Quoted 5 lines
for nnn being only the relevant USB classes for which we actually want to dump.
Does that work ?
No, I don't think you can enumerate all USB devices here. It can be any uclass device.

And only the blk one breaks things ?

Quoted 15 lines

Quoted 9 lines
Quoted 9 lines
Quoted 6 lines
Quoted 3 lines
in usb_show_info and usb_tree_graph.
also avoid addition of preamble for blk uclass child devices, otherwise tree dump gets messed up.
Also, sentences start with capital letter. This should be in a separate patch if it's a separate change ...
Ignoring preamble and device should go together, hence cannot be separate change.
[snip]
Regards,
Bin

Signature · 2 lines

Best regards,
Marek Vasut

Bin Meng

2017-10-19 08:56 UTC #7 lore raw

PATCH v6 1/1 cmd: usb: ignore blk, emulation devices in usb tree/info display

Hi Marek,

Quoted 18 lines · Marek Vasut

Quoted 1 line · Bin Meng
Hi Marek,
Hi,
Quoted 13 lines · Marek Vasut
Quoted 12 lines · Suneel Garapati
Quoted 10 lines · Marek Vasut
Quoted 8 lines · Suneel Garapati
Quoted 5 lines
usb tree/info commands iterate over all usb uclass devices recursively. blk uclass child devices are created for mass storage, treating these as usb uclass devices and referencing usb config interface descriptors cause crash. To fix, ignore blk and usb_emul uclass devices(sandbox)
^^^^^^^ what's this about ? USB_EMUL devices can be
enables elsewhere too, right ?
Only disabled during the tree/info dump.

Quoted 8 lines

Quoted 5 lines
Quoted 3 lines
I don't understand this answer. Can USB_EMUL devices be enabled on any other machine than sandbox or not ? I presume it can ...
No, it cannot.
Why ? Because of the Kconfig thing ? That can easily change and then this breaks ...

Yes, it's currently on on Sandbox. But whether it's on Sandbox or not does not matter. These devices are should be filtered out as they are not supposed to be on the USB topology.

Quoted 21 lines

Quoted 21 lines
Quoted 21 lines
Quoted 7 lines
Quoted 4 lines
Anyway, shouldn't you rather filter for positive matches (usb uclass devices etc) , rather than filtering out a few negative matches (blk etc) which might break in the future ?
usb_for_each_root_dev does that but we dont have uclass_find_first_child_device to call on UCLASS_USB like uclass_find_first_device.
So, device_find_first_child and check on uclass id is performed.
I mean, rather than doing
(device_get_uclass_id(child) != UCLASS_USB_xxx &&
device_get_uclass_id(child) != UCLASS_USB_yyy)
dump
do
(device_get_uclass_id(child) == UCLASS_USB_nnn)
dump
for nnn being only the relevant USB classes for which we actually want to dump.

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
Does that work ?
No, I don't think you can enumerate all USB devices here. It can be any uclass device.
And only the blk one breaks things ?

Yes, blk devices are not "struct usb_device" declared, as they are not USB devices. However their parent is.

Quoted 10 lines

Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 7 lines
Quoted 4 lines
in usb_show_info and usb_tree_graph.
also avoid addition of preamble for blk uclass child devices, otherwise tree dump gets messed up.
Also, sentences start with capital letter. This should be in a separate patch if it's a separate change ...
Ignoring preamble and device should go together, hence cannot be separate change.

Regards,
Bin

Marek Vasut

2017-10-19 09:47 UTC #8 lore raw

PATCH v6 1/1 cmd: usb: ignore blk, emulation devices in usb tree/info display

Quoted 25 lines · Bin Meng

Hi Marek,
Quoted 22 lines · Marek Vasut
Quoted 1 line · Bin Meng
Hi Marek,
Hi,
Quoted 17 lines · Marek Vasut
Quoted 15 lines · Suneel Garapati
Quoted 10 lines · Marek Vasut
Quoted 8 lines · Suneel Garapati
Quoted 5 lines
usb tree/info commands iterate over all usb uclass devices recursively. blk uclass child devices are created for mass storage, treating these as usb uclass devices and referencing usb config interface descriptors cause crash. To fix, ignore blk and usb_emul uclass devices(sandbox)
^^^^^^^ what's this about ? USB_EMUL devices can be
enables elsewhere too, right ?
Only disabled during the tree/info dump.
I don't understand this answer. Can USB_EMUL devices be enabled on any other machine than sandbox or not ? I presume it can ...

Quoted 8 lines

Quoted 4 lines
Quoted 1 line
No, it cannot.
Why ? Because of the Kconfig thing ? That can easily change and then this breaks ...
Yes, it's currently on on Sandbox. But whether it's on Sandbox or not does not matter. These devices are should be filtered out as they are not supposed to be on the USB topology.

I agree with that, I was just commenting on this "(sandbox)" in the description, which IMO is not precise.

Quoted 23 lines

Quoted 23 lines
Quoted 23 lines
Quoted 22 lines
Quoted 6 lines
Quoted 3 lines
Anyway, shouldn't you rather filter for positive matches (usb uclass devices etc) , rather than filtering out a few negative matches (blk etc) which might break in the future ?
usb_for_each_root_dev does that but we dont have uclass_find_first_child_device to call on UCLASS_USB like uclass_find_first_device.
So, device_find_first_child and check on uclass id is performed.
I mean, rather than doing
(device_get_uclass_id(child) != UCLASS_USB_xxx &&
device_get_uclass_id(child) != UCLASS_USB_yyy)
dump
do
(device_get_uclass_id(child) == UCLASS_USB_nnn)
dump
for nnn being only the relevant USB classes for which we actually want to dump.
Does that work ?

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
No, I don't think you can enumerate all USB devices here. It can be any uclass device.
And only the blk one breaks things ?
Yes, blk devices are not "struct usb_device" declared, as they are not USB devices. However their parent is.

In which case, _that_ should be in the commit message.

Anyway, is there any chance something else will come in, ie. net device for USB ethernet devices ?

Signature · 2 lines

Best regards,
Marek Vasut

Bin Meng

2017-10-19 09:52 UTC #9 lore raw

PATCH v6 1/1 cmd: usb: ignore blk, emulation devices in usb tree/info display

Hi Marek,

Quoted 66 lines · Marek Vasut

Quoted 25 lines · Bin Meng
Hi Marek,
Quoted 22 lines · Marek Vasut
Quoted 1 line · Bin Meng
Hi Marek,
Hi,
Quoted 17 lines · Marek Vasut
Quoted 15 lines · Suneel Garapati
Quoted 10 lines · Marek Vasut
Quoted 8 lines · Suneel Garapati
Quoted 5 lines
usb tree/info commands iterate over all usb uclass devices recursively. blk uclass child devices are created for mass storage, treating these as usb uclass devices and referencing usb config interface descriptors cause crash. To fix, ignore blk and usb_emul uclass devices(sandbox)
^^^^^^^ what's this about ? USB_EMUL devices can be
enables elsewhere too, right ?
Only disabled during the tree/info dump.
I don't understand this answer. Can USB_EMUL devices be enabled on any other machine than sandbox or not ? I presume it can ...
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
No, it cannot.
Why ? Because of the Kconfig thing ? That can easily change and then this breaks ...
Yes, it's currently on on Sandbox. But whether it's on Sandbox or not does not matter. These devices are should be filtered out as they are not supposed to be on the USB topology.
I agree with that, I was just commenting on this "(sandbox)" in the description, which IMO is not precise.
Quoted 28 lines
Quoted 27 lines
Quoted 17 lines
Quoted 17 lines
Quoted 6 lines
Quoted 3 lines
Anyway, shouldn't you rather filter for positive matches (usb uclass devices etc) , rather than filtering out a few negative matches (blk etc) which might break in the future ?
usb_for_each_root_dev does that but we dont have uclass_find_first_child_device to call on UCLASS_USB like uclass_find_first_device.
So, device_find_first_child and check on uclass id is performed.
I mean, rather than doing
(device_get_uclass_id(child) != UCLASS_USB_xxx &&
device_get_uclass_id(child) != UCLASS_USB_yyy)
dump
do
(device_get_uclass_id(child) == UCLASS_USB_nnn)
dump
Quoted 8 lines
Quoted 5 lines
for nnn being only the relevant USB classes for which we actually want to dump.
Does that work ?
No, I don't think you can enumerate all USB devices here. It can be any uclass device.
And only the blk one breaks things ?

Quoted 8 lines

Quoted 2 lines
Yes, blk devices are not "struct usb_device" declared, as they are not USB devices. However their parent is.
In which case, _that_ should be in the commit message.
Anyway, is there any chance something else will come in, ie. net device for USB ethernet devices ?

No problem with any USB devices that end up in the leaf node in the DM tree.

Regards,
Bin

Marek Vasut

2017-10-19 10:41 UTC #10 lore raw

PATCH v6 1/1 cmd: usb: ignore blk, emulation devices in usb tree/info display

Quoted 72 lines · Bin Meng

Hi Marek,
Quoted 69 lines · Marek Vasut
Quoted 25 lines · Bin Meng
Hi Marek,
Quoted 22 lines · Marek Vasut
Quoted 1 line · Bin Meng
Hi Marek,
Hi,
Quoted 17 lines · Marek Vasut
Quoted 15 lines · Suneel Garapati
Quoted 10 lines · Marek Vasut
Quoted 8 lines · Suneel Garapati
Quoted 5 lines
usb tree/info commands iterate over all usb uclass devices recursively. blk uclass child devices are created for mass storage, treating these as usb uclass devices and referencing usb config interface descriptors cause crash. To fix, ignore blk and usb_emul uclass devices(sandbox)
^^^^^^^ what's this about ? USB_EMUL devices can be
enables elsewhere too, right ?
Only disabled during the tree/info dump.
I don't understand this answer. Can USB_EMUL devices be enabled on any other machine than sandbox or not ? I presume it can ...
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
No, it cannot.
Why ? Because of the Kconfig thing ? That can easily change and then this breaks ...
Yes, it's currently on on Sandbox. But whether it's on Sandbox or not does not matter. These devices are should be filtered out as they are not supposed to be on the USB topology.
I agree with that, I was just commenting on this "(sandbox)" in the description, which IMO is not precise.
Quoted 30 lines
Quoted 20 lines
Quoted 20 lines
Quoted 20 lines
Quoted 6 lines
Quoted 3 lines
Anyway, shouldn't you rather filter for positive matches (usb uclass devices etc) , rather than filtering out a few negative matches (blk etc) which might break in the future ?
usb_for_each_root_dev does that but we dont have uclass_find_first_child_device to call on UCLASS_USB like uclass_find_first_device.
So, device_find_first_child and check on uclass id is performed.
I mean, rather than doing
(device_get_uclass_id(child) != UCLASS_USB_xxx &&
device_get_uclass_id(child) != UCLASS_USB_yyy)
dump
do
(device_get_uclass_id(child) == UCLASS_USB_nnn)
dump
for nnn being only the relevant USB classes for which we actually want to dump.
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
Does that work ?
No, I don't think you can enumerate all USB devices here. It can be any uclass device.
And only the blk one breaks things ?
Yes, blk devices are not "struct usb_device" declared, as they are not USB devices. However their parent is.

Quoted 7 lines

Quoted 5 lines
In which case, _that_ should be in the commit message.
Anyway, is there any chance something else will come in, ie. net device for USB ethernet devices ?
No problem with any USB devices that end up in the leaf node in the DM tree.

So what triggers this issue are partitions and co. then ?

Quoted 3 lines

Regards,
Bin

Signature · 2 lines

Best regards,
Marek Vasut