[U-Boot] [PATCH v3] cmd: usb: add blk devices to ignore list in tree graph

[Suneel Garapati]

Hi Bin,

On Tue, Sep 19, 2017 at 8:32 PM, Bin Meng <bmeng.cn at gmail.com> wrote:
> Hi Suneel,
>
> On Wed, Sep 20, 2017 at 2:31 AM, Suneel Garapati <suneelglinux at gmail.com> wrote:
>> Hi Bin,
>>
>> On Tue, Sep 19, 2017 at 12:32 AM, Bin Meng <bmeng.cn at gmail.com> wrote:
>>> Hi Suneel,
>>>
>>> On Tue, Sep 19, 2017 at 1:55 PM, Suneel Garapati <suneelglinux at gmail.com> wrote:
>>>> add blk child devices to ignore list while displaying
>>>> usb tree graph, otherwise usb tree and info commands
>>>> may cause crash treating blk as usb device.
>>>>
>>>> Signed-off-by: Suneel Garapati <suneelglinux at gmail.com>
>>>> ---
>>>>
>>>> Changes v3:
>>>>  - remove 'check on parent uclass' in description
>>>
>>> thanks for making the changes.
>>>
>>>> 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 | 11 ++++++++---
>>>>  1 file changed, 8 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/cmd/usb.c b/cmd/usb.c
>>>> index d95bcf5..3889994 100644
>>>> --- a/cmd/usb.c
>>>> +++ b/cmd/usb.c
>>>> @@ -414,8 +414,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 +609,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);
>>>>                 }
>>>> --
>>>
>>> My testing of 'usb info' looks OK, however 'usb tree' still has some
>>> issues below:
>>>
>>> => usb tree
>>> USB device tree:
>>>   1  Hub (5 Gb/s, 0mA)
>>>   |  U-Boot XHCI Host Controller
>>>   |
>>>   +-2  Hub (5 Gb/s, 0mA)
>>>   | |  GenesysLogic USB3.0 Hub
>>>   | |
>>>   | +-5  Vendor specific (5 Gb/s, 36mA)
>>>   |      Realtek USB 10/100/1000 LAN 00E04C680977
>>>   |
>> Leaving block devices, why the extra print here for lan port?
>
> There is nothing wrong here. Every device has a separation line.
>
>>
>>>   +-3  Hub (480 Mb/s, 100mA)
>>>   | |  GenesysLogic USB2.0 Hub
>>>   | |
>> And here?
>>
>
> Again, nothing wrong here.
>
>>>   | +-6  Mass Storage (480 Mb/s, 98mA)
>>>   | | |  USBest Technology USB Mass Storage Device 101111c452b7c0
>>>   | | |
>>>
>>> As you see, we just don't print out the BLK device, but we still print
>>> out the | here.
>> I believe if the extra print for other devices is correct, then this
>> tree is fine.
>
> It's not correct. The tree graphic does not look correct now.
>
>> Also, I believe this is not related to the fix this patch aims at.
>> Let me know if otherwise.
>
> No, you should not fix one thing but introduce another thing.

Ok. Let me be explicit here, to understand where I am going wrong.

Each usb_show_tree_graph call on a device can be broken down like
below into three line print sets

<old-pre><devnum> <class description>
<new-pre><device description>
<repeat new-pre> (This is unconditional print, even before this fix)

USB device tree:

  1  Hub (5 Gb/s, 0mA)
  |  U-Boot XHCI Host Controller
  |

  +-2  Hub (5 Gb/s, 0mA)
  | |  GenesysLogic USB3.0 Hub
  | |

  | +-5  Vendor specific (5 Gb/s, 36mA)
  |      Realtek USB 10/100/1000 LAN 00E04C680977
  |

  +-3  Hub (480 Mb/s, 100mA)
  | |  GenesysLogic USB2.0 Hub
  | | (You confirmed this as device separator)

  | +-6  Mass Storage (480 Mb/s, 98mA)
  | | |  USBest Technology USB Mass Storage Device 101111c452b7c0
  | | | (so is this unconditional print as device separator)

Call to block child is ignored here, so is the set of prints as
described above and continues with next device below.
It is not like only device description is cancelled but preamble is
being printed.

  | +-7  Human Interface (1.5 Mb/s, 70mA)
  |      Dell Dell USB Keyboard
  |

  +-4  Mass Storage (480 Mb/s, 300mA)
    |  JetFlash Mass Storage Device 16Q6ZPH20GF3E8UQ
    |

Also, request you to let me know the correct tree graph you have in mind.

Regards,
Suneel

>
>>
>> Regards,
>> Suneel
>>>
>>>   | +-7  Human Interface (1.5 Mb/s, 70mA)
>>>   |      Dell Dell USB Keyboard
>>>   |
>>>   +-4  Mass Storage (480 Mb/s, 300mA)
>>>     |  JetFlash Mass Storage Device 16Q6ZPH20GF3E8UQ
>>>     |
>>>
>>> And here.
>
> Regards,
> Bin