Message ID | ZJKjAysLpd47sNo0@xdrudis.tinet.cat |
---|---|
State | Accepted |
Commit | 7a875a8e5c7af1d7c1c7232fbef898d0f18ec7da |
Delegated to: | Marek Vasut |
Headers | show |
Series | [v2] cmd: usb: Prevent reset in usb tree/info command | expand |
On 6/21/23 09:13, Xavier Drudis Ferran wrote: > > Commands causing reset in some configs: > > When bootflow scan is run, this will cause a UCLASS_BOOTDEV device to > be added as sibling of those UCLASS_BLK devices found in the search > chain defined in environment variable "boot_targets", until boot > succeeds from some device. This can happen automatically as part of > the default boot process on some boards (example: Rock Pi 4) depending > on the board configuration (DISTRO_DEFAULTS, BOOTSTD, BOOTCOMMAND, > etc.) because they have bootcmd=bootflow scan. > > If boot doesn't succeed from any device, and usb is in boot_targets, > and an usb storage device is plugged to some usb port at boot time, > its UCLASS_MASS_STORAGE device will have a UCLASS_BOOTDEV device as > child, besides a UCLASS_BLK child. > > If once the boot fails the user enters at the U-Boot shell prompt: > > usb info > > or > > usb tree > > The code in cmd/usb.c will eventually recurse into the UCLASS_BOOTDEV > device and pass a null usb_device pointer to usb_show_tree_graph() or > usb_show_info() (because it has no parent_priv_). > > This causes a reset. The expected behaviour would be to ignore the > UCLASS_BOOTDEV device, continue listing the usb information and return > to the prompt. > > > Minimal test: > > Another way to trigger this reset as a minimal test or on boards with > a different bootcmd would be: > > - make sure "usb" is in environment variable boot_targets (might need > setenv boot_targets usb; and/or saveenv and reset), then, with a usb > storage device plugged to a usb port, run: > > => usb reset ; bootflow scan ; usb info > > > Solution: > > Fix it (twice) by checking for null parent_priv_ and adding > UCLASS_BOOTDEV to the list of ignored class ids before the recursive > call. > > This prevents the current particular problem with UCLASS_BOOTDEV, even > in case it ever gets some parent_priv_ struct which is not an > usb_device, despite being the child of a usb_device->dev. And it also > prevents possible future problems if other children are added to usb > devices that don't have parent_priv_ because they are not part of the > usb tree, just abstractions of functionality (like UCLASS_BLK and > UCLASS_BOOTDEV are now). > > > Signed-off-by: Xavier Drudis Ferran <xdrudis@tinet.cat> > Reviewed-by: Simon Glass <sjg@chromium.org> > Reviewed-by: Marek Vasut <marex@denx.de> > Tested-by: Marek Vasut <marex@denx.de> > > --- > > v2: added UCLASS_BOOTDEV check (discussion of v1 dried up without much > evident consensus, so hopefully Simon Glass likes it better now) > [ https://patchwork.ozlabs.org/project/uboot/patch/ZH39SVA1vbZR3+Gk@xdrudis.tinet.cat/ ] > Improved the commit message trying to follow Marek Vasut's advice. > > --- > cmd/usb.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/cmd/usb.c b/cmd/usb.c > index 6193728384..23253f2223 100644 > --- a/cmd/usb.c > +++ b/cmd/usb.c > @@ -421,7 +421,9 @@ static void usb_show_tree_graph(struct usb_device *dev, char *pre) > * Ignore emulators and block child devices, we only want > * real devices > */ > - if ((device_get_uclass_id(child) != UCLASS_USB_EMUL) && > + if (udev && > + (device_get_uclass_id(child) != UCLASS_BOOTDEV) && > + (device_get_uclass_id(child) != UCLASS_USB_EMUL) && > (device_get_uclass_id(child) != UCLASS_BLK)) { > usb_show_tree_graph(udev, pre); > pre[index] = 0; > @@ -604,10 +606,12 @@ static void usb_show_info(struct usb_device *udev) > child; > device_find_next_child(&child)) { > if (device_active(child) && > + (device_get_uclass_id(child) != UCLASS_BOOTDEV) && > (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); > + if (udev) > + usb_show_info(udev); > } > } > } Applied to usb/master, thanks.
diff --git a/cmd/usb.c b/cmd/usb.c index 6193728384..23253f2223 100644 --- a/cmd/usb.c +++ b/cmd/usb.c @@ -421,7 +421,9 @@ static void usb_show_tree_graph(struct usb_device *dev, char *pre) * Ignore emulators and block child devices, we only want * real devices */ - if ((device_get_uclass_id(child) != UCLASS_USB_EMUL) && + if (udev && + (device_get_uclass_id(child) != UCLASS_BOOTDEV) && + (device_get_uclass_id(child) != UCLASS_USB_EMUL) && (device_get_uclass_id(child) != UCLASS_BLK)) { usb_show_tree_graph(udev, pre); pre[index] = 0; @@ -604,10 +606,12 @@ static void usb_show_info(struct usb_device *udev) child; device_find_next_child(&child)) { if (device_active(child) && + (device_get_uclass_id(child) != UCLASS_BOOTDEV) && (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); + if (udev) + usb_show_info(udev); } } }