On Fri, 14 Feb 2014, Dan Williams wrote: > On Mon, Feb 10, 2014 at 1:01 PM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote: > > The patch description here could be improved substantially. It should > > start out with an explanation of the basic reason for the patch and a > > clear indication of what the patch does. > > > > On Fri, 31 Jan 2014, Dan Williams wrote: > > > >> Make port power recovery behave similarly to the power session recovery > >> that occurs after a system / hub suspend event. Arrange for a > > > > That is not clear to me, mostly because of its lack of specificity. I > > would phrase it more like this: > > > > Whenever a USB port is powered back on (runtime resumed), > > ask khubd to do an autoresume of the attached child device as > > soon as possible. > > Yes, that's clearer. > > > > >> usb_device to always complete a usb_port_resume() run prior to the next > >> khubd run. This serves several purposes: > > > > "Prior to the next khubd run" is misleading. khubd may run several > > times, for other purposes, before it gets around to handling this > > particular port. "As soon as possible" is better. > > *nod* > > > > >> 1/ The device may need a reset on power-session loss, without this > >> change port power-on recovery exposes khubd to scenarios that > >> usb_port_resume() is set to handle. > > > > What scenarios? > > Prior to port power control the only time a power session would be > lost is during dpm_suspend of the hub. In that scenario > usb_port_resume() is guaranteed to be called prior to khubd running > for that port. With this change we wakeup the child device prior to > khubd running again for this port. > > > Wouldn't khubd handle them by doing a runtime resume > > anyway? > > khubd will only do a runtime resume if the portstatus / portchange > indicates a suspend state. In the case of port power control we are > not coming from a hub-port-suspend state. > > >> Also, testing showed that USB3 > >> devices that are not reset on power-session loss may eventually > >> downgrade their connection to the USB2 pins. > > > > What do you mean by "eventually"? The delay before the reset is too > > long? Or the port power-cycles too many times? > > Empirically port power cycles too many times seems to be trigger. > > >> 2/ This mechanism rate limits port power toggling. The minimum port > >> power on/off period is now gated by the child device suspend/resume > >> latency. This mitigates devices downgrading their connection on > >> perceived instability of the host connection. This ratelimiting is > >> really only relevant to port power control testing, but it is a nice > >> side effect of closing the above race. > > > > What race? > > The race of khubd for the given port running while a usb_port_resume() > event is pending. > > > This reminds me... In the case of a tier mismatch, is it possible that > > the USB-3 root hub will be capable of switching off power to its ports > > but the integrated USB-2 hub won't? In that situation, the device is > > virtually certain to change over to high speed. > > Good point I should be gating USB3 port power off on the merged > wHubCharacteristics of the peered ports. > > >> 3/ Going forward if we find that power-session recovery requires > >> warm-resets (http://marc.info/?t=138659232900003&r=1&w=2) that is > >> something usb_port_resume() can drive and handle before khubd's next > >> evaluation of the portstatus. > > > > Isn't this more or less the case right now? In the current code, the > > only reason the hub driver would look at a port that had recently been > > powered back on is if the attached device was undergoing a runtime > > resume. > > This is more or less the case right now, yes. > > >> 4/ If the device *was* disconnected the only time we'll know for sure is > >> after a failed resume, so it's necessary for > >> usb_port_runtime_resume() to expedite a usb_port_resume() to clean up > >> the removed device. > > > > I don't follow the reasoning. Even granting that we want to find out > > about the disconnection, we have already waited an indefinitely long > > time for the port to be powered back on. What harm is there in waiting > > another indefinitely long time for the device to be runtime-resumed? > > Least surprise for the user. Turning on a port means that hotplug > detection is again enabled for the port, it is surprising that devices > that were removed while the port was off are not disconnected until > they are attempted to be used. As a user "why would I try to use a > device I removed from the system?" > > > Also, usb_port_runtime_resume _already_ includes code to check for a > > disconnection. Currently all it does is log a debugging message if the > > device is gone, but it could easily do more. > > That detection is buggy and we can't rely on it until > usb_port_resume() determines the disconnection. It's buggy because > the port may require a warm reset, and we leave that to be done by > usb_port_resume(). Along with the proposed changes to the code (omitted from this reply because we are in agreement on them), please include the ideas discussed above in the revised patch description. They will make it a lot clearer. 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
