On Mon, 2014-05-12 at 15:14 -0400, Alan Stern wrote: > On Mon, 12 May 2014, Dan Williams wrote: > > > Fixed. And dropped "wakeup" out of the patch subject. > > > > > There's a nasty bug here. I'll let you figure it out for yourself. :-) > > > Hint: Hiding a variable by declaring another local variable with the > > > same name in an inner block often leads to trouble. > > > > Added hub_disconnect_children() and a note in the changelog. > > > > 8<--------- > > Subject: usb: resume child device when port is powered on > > > > From: Dan Williams <dan.j.williams@xxxxxxxxx> > > > > Unconditionally wake up the child device when the power session is > > recovered. > > > > This addresses 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 implementation simply uses pm_request_resume() to wake the > > device and relies on the port_dev->status_lock to prevent any collisions > > between khubd and usb_port_resume(). > > > > 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 rpm_resume() case pm_request_resume() is used to > > request re-validation of the device, and if it happens to collide with a > > khubd run we rely on the port_dev->status_lock to synchronize those > > operations. > > > > Besides testing, the primary scenario where this mechanism is expected > > to be triggered is when the user changes the port power policy > > (control/pm_qos_no_poweroff, or power/control). Each time power is > > enabled want to revalidate the child device, where the revalidation is > > handled by usb_port_resume(). > > > > Given that this arranges for port_dev->child to be de-referenced in > > usb_port_runtime_resume() we need to make sure not to collide with > > usb_disconnect() that frees the usb_device. To this end we hold the > > port active with the "child_usage" reference across the disconnect > > event. Subsequently, the need to access hub->child_usage_bits lead to > > the creation of hub_disconnect_children() to remove any ambiguity of > > which "hub" is being acted on in usb_disconnect() (prompted-by sharp > > eyes from Alan). > > > > Cc: Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> > > Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx> > > This version looks good. > > Acked-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> > > Is that everything? Or did we skip a few patches in the middle? It's > hard to keep track. Sorry for the delay had some medically induced latency earlier this week, feeling better. My ack tracker script currently says: ACK [01/19] USB: mutual exclusion for resetting a hub and power-managing a port ACK [02/19] usb: disable port power control if not supported in wHubCharacteristics ACK [03/19] usb: rename usb_port device objects ACK [04/19] usb: cleanup setting udev->removable from port_dev->connect_type ACK [05/19] usb: assign default peer ports for root hubs ACK [06/19] usb: assign usb3 external hub port peers ACK [07/19] usb: find internal hub tier mismatch via acpi ACK [08/19] usb: sysfs link peer ports [09/19] usb: make usb_port flags atomic, rename did_runtime_put to child_usage ACK [10/19] usb: block suspension of superspeed port while hispeed peer is active ACK [11/19] usb: don't clear FEAT_C_ENABLE on usb_port_runtime_resume failure ACK [12/19] usb: usb3 ports do not support FEAT_C_ENABLE [13/19] usb: refactor port handling in hub_events() [14/19] usb: synchronize port poweroff and khubd [15/19] usb: introduce port status lock ACK [16/19] usb: hub_handle_remote_wakeup() depends on CONFIG_PM_RUNTIME=y ACK [17/19] usb: resume child device when port is powered on ack [18/19] usb: force warm reset to break link re-connect livelock [19/19] usb: documentation for usb port power off mechanisms ...where that lower case "ack" means acked, but not by you. -- 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
