On Thu, Oct 19, 2017 at 12:33 AM, Marek Vasut <ma...@denx.de> wrote: > On 10/19/2017 05:24 AM, Suneel Garapati wrote: >> On Wed, Oct 18, 2017 at 6:39 PM, Marek Vasut <ma...@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 <suneelgli...@gmail.com> >>>> Reviewed-by: Bin Meng <bmeng...@gmail.com> >>>> Tested-by: Bin Meng <bmeng...@gmail.com> >>>> Reviewed-by: Simon Glass <s...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot