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.ti... ] 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) &&@@ -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) &&
usb_show_info(udev);
if (udev)usb_show_info(udev);
Applied to usb/master, thanks.