Hi Mattijs, On Sat, 23 Sept 2023 at 10:39, Mattijs Korpershoek <[email protected]> wrote: > > Hi Simon, > > Thank you for your patch.
Thanks for reviewing it! > > On ven., sept. 22, 2023 at 15:38, Simon Glass <[email protected]> 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 <[email protected]> > > --- > > > > 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() ? I missed this comment but will do this for v4. > > > { > > 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(). I'm not sure about that. The USB enumeration binds new devices, which is why we unbind them before enumerating. remove_inactive_children() only removes them (makes them inactive / unprobed). It does not unbind them. > > > + > > 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 Regards, Simon
