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

[Simon Glass]

Hi Mattijs,

On Sat, 23 Sept 2023 at 10:39, Mattijs Korpershoek
<mkorpershoek at baylibre.com> wrote:
>
> Hi Simon,
>
> Thank you for your patch.

Thanks for reviewing it!

>
> On ven., sept. 22, 2023 at 15:38, Simon Glass <sjg at 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 at 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() ?

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