* [U-Boot] [PATCH v6] cmd: usb: ignore blk, emulation devices in usb tree/info display
@ 2017-10-19 1:22 Suneel Garapati
2017-10-19 1:39 ` Marek Vasut
0 siblings, 1 reply; 11+ messages in thread
From: Suneel Garapati @ 2017-10-19 1:22 UTC (permalink / raw)
To: u-boot
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) in usb_show_info and usb_tree_graph.
also avoid addition of preamble for blk uclass child devices,
otherwise tree dump gets messed up.
Signed-off-by: Suneel Garapati <suneelglinux@gmail.com>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
Tested-by: Bin Meng <bmeng.cn@gmail.com>
Reviewed-by: Simon Glass <sjg@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);
}
--
2.7.4
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [U-Boot] [PATCH v6] cmd: usb: ignore blk, emulation devices in usb tree/info display
2017-10-19 1:22 [U-Boot] [PATCH v6] cmd: usb: ignore blk, emulation devices in usb tree/info display Suneel Garapati
@ 2017-10-19 1:39 ` Marek Vasut
2017-10-19 3:24 ` Suneel Garapati
0 siblings, 1 reply; 11+ messages in thread
From: Marek Vasut @ 2017-10-19 1:39 UTC (permalink / raw)
To: u-boot
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 ?
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 ?
> 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 ...
> Signed-off-by: Suneel Garapati <suneelglinux@gmail.com>
> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> Tested-by: Bin Meng <bmeng.cn@gmail.com>
> Reviewed-by: Simon Glass <sjg@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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [U-Boot] [PATCH v6] cmd: usb: ignore blk, emulation devices in usb tree/info display
2017-10-19 1:39 ` Marek Vasut
@ 2017-10-19 3:24 ` Suneel Garapati
2017-10-19 7:33 ` Marek Vasut
0 siblings, 1 reply; 11+ messages in thread
From: Suneel Garapati @ 2017-10-19 3:24 UTC (permalink / raw)
To: u-boot
On Wed, Oct 18, 2017 at 6:39 PM, Marek Vasut <marex@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@gmail.com>
>> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
>> Tested-by: Bin Meng <bmeng.cn@gmail.com>
>> Reviewed-by: Simon Glass <sjg@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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [U-Boot] [PATCH v6] cmd: usb: ignore blk, emulation devices in usb tree/info display
2017-10-19 3:24 ` Suneel Garapati
@ 2017-10-19 7:33 ` Marek Vasut
2017-10-19 8:03 ` Suneel Garapati
2017-10-19 8:37 ` Bin Meng
0 siblings, 2 replies; 11+ messages in thread
From: Marek Vasut @ 2017-10-19 7:33 UTC (permalink / raw)
To: u-boot
On 10/19/2017 05:24 AM, Suneel Garapati wrote:
> On Wed, Oct 18, 2017 at 6:39 PM, Marek Vasut <marex@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@gmail.com>
>>> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
>>> Tested-by: Bin Meng <bmeng.cn@gmail.com>
>>> Reviewed-by: Simon Glass <sjg@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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [U-Boot] [PATCH v6] cmd: usb: ignore blk, emulation devices in usb tree/info display
2017-10-19 7:33 ` Marek Vasut
@ 2017-10-19 8:03 ` Suneel Garapati
2017-10-19 8:37 ` Bin Meng
1 sibling, 0 replies; 11+ messages in thread
From: Suneel Garapati @ 2017-10-19 8:03 UTC (permalink / raw)
To: u-boot
On Thu, Oct 19, 2017 at 12:33 AM, Marek Vasut <marex@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@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@gmail.com>
>>>> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
>>>> Tested-by: Bin Meng <bmeng.cn@gmail.com>
>>>> Reviewed-by: Simon Glass <sjg@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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [U-Boot] [PATCH v6] cmd: usb: ignore blk, emulation devices in usb tree/info display
2017-10-19 7:33 ` Marek Vasut
2017-10-19 8:03 ` Suneel Garapati
@ 2017-10-19 8:37 ` Bin Meng
2017-10-19 8:43 ` Marek Vasut
1 sibling, 1 reply; 11+ messages in thread
From: Bin Meng @ 2017-10-19 8:37 UTC (permalink / raw)
To: u-boot
Hi Marek,
On Thu, Oct 19, 2017 at 3:33 PM, Marek Vasut <marex@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@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 ...
>
No, it cannot.
>>> 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 ?
>
No, I don't think you can enumerate all USB devices here. It can be
any uclass device.
>>>> 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.
>>
[snip]
Regards,
Bin
^ permalink raw reply [flat|nested] 11+ messages in thread
* [U-Boot] [PATCH v6] cmd: usb: ignore blk, emulation devices in usb tree/info display
2017-10-19 8:37 ` Bin Meng
@ 2017-10-19 8:43 ` Marek Vasut
2017-10-19 8:56 ` Bin Meng
0 siblings, 1 reply; 11+ messages in thread
From: Marek Vasut @ 2017-10-19 8:43 UTC (permalink / raw)
To: u-boot
On 10/19/2017 10:37 AM, Bin Meng wrote:
> Hi Marek,
Hi,
> On Thu, Oct 19, 2017 at 3:33 PM, Marek Vasut <marex@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@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 ...
>>
>
> No, it cannot.
Why ? Because of the Kconfig thing ? That can easily change and then
this breaks ...
>>>> 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 ?
>>
>
> No, I don't think you can enumerate all USB devices here. It can be
> any uclass device.
And only the blk one breaks things ?
>>>>> 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.
>>>
>
> [snip]
>
> Regards,
> Bin
>
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 11+ messages in thread
* [U-Boot] [PATCH v6] cmd: usb: ignore blk, emulation devices in usb tree/info display
2017-10-19 8:43 ` Marek Vasut
@ 2017-10-19 8:56 ` Bin Meng
2017-10-19 9:47 ` Marek Vasut
0 siblings, 1 reply; 11+ messages in thread
From: Bin Meng @ 2017-10-19 8:56 UTC (permalink / raw)
To: u-boot
Hi Marek,
On Thu, Oct 19, 2017 at 4:43 PM, Marek Vasut <marex@denx.de> wrote:
> On 10/19/2017 10:37 AM, Bin Meng wrote:
>> Hi Marek,
>
> Hi,
>
>> On Thu, Oct 19, 2017 at 3:33 PM, Marek Vasut <marex@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@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 ...
>>>
>>
>> No, it cannot.
>
> Why ? Because of the Kconfig thing ? That can easily change and then
> this breaks ...
Yes, it's currently on on Sandbox. But whether it's on Sandbox or not
does not matter. These devices are should be filtered out as they are
not supposed to be on the USB topology.
>
>>>>> 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 ?
>>>
>>
>> No, I don't think you can enumerate all USB devices here. It can be
>> any uclass device.
>
> And only the blk one breaks things ?
Yes, blk devices are not "struct usb_device" declared, as they are not
USB devices. However their parent is.
>
>>>>>> 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,
Bin
^ permalink raw reply [flat|nested] 11+ messages in thread
* [U-Boot] [PATCH v6] cmd: usb: ignore blk, emulation devices in usb tree/info display
2017-10-19 8:56 ` Bin Meng
@ 2017-10-19 9:47 ` Marek Vasut
2017-10-19 9:52 ` Bin Meng
0 siblings, 1 reply; 11+ messages in thread
From: Marek Vasut @ 2017-10-19 9:47 UTC (permalink / raw)
To: u-boot
On 10/19/2017 10:56 AM, Bin Meng wrote:
> Hi Marek,
>
> On Thu, Oct 19, 2017 at 4:43 PM, Marek Vasut <marex@denx.de> wrote:
>> On 10/19/2017 10:37 AM, Bin Meng wrote:
>>> Hi Marek,
>>
>> Hi,
>>
>>> On Thu, Oct 19, 2017 at 3:33 PM, Marek Vasut <marex@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@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 ...
>>>>
>>>
>>> No, it cannot.
>>
>> Why ? Because of the Kconfig thing ? That can easily change and then
>> this breaks ...
>
> Yes, it's currently on on Sandbox. But whether it's on Sandbox or not
> does not matter. These devices are should be filtered out as they are
> not supposed to be on the USB topology.
I agree with that, I was just commenting on this "(sandbox)" in the
description, which IMO is not precise.
>>>>>> 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 ?
>>>>
>>>
>>> No, I don't think you can enumerate all USB devices here. It can be
>>> any uclass device.
>>
>> And only the blk one breaks things ?
>
> Yes, blk devices are not "struct usb_device" declared, as they are not
> USB devices. However their parent is.
In which case, _that_ should be in the commit message.
Anyway, is there any chance something else will come in, ie. net device
for USB ethernet devices ?
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 11+ messages in thread
* [U-Boot] [PATCH v6] cmd: usb: ignore blk, emulation devices in usb tree/info display
2017-10-19 9:47 ` Marek Vasut
@ 2017-10-19 9:52 ` Bin Meng
2017-10-19 10:41 ` Marek Vasut
0 siblings, 1 reply; 11+ messages in thread
From: Bin Meng @ 2017-10-19 9:52 UTC (permalink / raw)
To: u-boot
Hi Marek,
On Thu, Oct 19, 2017 at 5:47 PM, Marek Vasut <marex@denx.de> wrote:
> On 10/19/2017 10:56 AM, Bin Meng wrote:
>> Hi Marek,
>>
>> On Thu, Oct 19, 2017 at 4:43 PM, Marek Vasut <marex@denx.de> wrote:
>>> On 10/19/2017 10:37 AM, Bin Meng wrote:
>>>> Hi Marek,
>>>
>>> Hi,
>>>
>>>> On Thu, Oct 19, 2017 at 3:33 PM, Marek Vasut <marex@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@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 ...
>>>>>
>>>>
>>>> No, it cannot.
>>>
>>> Why ? Because of the Kconfig thing ? That can easily change and then
>>> this breaks ...
>>
>> Yes, it's currently on on Sandbox. But whether it's on Sandbox or not
>> does not matter. These devices are should be filtered out as they are
>> not supposed to be on the USB topology.
>
> I agree with that, I was just commenting on this "(sandbox)" in the
> description, which IMO is not precise.
>
>>>>>>> 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 ?
>>>>>
>>>>
>>>> No, I don't think you can enumerate all USB devices here. It can be
>>>> any uclass device.
>>>
>>> And only the blk one breaks things ?
>>
>> Yes, blk devices are not "struct usb_device" declared, as they are not
>> USB devices. However their parent is.
>
> In which case, _that_ should be in the commit message.
>
> Anyway, is there any chance something else will come in, ie. net device
> for USB ethernet devices ?
>
No problem with any USB devices that end up in the leaf node in the DM tree.
Regards,
Bin
^ permalink raw reply [flat|nested] 11+ messages in thread
* [U-Boot] [PATCH v6] cmd: usb: ignore blk, emulation devices in usb tree/info display
2017-10-19 9:52 ` Bin Meng
@ 2017-10-19 10:41 ` Marek Vasut
0 siblings, 0 replies; 11+ messages in thread
From: Marek Vasut @ 2017-10-19 10:41 UTC (permalink / raw)
To: u-boot
On 10/19/2017 11:52 AM, Bin Meng wrote:
> Hi Marek,
>
> On Thu, Oct 19, 2017 at 5:47 PM, Marek Vasut <marex@denx.de> wrote:
>> On 10/19/2017 10:56 AM, Bin Meng wrote:
>>> Hi Marek,
>>>
>>> On Thu, Oct 19, 2017 at 4:43 PM, Marek Vasut <marex@denx.de> wrote:
>>>> On 10/19/2017 10:37 AM, Bin Meng wrote:
>>>>> Hi Marek,
>>>>
>>>> Hi,
>>>>
>>>>> On Thu, Oct 19, 2017 at 3:33 PM, Marek Vasut <marex@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@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 ...
>>>>>>
>>>>>
>>>>> No, it cannot.
>>>>
>>>> Why ? Because of the Kconfig thing ? That can easily change and then
>>>> this breaks ...
>>>
>>> Yes, it's currently on on Sandbox. But whether it's on Sandbox or not
>>> does not matter. These devices are should be filtered out as they are
>>> not supposed to be on the USB topology.
>>
>> I agree with that, I was just commenting on this "(sandbox)" in the
>> description, which IMO is not precise.
>>
>>>>>>>> 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 ?
>>>>>>
>>>>>
>>>>> No, I don't think you can enumerate all USB devices here. It can be
>>>>> any uclass device.
>>>>
>>>> And only the blk one breaks things ?
>>>
>>> Yes, blk devices are not "struct usb_device" declared, as they are not
>>> USB devices. However their parent is.
>>
>> In which case, _that_ should be in the commit message.
>>
>> Anyway, is there any chance something else will come in, ie. net device
>> for USB ethernet devices ?
>>
>
> No problem with any USB devices that end up in the leaf node in the DM tree.
So what triggers this issue are partitions and co. then ?
> Regards,
> Bin
>
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2017-10-19 10:41 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-19 1:22 [U-Boot] [PATCH v6] cmd: usb: ignore blk, emulation devices in usb tree/info display Suneel Garapati
2017-10-19 1:39 ` Marek Vasut
2017-10-19 3:24 ` Suneel Garapati
2017-10-19 7:33 ` Marek Vasut
2017-10-19 8:03 ` Suneel Garapati
2017-10-19 8:37 ` Bin Meng
2017-10-19 8:43 ` Marek Vasut
2017-10-19 8:56 ` Bin Meng
2017-10-19 9:47 ` Marek Vasut
2017-10-19 9:52 ` Bin Meng
2017-10-19 10:41 ` Marek Vasut
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.