[U-Boot] [PATCH v6] cmd: usb: ignore blk, emulation devices in usb tree/info display

[Suneel Garapati]

On Wed, Oct 18, 2017 at 6:39 PM, Marek Vasut <marex at 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 at gmail.com>
>> Reviewed-by: Bin Meng <bmeng.cn at gmail.com>
>> Tested-by: Bin Meng <bmeng.cn at gmail.com>
>> Reviewed-by: Simon Glass <sjg at 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