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. > 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. > 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? Wouldn't khubd handle them by doing a runtime resume anyway? > 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? > 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? 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. > 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. > 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? 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. > 1, 2, and 4 are not a problem in the system resume case because, > although the power-session is lost, khubd is frozen until after device > resume. For the runtime pm case we can use runtime-pm-synchronization > to guarantee the same sequence of events. When a ->resume_child request > is set in usb_port_runtime_resume() the port device is in the > RPM_RESUMING state. khubd executes a pm_runtime_barrier() on the port > device to flush the port recovery, holds the port active while it > resumes the child, and completes child device resume before acting on > the current portstatus. > > Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx> > --- > drivers/usb/core/hub.c | 17 +++++++++++++++++ > drivers/usb/core/hub.h | 2 ++ > drivers/usb/core/port.c | 7 +++++++ > 3 files changed, 26 insertions(+), 0 deletions(-) > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c > index a310028e210d..9a505978ab92 100644 > --- a/drivers/usb/core/hub.c > +++ b/drivers/usb/core/hub.c > @@ -4767,6 +4767,23 @@ static void port_event(struct usb_hub *hub, int port1) > pm_runtime_barrier(&port_dev->dev); > usb_lock_port(port_dev); > do if (pm_runtime_active(&port_dev->dev)) { > + > + /* service child resume requests on behalf of > + * usb_port_runtime_resume() > + */ > + if (port_dev->resume_child && udev) { > + usb_unlock_port(port_dev); > + > + usb_lock_device(udev); > + usb_autoresume_device(udev); > + usb_autosuspend_device(udev); > + usb_unlock_device(udev); > + > + pm_runtime_put(&port_dev->dev); > + usb_lock_port(port_dev); > + } > + port_dev->resume_child = 0; This bears a close resemblance to hub_handle_remote_wakeup. It ought to be possible to use the same subroutine for both cases. In fact, looking more closely, it appears that patch 8 got rid of all uses of wakeup_change. This seems like a good reason to use it. > diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c > index be9c4486816a..be1e18355fec 100644 > --- a/drivers/usb/core/port.c > +++ b/drivers/usb/core/port.c > @@ -105,6 +105,13 @@ static int usb_port_runtime_resume(struct device *dev) > if (retval < 0) > dev_dbg(&port_dev->dev, "can't get reconnection after setting port power on, status %d\n", > retval); > + > + /* keep this port awake until we have had a chance to recover > + * the child > + */ > + pm_runtime_get_noresume(&port_dev->dev); > + port_dev->resume_child = 1; > + usb_kick_khubd(hdev); > retval = 0; > } You also need to set the appropriate bit in hub->wakeup_bits. Otherwise hub_events will have no reason to call port_event for this port. 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
