[U-Boot] [PATCH 08/22] dm: usb: Use device_chld_remove and _unbind to clean up usb devs on stop

[Simon Glass]

Hi Hans,

On 17 June 2015 at 13:33, Hans de Goede <hdegoede at redhat.com> wrote:
> On an usb stop instead of leaving orphan usb devices behind simply remove

On a usb_stop()

or

On a 'usb stop' command

?

> them. This requires CONFIG_DM_DEVICE_REMOVE to be set, so only build
> usb_stop() when that is set.

This seems a little unfortunate. I can see the reasoning, but do you
think this is necessary? I suspect people chasing code size may remove
that option and still want to use USB properly.

>
> The result of this commit is best seen in the output of "dm tree" after
> plugging out an usb hub with 2 devices plugges in and plugging in a keyb.
> instead, before this commit the output would be:
>
>  usb         [ + ]    `-- sunxi-musb
>  usb_hub     [   ]        |-- usb_hub
>  usb_mass_st [   ]        |   |-- usb_mass_storage
>  usb_dev_gen [   ]        |   `-- generic_bus_0_dev_3
>  usb_dev_gen [ + ]        `-- generic_bus_0_dev_1
>
> Notice the non active usb_hub child and its 2 non active children. The
> first child being non-active as in this example also causes usb_get_dev_index
> to return NULL when probing the first child, which results in the usb kbd
> code not binding to the keyboard.

Although I suspect that could be fixed.

>
> With this commit in place the output after swapping and "usb reset" is:
>
>  usb         [ + ]    `-- sunxi-musb
>  usb_dev_gen [ + ]        `-- generic_bus_0_dev_1
>
> As expected, and usb_get_dev_index works properly and the keyboard works.
>
> After this commit usb_find_child() is only necessary for emulated usb
> devices, so make its body #ifdef CONFIG_USB_EMUL.
>
> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> ---
>  drivers/usb/host/usb-uclass.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c
> index bce6cec..8f26e35 100644
> --- a/drivers/usb/host/usb-uclass.c
> +++ b/drivers/usb/host/usb-uclass.c
> @@ -128,6 +128,7 @@ int usb_alloc_device(struct usb_device *udev)
>         return ops->alloc_device(bus, udev);
>  }
>
> +#ifdef CONFIG_DM_DEVICE_REMOVE
>  int usb_stop(void)
>  {
>         struct udevice *bus;
> @@ -143,6 +144,12 @@ int usb_stop(void)
>         uc_priv = uc->priv;
>
>         uclass_foreach_dev(bus, uc) {
> +               ret = device_chld_remove(bus);
> +               if (ret && !err)
> +                       err = ret;
> +               ret = device_chld_unbind(bus);
> +               if (ret && !err)
> +                       err = ret;
>                 ret = device_remove(bus);
>                 if (ret && !err)
>                         err = ret;
> @@ -166,6 +173,7 @@ int usb_stop(void)
>
>         return err;
>  }
> +#endif
>
>  static void usb_scan_bus(struct udevice *bus, bool recurse)
>  {
> @@ -491,6 +499,7 @@ static int usb_find_child(struct udevice *parent,
>                           struct usb_interface_descriptor *iface,
>                           struct udevice **devp)
>  {
> +#ifdef CONFIG_USB_EMUL /* Emulated devices are explictily bound */

explicitly

Can you add a comment about this? It seems that we should rename this
function to usb_find_emul_child() and have it present only when
CONFIG_USB_EMUL is around?

Also, why bother with the #ifdef if this function is never called
outside sandbox?

>         struct udevice *dev;
>
>         *devp = NULL;
> @@ -509,6 +518,7 @@ static int usb_find_child(struct udevice *parent,
>                         return 0;
>                 }
>         }
> +#endif
>
>         return -ENOENT;
>  }
> --
> 2.4.3
>

Regards,
Simon