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(). > >> 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. In fact++, usb_wakeup_notification() is very similar to what usb_port_runtime_resume() wants to do, so I can re-use that as well. > >> 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. Yes, all the more reason to just use usb_wakeup_notification() > > Alan Stern > Thanks Alan. -- 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
