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.
> > 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.
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
