On Thu, 16 Jan 2014, Sarah Sharp wrote: > > > What do you think about the rest of the patchset? > > > > I regret that I haven't taken the time to look through it yet. :-( > > I'll do my best to go through it soon. > > No worries! I'm most interested in your comments about the changes to > khubd, so if you could find time to look at just patches 7 and 9, that > would be appreciated. Okay, I have looked at them (and patch 8 too). In short, I disapprove of the design. It's an ad-hoc approach that ignores the larger issues. The real problem is that we need to guarantee mutual exclusion for access to a particular port on the hub. The things we can do to a port include: khubd's normal processing: turning off various port status-change flags, handling remote wakeups, taking care of overcurrent events, issuing USB-3 warm resets, and handling connect/disconnect events; Issuing a port reset, which can happen: when a USB-3 port goes into SS.Inactive, when a driver resets a device, or when a reset-resume is needed; Resuming a port (perhaps suspending it too); Turning port power on or off. These four somewhat overlapping actions need to be mutually exclusive. (Actually, the first of khubd's activities -- turning off status-change flags -- probably doesn't need to be exclusive with anything else. But we might as well include it with the rest.) The natural solution is use a per-port mutex, instead of messing around with atomic busy_bits stuff or PM barriers. It turns out that we will require a particular locking order. It will sometimes be necessary to acquire the port's mutex while holding the child device's lock. This can happen when we are binding or unbinding a driver to the child device; usb_probe_interface() is called with the child locked, and it calls usb_autoresume_device() which will need to lock the upstream port if the device is in runtime suspend. As far as I can tell, we don't actually need to acquire the locks in the opposite order, although to make that work means we will have to drop the port lock in various parts of hub_port_connect_change() where the child device gets locked. One possibility is to use the port's own embedded device lock as the port mutex. But this would preclude ever moving the child USB device directly under the port device in the device tree, because the locking order would be wrong. It seems safer to embed a new mutex in struct usb_port. Dan, what do you think of this approach? Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
