[PATCH v2] cmd: usb: Prevent reset in usb tree/info command

[Marek Vasut]

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 at tinet.cat>
> Reviewed-by: Simon Glass <sjg at chromium.org>
> Reviewed-by: Marek Vasut <marex at denx.de>
> Tested-by: Marek Vasut <marex at 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.