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

[Heinrich Schuchardt <xypron.glpk@gmx.de>]

Am 12. November 2023 22:20:50 MEZ schrieb Simon Glass <sjg@chromium.org>:
>Hi Heinrich,
>
>On Sun, 12 Nov 2023 at 13:32, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>>
>>
>> Am 12. November 2023 21:02:42 MEZ schrieb Simon Glass <sjg@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.

The EFI application reads the memory map and receives an ID which it passes to ExitBootServiced() after this point any memory not marked as EFI runtime can be used by the EFI app. This includes the memory that harbors the U-Boot USB drivers. Therefore no drivers can be used here.

Starting the EFI app via StartImage() must  terminate any running U-Boot "boot flow".

After ExitBootServices() the EFI application cannot return to U-Boot except for SetVirtualAdressMspsetting which relocates the EFI runtime.

Bootflows and U-Boot drivers have no meaning after ExitBootServices().



>
>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.

We need a better understanding of the problem that you experience to find an adequate solution. Why does removing all devices lead to hanging the system?

Best regards

Heinrich

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