[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 09:54, Hans de Goede <hdegoede at redhat.com> wrote:
> Hi,
>
>
> On 06/30/2015 04:58 PM, Simon Glass wrote:
>>
>> 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?
>
>
> Ok will do.
>
>> 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?
>
>
> That should work, but I do not really see any advantage in that,
> removing the children is not that expensive and it feels like a
> kludge.

That's how it currently works, from what I can see in the code. But
since there is a 'usb_started' boolean this is irrelevant.

>
>> 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.
>
>
> AFAIK there is no such thing as usb devices in devicetree, which
> makes sense as usb is a fully discoverable bus.

Sort-of. But as with PCI it is useful to be able to add settings for
the devices in some cases. You can match them using vendor/device or
interface IDs. Then the driver can access its settings.

That's why I'm suggesting we unbind the devices that are no-longer present.

>
>
>
>>
>>>
>>>
>>>>
>>>>>
>>>>> 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.
>
>
> That seems like needless complexity. I believe that simply removing +
> unbinding
> the children on usb_stop is the right thing to do, and it also is the KISS
> solution.

I'm good with the remove(), but less sure about the unbind(). The
state of 'dm tree' does not bother me, and I worry that we then limit
our ability to use usb_find_child() to locate a device's parameters
(i.e. support for more complex devices which need settings might be
harder).

For now, can we just leave this alone? I really don't want to re-visit
this later.

Regards,
Simon