On Thu, 2014-05-08 at 15:14 -0400, Alan Stern wrote:
> On Thu, 8 May 2014, Dan Williams wrote:
>
> > On Thu, May 8, 2014 at 11:09 AM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> > > On Thu, 8 May 2014, Dan Williams wrote:
> > >
> > >> On Thu, May 8, 2014 at 9:09 AM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> > >> > On Thu, 8 May 2014, Dan Williams wrote:
> > >> >
> > >> >> > I don't understand this last part. Why do we need to guarantee the
> > >> >> > child device has recovered from power loss? Why not proceed the same
> > >> >> > way we do now when the child is suspended?
> > >> >>
> > >> >> Two reasons I believe:
> > >> >>
> > >> >> 1/ The child may be gone, and usb_port_resume() will mark it for disconnect
> > >> >>
> > >> >> 2/ Currently port_event() knows how to handle suspended devices
> > >> >> (USB_PORT_STAT_C_SUSPEND), but in the case of power loss recovery the
> > >> >> status and change bits are different. I figure why special case
> > >> >> port_event()? Just make it so it handles all the same cases that are
> > >> >> presented when the port does not lose power.
> > >> >
> > >> > How much special casing would really be needed?
> > >>
> > >> Don't know. Is the forced wakeup really that onerous, vs the risk of
> > >> touch established code? I've broken the port_event() path enough
> > >> times through this exercise that I'd want a driving reason beyond "why
> > >> not?".
> > >
> > > As far as I can see, it looks like no special casing is needed. Now, I
> > > haven't gone through all the changes introduced by the patch series to
> > > make sure, but it seems that all the various port status tests in
> > > port_event() will see everything appearing normal, because that's how
> > > hub_port_debounce_be_connected() -- or whatever its current name is --
> > > will leave the port.
> > >
> > > Thus, nothing will happen up until the place in
> > > hub_port_connect_change() headed by the "Try to resuscitate an existing
> > > device" comment. At that point we will resume the child device. That
> > > is, assuming khubd thinks it has any reason for for calling
> > > hub_port_connect_change(), which it probably won't.
> > >
> > > So at first sight, it appears that nothing would need to be changed. I
> > > suggest you try it and see.
> > >
> >
> > To be clear what I am trying to remove is the udev runtime_barrier
> > prior to the port_event, right? We still agree that
> > pm_request_resume() needs to be there in the port_dev resume path?
>
> Yes, pm_request_resume() is necessary. But setting wakeup_bits and
> calling usb_kick_khubd() aren't, which means the code you added to
> hub_events() with the comment "Revalidate the device if it was
> requested by usb_port_runtime_resume" can be left out.
>
Testing looks good. In my objection to this approach I neglected to
consider that the port_status_lock now protects us from port_event()
usb_port_{suspend_resume} collisions. I have updated the changelog
accordingly.
8<-----------
Subject: usb: resume (wakeup) 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 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 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.
Cc: Rafael J. Wysocki <rjw@xxxxxxxxxxxxx>
Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
---
drivers/usb/core/hub.c | 22 +++++++++++++++-------
drivers/usb/core/port.c | 9 ++++++++-
2 files changed, 23 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index b9aff5c6f855..7cd71e338104 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -2057,9 +2057,11 @@ static void hub_free_dev(struct usb_device *udev)
*/
void usb_disconnect(struct usb_device **pdev)
{
- struct usb_device *udev = *pdev;
- struct usb_hub *hub = usb_hub_to_struct_hub(udev);
- int i;
+ int i;
+ struct usb_device *udev = *pdev;
+ int port1 = udev->portnum;
+ struct usb_port *port_dev = NULL;
+ struct usb_hub *hub = usb_hub_to_struct_hub(udev);
/* mark the device as inactive, so any further urb submissions for
* this device (and any of its children) will fail immediately.
@@ -2086,15 +2088,18 @@ void usb_disconnect(struct usb_device **pdev)
usb_hcd_synchronize_unlinks(udev);
if (udev->parent) {
- int port1 = udev->portnum;
struct usb_hub *hub = usb_hub_to_struct_hub(udev->parent);
- struct usb_port *port_dev = hub->ports[port1 - 1];
+ port_dev = hub->ports[port1 - 1];
sysfs_remove_link(&udev->dev.kobj, "port");
sysfs_remove_link(&port_dev->dev.kobj, "device");
- if (test_and_clear_bit(port1, hub->child_usage_bits))
- pm_runtime_put(&port_dev->dev);
+ /*
+ * As usb_port_runtime_resume() de-references udev, make
+ * sure no resumes occur during removal
+ */
+ if (!test_and_set_bit(port1, hub->child_usage_bits))
+ pm_runtime_get_sync(&port_dev->dev);
}
usb_remove_ep_devs(&udev->ep0);
@@ -2116,6 +2121,9 @@ void usb_disconnect(struct usb_device **pdev)
*pdev = NULL;
spin_unlock_irq(&device_state_lock);
+ if (port_dev && test_and_clear_bit(port1, hub->child_usage_bits))
+ pm_runtime_put(&port_dev->dev);
+
hub_free_dev(udev);
put_device(&udev->dev);
diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index 8b1655700104..62036faf56c0 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -76,6 +76,7 @@ static int usb_port_runtime_resume(struct device *dev)
struct usb_device *hdev = to_usb_device(dev->parent->parent);
struct usb_interface *intf = to_usb_interface(dev->parent);
struct usb_hub *hub = usb_hub_to_struct_hub(hdev);
+ struct usb_device *udev = port_dev->child;
struct usb_port *peer = port_dev->peer;
int port1 = port_dev->portnum;
int retval;
@@ -97,7 +98,7 @@ static int usb_port_runtime_resume(struct device *dev)
usb_autopm_get_interface(intf);
retval = usb_hub_set_port_power(hdev, hub, port1, true);
msleep(hub_power_on_good_delay(hub));
- if (port_dev->child && !retval) {
+ if (udev && !retval) {
/*
* Attempt to wait for usb hub port to be reconnected in order
* to make the resume procedure successful. The device may have
@@ -109,6 +110,12 @@ static int usb_port_runtime_resume(struct device *dev)
dev_dbg(&port_dev->dev, "can't get reconnection after setting port power on, status %d\n",
retval);
retval = 0;
+
+ /* Force the child awake to revalidate after the power loss. */
+ if (!test_and_set_bit(port1, hub->child_usage_bits)) {
+ pm_runtime_get_noresume(&port_dev->dev);
+ pm_request_resume(&udev->dev);
+ }
}
usb_autopm_put_interface(intf);
--
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
