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

Simon Glass
2015-06-29T03:45:11Z

Hi Hans,

On 17 June 2015 at 13:33, Hans de Goede <hdegoede@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@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