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

[Hans de Goede]

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.

Regards,

Hans