Re: [PATCH v2 4/4] usb: Avoid unbinding devices in use by bootflows

Hi Simon,

Thank you for your patch.

On ven., sept. 22, 2023 at 15:38, Simon Glass sjg@chromium.org wrote:

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.

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.

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@chromium.org

Changes in v2:

• Add new patch to avoid unbinding devices in use by bootflows

boot/bootm.c | 2 +- common/usb.c | 7 ++++++- drivers/usb/host/usb-uclass.c | 14 ++++++++++++-- include/usb.h | 15 ++++++++++++++- 4 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/boot/bootm.c b/boot/bootm.c index b1c3afe0a3a..5c9ba083e64 100644 --- a/boot/bootm.c +++ b/boot/bootm.c @@ -501,7 +501,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 836506dcd9e..4d6ac69111e 100644 --- a/common/usb.c +++ b/common/usb.c @@ -126,7 +126,7 @@ int usb_init(void) /******************************************************************************

• Stop USB this stops the LowLevel Part and deregisters USB devices.

*/ -int usb_stop(void) +int usb_pause(void)

nit: I know this is the "legacy" (pre-DM) stuff, but isn't renaming the above function making the comment wrong?

Can't we keep this function usb_stop() and create another one named usb_pause() which calls usb_stop() ?

{ int i;

@@ -144,6 +144,11 @@ int usb_stop(void) return 0; }

+int usb_stop(void) +{

• return usb_pause();

+}

/******************************************************************************

• 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 a1cd0ad2d66..c26c65d7986 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);

+}

What happens if someone calls usb_pause() followed by usb_init() on a root hub ?

I think it behaves properly because usb_init() calls remove_inactive_children().

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 09e3f0cb309..ad39b09a6e4 100644 --- a/include/usb.h +++ b/include/usb.h @@ -265,7 +265,20 @@ 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
• • Return: 0 if OK, -ve on error
• */

+int usb_stop(void);

+/**

• • usb_pause() - stop the USB Controller DMA, etc.
• • 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 */

-- 2.42.0.515.g380fc7ccd1-goog