On Mon, Apr 28, 2014 at 1:29 PM, Alan Stern <st...@rowland.harvard.edu> 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.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);
> }
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
