[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 30 June 2015 at 06:54, Hans de Goede <hdegoede at redhat.com> wrote:
> Hi,
>
> On 29-06-15 05:45, Simon Glass wrote:
>>
>> 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  ?
>
>
> 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?
>

I don't think that is necessary, it feels a bit too inflexible. But
perhaps you could add a comment to the Kconfig help for
CONFIG_DM_DEVICE_REMOVE?

It is remove() that is needed, not unbind(). Actually I think it is
quite unfortunate to make usb_stop() call unbind. It is a waste of
time to do this just before booting the kernel - the current design
leaves all devices bound (but I hope we can remove() them at some
point).

Instead, I wonder if we can remove the children when we probe the bus?
Also, what happens to children that are in the device tree - i.e.
static USB devices like WiFi? The device tree might have parameters
for them. Still, that might not matter as I'm not sure that case is
handled correctly today.

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

True, although you also have 'usb tree' for that. Another option would
be to mark devices that were found and remove the others after the
scan.

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