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

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 29-06-15 05:45, Simon Glass wrote:
> 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  ?

My intention was for both, since I was under the assumption that "usb stop"
on the cmdline, was the only caller of usb_stop(), but a quick grep to the
sources show that I'm wrong ...

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

This was mostly a result of my thinking that usb_stop() is only used
on "usb stop" at the cmdline, which I know realize is wrong.

However my quick grep has learned that we do really need CONFIG_DM_DEVICE_REMOVE
to properly implement usb_stop():

 From common/bootm.c :

#if defined(CONFIG_CMD_USB)
         /*
          * turn off USB to prevent the host controller from writing to the
          * SDRAM while Linux is booting. This could happen (at least for OHCI
          * controller), because the HCCA (Host Controller Communication Area)
          * lies within the SDRAM and the host controller writes continously to
          * this area (as busmaster!). The HccaFrameNumber is for example
          * updated every 1 ms within the HCCA structure in SDRAM! For more
          * details see the OpenHCI specification.
          */
         usb_stop();
#endif

And without CONFIG_DM_DEVICE_REMOVE we end up never calling the hcd's remove
callback and thus do not properly stop the usb controller.

So this problem of usb_stop() needing CONFIG_DM_DEVICE_REMOVE already exists
before this patch. If you want I can split out the adding of the #ifdef
in a separate commit, spelling out why usb_stop() MUST have
CONFIG_DM_DEVICE_REMOVE in the commit message. Or maybe just move this all to
Kconfig and make DM_USB conflict with CONFIG_DM_DEVICE_REMOVE?



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

Right, but just removing the children is a much cleaner solution, and also
makes the output of "dm tree" properly reflect reality.

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

Ack.

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

Renaming it to usb_find_emul_child() and only defining the function when
CONFIG_USB_EMUL works for me I will do that for v2.

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

Because its call site does not have a #ifdef, I'll add an #ifdef at its
call site to make it more clear that this is only used for emulated
devices.

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

Regards,

Hans