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

[Marek Vasut]

On 10/19/2017 05:24 AM, Suneel Garapati wrote:
> 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.

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

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

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


-- 
Best regards,
Marek Vasut