[U-Boot] [PATCH v2 1/9] dm: usb: Copy over usb_device values from usb_scan_device() to final usb_device

[Simon Glass]

Hi Hans,

On 1 May 2015 at 04:04, Hans de Goede <hdegoede at redhat.com> wrote:
>
> Currently we copy over a number of usb_device values stored in the on stack
> struct usb_device probed in usb_scan_device() to the final driver-model managed
> struct usb_device in usb_child_pre_probe() through usb_device_platdata, and
> then call usb_select_config() to fill in the rest.
>
> There are 3 problems with this approach:
>
> 1) It does not fill in enough fields before calling usb_select_config(),
> specifically it does not fill in ep0's maxpacketsize causing a div by zero
> exception in the ehci driver.
>
> 2) It unnecessarily redoes a number of usb requests making usb probing slower
>
> 3) Calling usb_select_config() a second time fails on some usb-1 devices
> plugged into usb-2 hubs, causing u-boot to not recognize these devices.
>
> This commit fixes these issues by removing the usb_select_config() call from
> usb_child_pre_probe(), and instead of copying over things field by field
> through usb_device_platdata, store a pointer to the in stack usb_device
> (which is still valid when usb_child_pre_probe() gets called) and copy
> over the entire struct.

OK well you've heard my reservations. Given that we need to fix this
and I don't have the inclination to do the full refactor now (to allow
USB drivers to send requests without a device) I think we should go
along with what you propose.

Let's make sure no one builds on it, by adding comments about the
intended direction (separating out the request from the device that
issues it).

>
> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> ---
>  drivers/usb/host/usb-uclass.c | 27 ++++++---------------------
>  include/usb.h                 |  5 +----
>  2 files changed, 7 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c
> index 714bc0e..95e371d 100644
> --- a/drivers/usb/host/usb-uclass.c
> +++ b/drivers/usb/host/usb-uclass.c
> @@ -535,12 +535,7 @@ int usb_scan_device(struct udevice *parent, int port,
>         }
>         plat = dev_get_parent_platdata(dev);
>         debug("%s: Probing '%s', plat=%p\n", __func__, dev->name, plat);
> -       plat->devnum = udev->devnum;
> -       plat->speed = udev->speed;
> -       plat->slot_id = udev->slot_id;
> -       plat->portnr = port;
> -       debug("** device '%s': stashing slot_id=%d\n", dev->name,
> -             plat->slot_id);
> +       plat->udev = udev;

This is a local variable. How can it be safe to use this pointer and
then return from this function?

For now, how about putting the whole usb_device in the plat structure.
I think it is a lesser evil. You can add a large TODO to fix it up.

>         priv->next_addr++;
>         ret = device_probe(dev);
>         if (ret) {
> @@ -599,25 +594,15 @@ int usb_get_bus(struct udevice *dev, struct udevice **busp)
>
>  int usb_child_pre_probe(struct udevice *dev)
>  {
> -       struct udevice *bus;
>         struct usb_device *udev = dev_get_parentdata(dev);
>         struct usb_dev_platdata *plat = dev_get_parent_platdata(dev);
> -       int ret;
>
> -       ret = usb_get_bus(dev, &bus);
> -       if (ret)
> -               return ret;
> -       udev->controller_dev = bus;
> +       /*
> +        * Copy over all the values set in the on stack struct usb_device in
> +        * usb_scan_device() to our final struct usb_device for this dev.
> +        */
> +       *udev = *(plat->udev);
>         udev->dev = dev;
> -       udev->devnum = plat->devnum;
> -       udev->slot_id = plat->slot_id;
> -       udev->portnr = plat->portnr;
> -       udev->speed = plat->speed;
> -       debug("** device '%s': getting slot_id=%d\n", dev->name, plat->slot_id);
> -
> -       ret = usb_select_config(udev);
> -       if (ret)
> -               return ret;
>
>         return 0;
>  }
> diff --git a/include/usb.h b/include/usb.h
> index 1984e8f..1b667ff 100644
> --- a/include/usb.h
> +++ b/include/usb.h
> @@ -581,10 +581,7 @@ struct usb_platdata {
>   */
>  struct usb_dev_platdata {
>         struct usb_device_id id;
> -       enum usb_device_speed speed;
> -       int devnum;
> -       int slot_id;
> -       int portnr;     /* Hub port number, 1..n */
> +       struct usb_device *udev;

Structure comment needs updating. I think it could do with some big
hairy warnings also.

>  #ifdef CONFIG_SANDBOX
>         struct usb_string *strings;
>         /* NULL-terminated list of descriptor pointers */
> --
> 2.3.6
>

Regards,
Simon