From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hans de Goede Date: Tue, 30 Jun 2015 22:20:56 +0200 Subject: [U-Boot] [PATCH 08/22] dm: usb: Use device_chld_remove and _unbind to clean up usb devs on stop In-Reply-To: References: <1434569645-30322-1-git-send-email-hdegoede@redhat.com> <1434569645-30322-9-git-send-email-hdegoede@redhat.com> <5592917D.9040000@redhat.com> <5592BBCE.7020008@redhat.com> Message-ID: <5592FA28.8010704@redhat.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi, On 06/30/2015 06:07 PM, Simon Glass wrote: >>> 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