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

[Suneel Garapati]

Hi Bin,

On Wed, Sep 20, 2017 at 12:42 AM, Bin Meng <bmeng.cn at gmail.com> wrote:
> Hi Suneel,
>
> On Wed, Sep 20, 2017 at 2:32 PM, Suneel Garapati <suneelglinux at gmail.com> wrote:
>> 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.
>>
>
> Please check the attached 'usb tree' logs, and I believe that explains
> where the problem is.

Thanks for the inputs. I have sent v4 including the fix for unwanted
preamble for child blk devices.

Regards,
Suneel

>
> Regards,
> Bin