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.willi...@intel.com>
> ---
> 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
