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

[Suneel Garapati]

On Thu, Oct 19, 2017 at 12:33 AM, Marek Vasut <marex at 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 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 ...
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 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