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