On Tue, Apr 29, 2014 at 8:12 AM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> On Mon, 28 Apr 2014, Dan Williams wrote:
>
>> > 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.
>
> Another possibility is the user writes "on" to the port's power/control
> file while the port and device are suspended.
>
>> > 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?
>
> It does change the power state without changing the usage count, but
> this isn't a subversion. Runtime PM allows a device to be at full
> power with a usage count of 0 or at low power with a usage count > 0.
>
> (It's not hard to think of ways of doing these things. For example,
> pm_runtime_resume() will power-up a suspended device without increasing
> the usage count, and pm_runtime_get_noresume() will increase the usage
> count without powering-up a suspended device.)
>
> The only promise made by the runtime PM core is that it won't issue a
> pm_suspend callback if the usage count is positive.
>
>> usb_wakeup_notification() also handles putting the device back to
>> sleep if there is nothing to do.
>
> This happens automatically with any kind of runtime resume, because USB
> devices use autosuspend.
True, true, true. I forgot that this resume was backstopped by the
timer and an implied call to rpm_suspend(RPM_AUTO), so no worries
about staying active indefinitely. I'll proceed with the cleanup.
>
>> > 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}.
>
> You mean, add pm_runtime_get_sync(&port_dev->dev) before the
> device_del() call and pm_runtime_put(&port_dev->dev) after the *pdev =
> NULL statement? That seems reasonable.
>
>> ...and as I go to add this I notice that prior to the "use
>> pm_request_resume" suggestion we don't de-reference port_dev->child in
>> usb_port_runtime_resume(). This realization plus the usage count
>> tracking that usb_remote_wakeup() affords is leaning me towards
>> leaving the "force wakeup" mechanism as is for the upcoming re-post.
>
> The usage count tracking isn't an issue; that's the way autosuspend is
> meant to work. Also, you're abusing usb_wakeup_notification() by
> calling it for something that wasn't triggered by a remote wakeup
> request from the device.
>
> However, either way we still have bad interactions between
> hub_{quiesce|activate}() and usb_port_runtime_{suspend|resume}().
> Consider, for example, what happens to the ports' power states when the
> hub is reset.
>
You mean in terms of pm_runtime state getting out of sync with the
hardware state? We do have (admittedly ugly in my opinion) a check of
the power state in hub_power_on() so if port power was runtime removed
hub_activate() will honor that and not restore power. And a new
pm_runtime_get_sync in usb_disconnect() will handle the hub_quiesce()
interfaction. Am I missing something?
--
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
