On Mon, 16 Apr 2012, Sarah Sharp wrote: > > In other words, something like this patch. However I'm not sure how > > well it would interact with your > > fccf4e86200b8f5edd9a65da26f150e32ba79808 commit; it undoes several of > > the changes you introduced. > > I'm not sure what you mean by "undoes". This patch just pushes the > bandwidth mutex acquirement into usb_disable_device() and shrinks the > mutex code coverage a bit, correct? Assuming there are no unwanted race > conditions for bandwidth allocation/deallocation, the behavior should > still be basically the same, right? Yes, it should. > The only issue I can see with pushing the mutex lock acquire into > usb_disable_device() is the following scenario: > > 1. Userspace (via the bConfiguration file) or a driver wants to switch a > configured device (device A) to a new configuration. > > 2. usb_disable_device() locks the mutex and removes the old > configuration from the xHCI internal structures. It drops the mutex and > returns to usb_set_configuration(). > > 3. A new USB device (device B) is connected, and the hub thread takes > the bandwidth mutex before usb_set_configuration() can re-acquire it. > It installs a configuration for device B that takes up the rest of the > bandwidth. > > 4. usb_set_configuration() re-acquires the bandwidth mutex and attempts > to install the requested new configuration for device A. That fails > because there isn't enough bandwidth. > > Now device A is in an odd state, because the old configuration is still > installed in the device. We can't re-install the old configuration in > the xHCI host internal structures because there isn't enough bandwidth. That's true. The device is unchanged, but the old data structures have been destroyed in both the host controller and the kernel. > If the bandwidth mutex coverage stayed the same, device B would fail to > be enumerated, and device A would have either the new configuration or > the old configuration (depending on if there was enough room for the new > configuration), but it would still be usable. Well, not exactly. The kernel's data structures (including the driver bindings) would still be gone, so the A device would effectively be unusable if there wasn't enough bandwidth for the new config. usb_set_configuration() doesn't try to re-register the old interfaces when that happens. > I guess from a user's perspective, in either case, one of the devices is > just not going to work. I think it would be better if the new device > was the one that stops working, but since there's race conditions in the > current code, I think it's OK that the old device could become unusable > and the user will need to unplug/replug it. It's going to be very > confusing to users to have inconsistent behavior, but I think it's a > corner case that not many people will run into. In fact, once the transition gets started the A device cannot continue to be used as it was. Hence the choice amounts to: A device can be used in its new config, B device can be used, or neither device is usable. I think either the first or the second would be acceptable (better than the third, at least). Recovering the old device wouldn't require an unplug/replug. Setting the old configuration over agin would work once the race was over, assuming enough bandwidth was still available. > So my initial reaction to the patch is that it looks fine. Did you have > any other specific concerns/scenarios that you wanted to run past me? Nope. Just wanted to make sure I hadn't missed anything subtle. > You should also get rid of this comment above usb_disable_device(): > > /* > ... > * Must be called with hcd->bandwidth_mutex held. > */ Right, thanks. 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
