On Mon, Apr 28, 2014 at 1:29 PM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote: > 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. I guess it could be considered testing, but changing the poweroff policy of the port is one case where it matters to immediately resume the child. A user could (even though it is warned against in Documentation/usb/power-management.txt) unplug a device while the port is powered off. Without the forced resume of the child we won't notice the unplug event until much later. So, it's a "the world may have changed, revalidate" operation. I'll include text along these lines in the update. >> 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); > } Doesn't this subvert usb_auto{resume|suspend} by changing the power state of the device relative to the usage count? usb_wakeup_notification() also handles putting the device back to sleep if there is nothing to do. > > 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). Yes, I've actually already added a patch to the series to do this, but kept the field in struct usb_port. > 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? Yes, I think we simply need to add pm_runtime_{get|put}(&port_dev->dev) to guarantee that port_dev->child is always safe to de-reference in usb_port_runtime{suspend|resume}. -- 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
