[U-Boot] [UBOOT] [PATCH] cmd: usb: ignore block devices under mass storage device

[Suneel Garapati]

Hi Simon,

On Thu, Aug 31, 2017 at 5:52 AM, Simon Glass <sjg at chromium.org> wrote:
> Hi Suneel,
>
> On 15 August 2017 at 11:06, Suneel Garapati <suneelglinux at gmail.com> wrote:
>> Hi Simon,
>>
>>
>> On Sun, Aug 13, 2017 at 2:37 PM, Simon Glass <sjg at chromium.org> wrote:
>>> Hi Suneel,
>>>
>>> On 10 August 2017 at 23:53, Suneel Garapati <suneelglinux at gmail.com> wrote:
>>>> usb tree and info commands may cause crash otherwise
>>>>
>>>> Signed-off-by: Suneel Garapati <suneelglinux at gmail.com>
>>>> ---
>>>>  cmd/usb.c | 6 ++++--
>>>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>
>>> Thank you for the patch - it certainly looks like a bug. Can you
>>> please expand the commit message a little? E.g. you have
>>> UCLASS_USB_EMUL below.
>>
>> I will change the description
>>
>>>
>>>> diff --git a/cmd/usb.c b/cmd/usb.c
>>>> index 992d414..81e1a7b 100644
>>>> --- a/cmd/usb.c
>>>> +++ b/cmd/usb.c
>>>> @@ -415,7 +415,8 @@ 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) {
>>>> +               if (device_get_uclass_id(child) !=
>>>> +                   (UCLASS_USB_EMUL | UCLASS_BLK)) {
>>>
>>> This seems odd to me. Do you mean to check that the child uclass is
>>> neither USB_EMUL nor BLK?
>>>
>>> Would it be possible to check that the parent is UCLASS_USB? That
>>> seems like a better condition to determine whether the child has USB
>>> parent data.
>>
>> It is possible to check parent uclass but would that ever fail?
>
> How would it fail? The only device that does not have a parent is the
> root device, and we know it cannot be that since it will be
> UCLASS_ROOT.
>
>> I assume, block device under usb storage device will always have
>> parent as usb class.
>
> Yes that sounds right.
>
>> Also, tree is called on only usb class devices. Maybe I am missing something.
>
> Maybe, but I think it is more robust to check for the thing you want
> than the things you don't. If someone adds a new thing that can be a
> child then this code will need updating.
I will add a check on parent uclass for USB in v1.

Regards,
Suneel

>
>>
>> Regards,
>> Suneel
>>
>>>
>>>>                         usb_show_tree_graph(udev, pre);
>>>>                         pre[index] = 0;
>>>>                 }
>>>> @@ -605,7 +606,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);
>>>>                 }
>>>> --
>>>> 2.7.4
>>>>
>
> Regards,
> Simon