[U-Boot] [UBOOT] [PATCH] cmd: usb: ignore block devices under mass storage device
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); }
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
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
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
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
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
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
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
participants (3)
-
Lothar Waßmann -
Simon Glass -
Suneel Garapati