On Wed, Oct 18, 2017 at 6:39 PM, Marek Vasut marex@denx.de wrote:

On 10/19/2017 03:22 AM, Suneel Garapati wrote:

...

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.

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.

...

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

...

Signed-off-by: Suneel Garapati suneelglinux@gmail.com Reviewed-by: Bin Meng bmeng.cn@gmail.com Tested-by: Bin Meng bmeng.cn@gmail.com Reviewed-by: Simon Glass sjg@chromium.org

---

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

diff --git a/cmd/usb.c b/cmd/usb.c index d95bcf5..907debe 100644 --- a/cmd/usb.c +++ b/cmd/usb.c @@ -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);
          }
  
  

-- Best regards, Marek Vasut

On Thu, Oct 19, 2017 at 12:33 AM, Marek Vasut marex@denx.de wrote:

On 10/19/2017 05:24 AM, Suneel Garapati wrote:

...

On Wed, Oct 18, 2017 at 6:39 PM, Marek Vasut marex@denx.de wrote:

...

On 10/19/2017 03:22 AM, Suneel Garapati wrote:

...

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 ...

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.

...

...

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 ?

May work, I will test and send v7

Regards, Suneel

...

...

...

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

...

...

Signed-off-by: Suneel Garapati suneelglinux@gmail.com Reviewed-by: Bin Meng bmeng.cn@gmail.com Tested-by: Bin Meng bmeng.cn@gmail.com Reviewed-by: Simon Glass sjg@chromium.org

---

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

diff --git a/cmd/usb.c b/cmd/usb.c index d95bcf5..907debe 100644 --- a/cmd/usb.c +++ b/cmd/usb.c @@ -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);
          }
  
  

-- Best regards, Marek Vasut

-- Best regards, Marek Vasut