[U-Boot] [PATCH 0/5] RFC: usb: Drop requirement for USB unbinding

[Simon Glass]

Hi Hans,

On 12 September 2015 at 08:00, Hans de Goede <hdegoede at redhat.com> wrote:
>
> Hi,
>
> On 08-09-15 19:15, Simon Glass wrote:
>>
>> There was quite a bit of discussion about the change that required the
>> unbinding of USB devices for the subsystem to function correctly. E.g.
>>
>> https://patchwork.ozlabs.org/patch/485637/
>>
>> The key issue is the usb_get_dev_index() function which is not a good API
>> for driver model. We can drop use of this function once everything is
>> converted to driver model. Then I believe the problems raised by Hans go
>> away. For now we can add a deprecation warning on the function.
>>
>> It is easy to convert USB keyboards to driver model. This series includes
>> a patch for that.
>>
>> This series also includes reverts for the three commits which as discussed
>> I would like to drop. U-Boot should be able to run normally and exit without
>> unbinding anything.
>>
>> I cannot actually repeat the problem that Hans mentions in the above thread
>> so I may be missing a step. Hans, any ideas on this?
>
>
> So I've just tried this series on an allwinner tablet which uses a musb
> controller in host mode. Note that this has no root hub, so the first
> child of the controller is an actual device not a root hub.
>
> When I first plug in a usb keyboard directly into the otg port and then do
> usb tree + dm tree (output shortened) I get:
>
> USB device tree:
>   1  Human Interface (1.5 Mb/s, 100mA)
>      SINO WEALTH USB Composite Device
>
> => dm tree
>  Class       Probed   Name
> ----------------------------------------
>  usb         [ + ]    `-- sunxi-musb
>  keyboard    [ + ]        `-- usb_kbd
>
> If I then unplug the keyboard, plug in a hub and plug the
> keyboard into the hub and do a usb reset I get:
>
> => usb tree
> USB device tree:
> => dm tree
>  Class       Probed   Name
> ----------------------------------------
>  usb         [ + ]    `-- sunxi-musb
>  keyboard    [   ]        |-- usb_kbd
>  usb_hub     [ + ]        `-- usb_hub
>  keyboard    [ + ]            `-- usb_kbd
>
> Notice how the "usb tree" command output is
> empty, that is because the usb tree code stops
> at the first removed usb-device. As discussed before
> if we go the route this patch-set is going we need
> to teach *all* code iterating over devices to skip
> removed devices, rather then to see a removed device
> as the end of the list.
>
> At least thanks to the conversion of the usb-kbd driver
> to the dm the keyboard still works (which it did not in
> the past). That conversion has issues of its own btw,
> I will reply to that patch with my comments.
>
> ###
>
> Related to the above (failed) test I believe that
> the first set of this patch:
>
> Revert "dm: usb: Rename usb_find_child to usb_find_emul_child"
>
> Is wrong and is the beginning of various problems, last time
> we discussed not making the usb code depend on the dm unbind
> code you suggested to simply remove the devices, and not re-use
> them ever (which means not using usb_find_child in non sandbox
> builds).
>
> I believe that this is the right approach, re-using them will
> result in all kind of weirdness, let me give an example:
>
> Plug in a hub, plug a keyb into port 1 and a storage device into
> port 2, do "usb tree" :
>
> => usb tree
> USB device tree:
>   1  Hub (480 Mb/s, 100mA)
>   |   USB2.0 Hub
>   |
>   +-2  Human Interface (1.5 Mb/s, 100mA)
>   |    SINO WEALTH USB Composite Device
>   |
>   +-3  Mass Storage (480 Mb/s, 100mA)
>        USB Flash Disk 4C0E960F
>
> No swap the 2 devices, so usb storage into port 1, keyb into
> port 2:
>
> USB device tree:
>   1  Hub (480 Mb/s, 100mA)
>   |   USB2.0 Hub
>   |
>   +-3  Human Interface (1.5 Mb/s, 100mA)
>   |    SINO WEALTH USB Composite Device
>   |
>   +-2  Mass Storage (480 Mb/s, 100mA)
>        USB Flash Disk 4C0E960F
>
> And here we see that the usb stack now scans the
> storage-dev first and assigs it usb addr 2, but usb tree
> shows it after the keyb because the existing dm-device
> structs were re-used, and they appear out of order in
> the list now making the tree output no longer an accurate
> representation of the actual physical topology.
>
> And if we add more levels of hubs etc, things will likely
> only get worse, not better, possibly leading to non
> cosmetic problems.
>
>
> I believe that the way to make the dm usb code work
> without dm-unbind is to simply keep the removed devices,
> and make sure that all code going over the device tree
> ignores these removed usb devices (with the exception
> of core dm functions), and to NEVER re-use them.
>
> That and in the case of not building without dm-unbind
> actually call device_unbind_children(bus), iow instead
> of reverting "dm: usb: Use device_unbind_children to
> clean up usb devs on stop" wrap the device_unbind_children(bus)
> call it adds in #ifdef CONFIG_DM_DEVICE_REMOVE
>
> ###
>
> How ever we end up fixing this, I believe that this set
> certainly is not ready for merging into v2015.10.

Thanks for looking at this. I'm sorry I didn't get to it before. But
your original patches fixed a bug so I was keen to to avoid holding
things up. Let's target the next release.

My main objective is to make the unbinding optional, rather than
required for USB to function.

I'll see if I can repeat your test case. It is the other way around
from what I was trying, and from how I understood your original bug
report: here you are plugging in a device, then removing it and
plugging it in via a hub. I was doing it the other way around.

I think the best approach may be to create a sandbox test which checks
this behaviour as well as the output of 'usb tree'.

Were you able to test the driver model USB keyboard support?

Regards,
Simon