On Wed, 19 Mar 2014, Dan Williams wrote: > Unconditionally wake up the child device when the power session is > recovered. > > This address the following scenarios: > > 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. 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 as soon as possible (prior to khubd running again for this > port). > > Although khubd has facilities to wake a child device it will only do > so 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. This implemenation extends usb_wake_notification() for the > port rpm_resume case. > > 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. Empirically 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. Namely, the race of khubd for > the given port running while a usb_port_resume() event is pending. > > 3/ Going forward we are finding that power-session recovery requires > warm-resets (http://marc.info/?t=138659232900003&r=1&w=2). This > mechanism allows for warm-resets to be requested at the same point in > the resume path for hub dpm_suspend power session losses, or port > rpm_suspend power session losses. > > 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. The > reasoning for this is "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?" > > 1, 2, and 4 are not a problem in the system dpm_resume case because, > although the power-session is lost, khubd is frozen until after device > resume. For the port rpm_resume use runtime-pm-synchronization to > guarantee the same sequence of events. When a usb_wakeup_notification() > is invoked the port device is in the RPM_RESUMING state. khubd in turn > performs 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. Can you include a brief description of situations in which this would be needed, i.e., when something would runtime-resume the port without also resuming the child device? Testing, sure, but not much else comes to mind. > Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx> > --- > drivers/usb/core/hub.c | 40 +++++++++++++++++++++++++++++----------- > drivers/usb/core/port.c | 2 ++ > 2 files changed, 31 insertions(+), 11 deletions(-) > --- a/drivers/usb/core/port.c > +++ b/drivers/usb/core/port.c > @@ -104,6 +104,8 @@ 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); > + > + usb_wakeup_notification(hdev, port1); > retval = 0; > } I think this can be simplified a lot. At first glance, almost no changes to hub.c will be necessary if instead you insert: if (port_dev->did_runtime_put) { port_dev->did_runtime_put = false; pm_runtime_get_noresume(&port_dev->dev); pm_request_resume(&port_dev->child->dev); } Then in usb_port_resume(), simply interchange these two lines: status = pm_runtime_get_sync(&port_dev->dev); port_dev->did_runtime_put = false; Unfortunately, these new references to port_dev->did_runtime_put will race with the existing references in hub.c. The simplest solution seems to be to convert the did_runtime_put values to a set of atomic bitflags stored in the hub structure (like hub->change_bits). But this got me thinking... It looks like the reference to port_dev->child in usb_port_runtime_resume() already races with the line *pdev = NULL; in usb_disconnect(). We need to make sure that a port runtime-resume is mutually exclusive with child device removal. (Consider, for example, what happens if a thread does a runtime resume on a port and at the same time, the hub is unplugged.) Any ideas? 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
