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

[Hans de Goede]

Hi,

On 06/30/2015 06:07 PM, Simon Glass wrote:

<snip>

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

AFAIK there is not a single example of having settings in devicetree
for an usb device, since usb-devices are always 100% self describing
since usb is a bus designed for hot(un)plug from the outset.

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

You're asking to make the code more complicated here using a what if
reasoning with a "what if" which is likely to never happen.

>
>>
>>
>>
>>>
>>>>
>>>>
>>>>>
>>>>>>
>>>>>> 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 unbind is necessary for usb_get_dev_index() to work properly,
which is necessary for proper output of "usb tree" and for driver
binding to work properly, without the unbind usb-keyboards will e.g.
not work in certain circumstances.

> The
> state of 'dm tree' does not bother me,

The state if dm tree is what usb_get_dev_index() works from, so if
it is not in a good state, then usb_get_dev_index() will not work.

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

Again this is a what if reasoning for a hypothetical future problem
which will likely never happen, where as the broken state of the
dm tree after a "usb reset" is causing real problems.

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

Nope we cannot leave this alone, without unbinding usb devices which
no longer exist, the dm tree will be broken and with it
usb_get_dev_index() and through usb_get_dev_index() the keyboard
driver.

Regards,

Hans