[PATCH v4 05/12] usb: Avoid unbinding devices in use by bootflows

[Heinrich Schuchardt]

Am 12. November 2023 21:02:42 MEZ schrieb Simon Glass <sjg at chromium.org>:
>When a USB device is unbound, it causes any bootflows attached to it to
>be removed, via a call to bootdev_clear_bootflows() from
>bootdev_pre_unbind(). This obviously makes it impossible to boot the
>bootflow.
>
>However, when booting a bootflow that relies on USB, usb_stop() is
>called, which unbinds the device. For EFI, this happens in
>efi_exit_boot_services() which means that the bootflow disappears
>before it is finished with.

After ExitBootServices() no driver of U-Boot can be used as the operating system is in charge.

Any bootflow inside U-Boot is terminated by definition when reaching ExitBootServices.

>
>There is no need to unbind all the USB devices just to quiesce them.
>Add a new usb_pause() call which removes them but leaves them bound.

As U-Boot must not access any device after ExitBootServices() it is unclear to me what you want to achieve.

Best regards

Heinrich

>
>This resolves a hang on x86 when booting a distro from USB. This was
>found using a device with 4 bootflows, the last of which was USB.
>
>
>Signed-off-by: Simon Glass <sjg at chromium.org>
>---
>
>Changes in v4:
>- Don't rename the legacy-USB functions
>- Add a bit more detail to the comment
>
>Changes in v2:
>- Add new patch to avoid unbinding devices in use by bootflows
>
> boot/bootm.c                  |  2 +-
> common/usb.c                  |  5 +++++
> drivers/usb/host/usb-uclass.c | 14 ++++++++++++--
> include/usb.h                 | 21 ++++++++++++++++++++-
> 4 files changed, 38 insertions(+), 4 deletions(-)
>
>diff --git a/boot/bootm.c b/boot/bootm.c
>index cb61485c226c..d9c3fa8dad99 100644
>--- a/boot/bootm.c
>+++ b/boot/bootm.c
>@@ -502,7 +502,7 @@ ulong bootm_disable_interrupts(void)
> 	 * updated every 1 ms within the HCCA structure in SDRAM! For more
> 	 * details see the OpenHCI specification.
> 	 */
>-	usb_stop();
>+	usb_pause();
> #endif
> 	return iflag;
> }
>diff --git a/common/usb.c b/common/usb.c
>index 836506dcd9e9..a486d1c87d4d 100644
>--- a/common/usb.c
>+++ b/common/usb.c
>@@ -144,6 +144,11 @@ int usb_stop(void)
> 	return 0;
> }
> 
>+int usb_pause(void)
>+{
>+	return usb_stop();
>+}
>+
> /******************************************************************************
>  * Detect if a USB device has been plugged or unplugged.
>  */
>diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c
>index a1cd0ad2d669..c26c65d7986b 100644
>--- a/drivers/usb/host/usb-uclass.c
>+++ b/drivers/usb/host/usb-uclass.c
>@@ -173,7 +173,7 @@ int usb_get_max_xfer_size(struct usb_device *udev, size_t *size)
> 	return ops->get_max_xfer_size(bus, size);
> }
> 
>-int usb_stop(void)
>+static int usb_finish(bool unbind_all)
> {
> 	struct udevice *bus;
> 	struct udevice *rh;
>@@ -195,7 +195,7 @@ int usb_stop(void)
> 
> 		/* Locate root hub device */
> 		device_find_first_child(bus, &rh);
>-		if (rh) {
>+		if (rh && unbind_all) {
> 			/*
> 			 * All USB devices are children of root hub.
> 			 * Unbinding root hub will unbind all of its children.
>@@ -222,6 +222,16 @@ int usb_stop(void)
> 	return err;
> }
> 
>+int usb_stop(void)
>+{
>+	return usb_finish(true);
>+}
>+
>+int usb_pause(void)
>+{
>+	return usb_finish(false);
>+}
>+
> static void usb_scan_bus(struct udevice *bus, bool recurse)
> {
> 	struct usb_bus_priv *priv;
>diff --git a/include/usb.h b/include/usb.h
>index 09e3f0cb309c..b964e2e1f6b2 100644
>--- a/include/usb.h
>+++ b/include/usb.h
>@@ -265,7 +265,26 @@ int usb_kbd_deregister(int force);
>  */
> int usb_init(void);
> 
>-int usb_stop(void); /* stop the USB Controller */
>+/**
>+ * usb_stop() - stop the USB Controller and unbind all USB controllers/devices
>+ *
>+ * This unbinds all devices on the USB buses.
>+ *
>+ * Return: 0 if OK, -ve on error
>+ */
>+int usb_stop(void);
>+
>+/**
>+ * usb_pause() - stop the USB Controller DMA, etc.
>+ *
>+ * Note that this does not unbind bus devices, so using usb_init() after this
>+ * call is not permitted. This function is only useful just before booting, to
>+ * ensure that devices are dormant.
>+ *
>+ * Return: 0 if OK, -ve on error
>+ */
>+int usb_pause(void);
>+
> int usb_detect_change(void); /* detect if a USB device has been (un)plugged */
> 
>