Re: [PATCH v7 14/16] usb: resume (wakeup) child device when port is powered on

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux