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

[Simon Glass]

Hi Heinrich,

On Sun, 12 Nov 2023 at 13:32, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
>
>
> 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.

I can't remember exactly what is needed from the bootflow, but
something is. Perhaps the kernel, or the cmdline, or fdt? It would
have been better if I had mentioned this explicitly,  But then this
patch has been sitting around for ages...

In any case, the intent of exit-boot-services is not to free all the
memory used, since some of it may have been used to hold data that the
app continues to use.

Also there is U-Boot code between when the devices are unbound and
when U-Boot actually exits back to the app.

This hang was 100% repeatable on brya (an x86 Chromebook) and it took
quite a while to find.

Regards,
Simon


>
> 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 */
> >
> >