[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 14:20, Hans de Goede <hdegoede at redhat.com> wrote:
> 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.

We really should not be calling usb_get_dev_index() from the keyboard
driver - that function is an interim port-helper. If it were ported
fully to driver model it could use USB_DEVICE() like mass storage. We
could fairly easily change the devnum to 0 for all devices instead of
deleting them, and I suspect that would solve the problem.

But we would have to port usb ethernet which uses the same technique.

Anyway I think you've persuaded me that I might be worrying too much
about what is to come. USB is not really the same as PCI in the sense
that settings don't seem to be added to the device like PCI. Let's go
with your approach. We still have the USB emulation stuff working, and
that's the only real use case today.

Regards,
Simon