* [U-Boot] [UBOOT] [PATCH] cmd: usb: ignore block devices under mass storage device
@ 2017-08-11 5:53 Suneel Garapati
2017-08-13 21:37 ` Simon Glass
2017-09-01 6:30 ` Lothar Waßmann
0 siblings, 2 replies; 8+ messages in thread
From: Suneel Garapati @ 2017-08-11 5:53 UTC (permalink / raw)
To: u-boot
usb tree and info commands may cause crash otherwise
Signed-off-by: Suneel Garapati <suneelglinux@gmail.com>
---
cmd/usb.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
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)) {
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [U-Boot] [UBOOT] [PATCH] cmd: usb: ignore block devices under mass storage device
2017-08-11 5:53 [U-Boot] [UBOOT] [PATCH] cmd: usb: ignore block devices under mass storage device Suneel Garapati
@ 2017-08-13 21:37 ` Simon Glass
2017-08-15 3:06 ` Suneel Garapati
2017-09-01 6:30 ` Lothar Waßmann
1 sibling, 1 reply; 8+ messages in thread
From: Simon Glass @ 2017-08-13 21:37 UTC (permalink / raw)
To: u-boot
Hi Suneel,
On 10 August 2017 at 23:53, Suneel Garapati <suneelglinux@gmail.com> wrote:
> usb tree and info commands may cause crash otherwise
>
> Signed-off-by: Suneel Garapati <suneelglinux@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.
> 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.
> 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [U-Boot] [UBOOT] [PATCH] cmd: usb: ignore block devices under mass storage device
2017-08-13 21:37 ` Simon Glass
@ 2017-08-15 3:06 ` Suneel Garapati
2017-08-28 18:37 ` Suneel Garapati
2017-08-31 12:52 ` Simon Glass
0 siblings, 2 replies; 8+ messages in thread
From: Suneel Garapati @ 2017-08-15 3:06 UTC (permalink / raw)
To: u-boot
Hi Simon,
On Sun, Aug 13, 2017 at 2:37 PM, Simon Glass <sjg@chromium.org> wrote:
> Hi Suneel,
>
> On 10 August 2017 at 23:53, Suneel Garapati <suneelglinux@gmail.com> wrote:
>> usb tree and info commands may cause crash otherwise
>>
>> Signed-off-by: Suneel Garapati <suneelglinux@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?
I assume, block device under usb storage device will always have
parent as usb class.
Also, tree is called on only usb class devices. Maybe I am missing something.
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [U-Boot] [UBOOT] [PATCH] cmd: usb: ignore block devices under mass storage device
2017-08-15 3:06 ` Suneel Garapati
@ 2017-08-28 18:37 ` Suneel Garapati
2017-08-31 12:52 ` Simon Glass
1 sibling, 0 replies; 8+ messages in thread
From: Suneel Garapati @ 2017-08-28 18:37 UTC (permalink / raw)
To: u-boot
Hi,
Request for review on comments below.
Regards,
Suneel
On Mon, Aug 14, 2017 at 8:06 PM, Suneel Garapati <suneelglinux@gmail.com> wrote:
> Hi Simon,
>
>
> On Sun, Aug 13, 2017 at 2:37 PM, Simon Glass <sjg@chromium.org> wrote:
>> Hi Suneel,
>>
>> On 10 August 2017 at 23:53, Suneel Garapati <suneelglinux@gmail.com> wrote:
>>> usb tree and info commands may cause crash otherwise
>>>
>>> Signed-off-by: Suneel Garapati <suneelglinux@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?
> I assume, block device under usb storage device will always have
> parent as usb class.
> Also, tree is called on only usb class devices. Maybe I am missing something.
>
> 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [U-Boot] [UBOOT] [PATCH] cmd: usb: ignore block devices under mass storage device
2017-08-15 3:06 ` Suneel Garapati
2017-08-28 18:37 ` Suneel Garapati
@ 2017-08-31 12:52 ` Simon Glass
2017-09-01 7:34 ` Suneel Garapati
1 sibling, 1 reply; 8+ messages in thread
From: Simon Glass @ 2017-08-31 12:52 UTC (permalink / raw)
To: u-boot
Hi Suneel,
On 15 August 2017 at 11:06, Suneel Garapati <suneelglinux@gmail.com> wrote:
> Hi Simon,
>
>
> On Sun, Aug 13, 2017 at 2:37 PM, Simon Glass <sjg@chromium.org> wrote:
>> Hi Suneel,
>>
>> On 10 August 2017 at 23:53, Suneel Garapati <suneelglinux@gmail.com> wrote:
>>> usb tree and info commands may cause crash otherwise
>>>
>>> Signed-off-by: Suneel Garapati <suneelglinux@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.
>
> 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [U-Boot] [UBOOT] [PATCH] cmd: usb: ignore block devices under mass storage device
2017-08-11 5:53 [U-Boot] [UBOOT] [PATCH] cmd: usb: ignore block devices under mass storage device Suneel Garapati
2017-08-13 21:37 ` Simon Glass
@ 2017-09-01 6:30 ` Lothar Waßmann
2017-09-01 7:35 ` Suneel Garapati
1 sibling, 1 reply; 8+ messages in thread
From: Lothar Waßmann @ 2017-09-01 6:30 UTC (permalink / raw)
To: u-boot
Hi,
On Thu, 10 Aug 2017 22:53:31 -0700 Suneel Garapati wrote:
> usb tree and info commands may cause crash otherwise
>
> Signed-off-by: Suneel Garapati <suneelglinux@gmail.com>
> ---
> cmd/usb.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> 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 should most probably be:
> + if ((device_get_uclass_id(child) != UCLASS_USB_EMUL) &&
(device_get_uclass_id(child) != UCLASS_BLK)) {
Lothar Waßmann
^ permalink raw reply [flat|nested] 8+ messages in thread
* [U-Boot] [UBOOT] [PATCH] cmd: usb: ignore block devices under mass storage device
2017-08-31 12:52 ` Simon Glass
@ 2017-09-01 7:34 ` Suneel Garapati
0 siblings, 0 replies; 8+ messages in thread
From: Suneel Garapati @ 2017-09-01 7:34 UTC (permalink / raw)
To: u-boot
Hi Simon,
On Thu, Aug 31, 2017 at 5:52 AM, Simon Glass <sjg@chromium.org> wrote:
> Hi Suneel,
>
> On 15 August 2017 at 11:06, Suneel Garapati <suneelglinux@gmail.com> wrote:
>> Hi Simon,
>>
>>
>> On Sun, Aug 13, 2017 at 2:37 PM, Simon Glass <sjg@chromium.org> wrote:
>>> Hi Suneel,
>>>
>>> On 10 August 2017 at 23:53, Suneel Garapati <suneelglinux@gmail.com> wrote:
>>>> usb tree and info commands may cause crash otherwise
>>>>
>>>> Signed-off-by: Suneel Garapati <suneelglinux@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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [U-Boot] [UBOOT] [PATCH] cmd: usb: ignore block devices under mass storage device
2017-09-01 6:30 ` Lothar Waßmann
@ 2017-09-01 7:35 ` Suneel Garapati
0 siblings, 0 replies; 8+ messages in thread
From: Suneel Garapati @ 2017-09-01 7:35 UTC (permalink / raw)
To: u-boot
Hi,
On Thu, Aug 31, 2017 at 11:30 PM, Lothar Waßmann <LW@karo-electronics.de> wrote:
> Hi,
>
> On Thu, 10 Aug 2017 22:53:31 -0700 Suneel Garapati wrote:
>> usb tree and info commands may cause crash otherwise
>>
>> Signed-off-by: Suneel Garapati <suneelglinux@gmail.com>
>> ---
>> cmd/usb.c | 6 ++++--
>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> 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 should most probably be:
>> + if ((device_get_uclass_id(child) != UCLASS_USB_EMUL) &&
> (device_get_uclass_id(child) != UCLASS_BLK)) {
Agree. Will make change in v1.
Regards,
Suneel
>
>
> Lothar Waßmann
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-09-01 7:35 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-11 5:53 [U-Boot] [UBOOT] [PATCH] cmd: usb: ignore block devices under mass storage device Suneel Garapati
2017-08-13 21:37 ` Simon Glass
2017-08-15 3:06 ` Suneel Garapati
2017-08-28 18:37 ` Suneel Garapati
2017-08-31 12:52 ` Simon Glass
2017-09-01 7:34 ` Suneel Garapati
2017-09-01 6:30 ` Lothar Waßmann
2017-09-01 7:35 ` Suneel Garapati
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.