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

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

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(-)

Hi Simon,

On Sun, Nov 19, 2023 at 7:12 PM 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.
>

I am no UEFI / bootloader expert and while trying to gather more info
on EFI ExitBootServies I came across this
https://github.com/tianocore-docs/edk2-UefiDriverWritersGuide/blob/master/7_driver_entry_point/77_adding_the_exit_boot_services_feature.md

If I understand this correctly, EFI ( U-boot in this case) should be
halting all USB controllers like done in usb_stop()
Is my understanding correct?

 Kind regards
Shantur

Hi Shantur,

On Sun, 19 Nov 2023 at 15:47, Shantur Rathore <i@shantur.com> wrote:
>
> Hi Simon,
>
> On Sun, Nov 19, 2023 at 7:12 PM 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.
> >
>
> I am no UEFI / bootloader expert and while trying to gather more info
> on EFI ExitBootServies I came across this
> https://github.com/tianocore-docs/edk2-UefiDriverWritersGuide/blob/master/7_driver_entry_point/77_adding_the_exit_boot_services_feature.md
>
> If I understand this correctly, EFI ( U-boot in this case) should be
> halting all USB controllers like done in usb_stop()
> Is my understanding correct?

Yes, that is correct. The dm_remove_devices_flags() call should do
that. The code in bootm_disable_interrupts() is a hangover from the
pre-driver model days, I suspect.

Perhaps we should also remove the eth_halt() and usb_pause() from
bootm_disable_interrupts() ?

Regards,
Simon

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

Hi Simon,

On Tue, Nov 21, 2023 at 2:17 AM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Shantur,
>
> On Sun, 19 Nov 2023 at 15:47, Shantur Rathore <i@shantur.com> wrote:
> >
> > Hi Simon,
> >
> > On Sun, Nov 19, 2023 at 7:12 PM 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.
> > >
> >
> > I am no UEFI / bootloader expert and while trying to gather more info
> > on EFI ExitBootServies I came across this
> > https://github.com/tianocore-docs/edk2-UefiDriverWritersGuide/blob/master/7_driver_entry_point/77_adding_the_exit_boot_services_feature.md
> >
> > If I understand this correctly, EFI ( U-boot in this case) should be
> > halting all USB controllers like done in usb_stop()
> > Is my understanding correct?
>
> Yes, that is correct. The dm_remove_devices_flags() call should do
> that. The code in bootm_disable_interrupts() is a hangover from the
> pre-driver model days, I suspect.
>
> Perhaps we should also remove the eth_halt() and usb_pause() from
> bootm_disable_interrupts() ?
>

Apologies for delayed response.
In this case, I think we should remove eth_halt() and
usb_stop() (there is no usb_pause() ) from bootm_disable_interrupts().

No point stopping things twice.

Kind regards,
Shantur

Hi Shantur,

On Thu, 23 Nov 2023 at 04:35, Shantur Rathore <i@shantur.com> wrote:
>
> Hi Simon,
>
> On Tue, Nov 21, 2023 at 2:17 AM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Shantur,
> >
> > On Sun, 19 Nov 2023 at 15:47, Shantur Rathore <i@shantur.com> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Sun, Nov 19, 2023 at 7:12 PM 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.
> > > >
> > >
> > > I am no UEFI / bootloader expert and while trying to gather more info
> > > on EFI ExitBootServies I came across this
> > > https://github.com/tianocore-docs/edk2-UefiDriverWritersGuide/blob/master/7_driver_entry_point/77_adding_the_exit_boot_services_feature.md
> > >
> > > If I understand this correctly, EFI ( U-boot in this case) should be
> > > halting all USB controllers like done in usb_stop()
> > > Is my understanding correct?
> >
> > Yes, that is correct. The dm_remove_devices_flags() call should do
> > that. The code in bootm_disable_interrupts() is a hangover from the
> > pre-driver model days, I suspect.
> >
> > Perhaps we should also remove the eth_halt() and usb_pause() from
> > bootm_disable_interrupts() ?
> >
>
> Apologies for delayed response.
> In this case, I think we should remove eth_halt() and
> usb_stop() (there is no usb_pause() ) from bootm_disable_interrupts().
>
> No point stopping things twice.

Yes, I agree. I will take a look.

Regards,
Simon