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

[Mattijs Korpershoek <mkorpershoek@baylibre.com>]

Hi Simon,

Thank you for your patch.

On dim., nov. 19, 2023 at 12:11, Simon Glass <sjg@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. At that point any information
> attached to the bootflow is dropped.
>
> This is quite risky since the contents of freed memory are not
> guaranteed to remain unchanged. Depending on what other options are
> done before boot, a hard-to-find bug may crop up.
>
> 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.
>
> Tested-by: Shantur Rathore <i@shantur.com>
>
> Signed-off-by: Simon Glass <sjg@chromium.org>

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

> ---
>
> Changes in v5:
> - Adjust motivation for patch, since the EFI hang is fixed
>
> 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 */
>  
>  
> -- 
> 2.43.0.rc0.421.g78406f8d94-goog