Hi Heinrich,
On Sun, 12 Nov 2023 at 13:32, Heinrich Schuchardt <xypron.g...@gmx.de> wrote:
>
>
>
> Am 12. November 2023 21:02:42 MEZ schrieb Simon Glass <s...@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 <s...@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
> > */
> >
> >
