On Thu, 1 May 2014, Dan Williams wrote:
> On Mon, Apr 28, 2014 at 1:29 PM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> > On Wed, 19 Mar 2014, Dan Williams wrote:
> [..]
> >> --- 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;
> >
>
> So I spent a day debugging this approach as it de-stabilized my tests,
> resume failures and disconnects, relative to the
> usb_wakeup_notification approach. I got it stabilized again, but I
What was the problem? Or problems?
> have effectively open coded the usb_wakeup_notification(). To me
Why did you have to open-code it?
> "abusing" usb_wakeup_notification() vs open coding it is a distinction
> without a difference.
>
> pm_request_resume() arranges for the child device wakeup to run
> asynchronously with respect to khubd outside of usb_lock_device().
Well, one of the major points of this exercise is that the resume _has_
to be asynchronous with respect to the usb_port_runtime_resume()
routine. Letting the PM work queue handle it seems the most reasonable
approach, since that's what it was intended for. Khubd, on the other
hand, was not meant for this sort of thing. Using it for this purpose
is, in its own way, a kludge.
> In comparison, waking up the device in port_event() means:
> 1/ the hub is powered (as we are within usb_autopm_get_interface() for the hub)
What difference does it make whether or not the hub is at full power?
Doing a runtime resume on the child device will automatically wake up
the hub in any case.
On the other hand, it would be nice to avoid having the hub go back
into runtime suspend when usb_port_runtime_resume() calls
usb_autopm_put_interface(), only to be woken up again a moment later.
Preventing that would require some extra code somewhere -- but not
necessarily in khubd.
> 2/ portstatus has already been read for the port_event()
Again, so what? Resuming the child device will require more reads of
the port status regardless.
> 3/ we can take usb_lock_device().
Yes, that is a significant difference. It isn't really necessary to
take this lock, as far as I know -- I have been including it whenever
possible more as a form of "voodoo" programming than for any rational
reason. As far as I can tell, it isn't possible to guarantee that the
lock is held _every_ time a USB device is resumed.
> I can leave it open coded if you like, but I'd just as soon re-use the
> existing wakeup notification infrastructure. Maybe a comment to
> clarify the distinction between remote and port pm runtime induced
> resume of the device?
Without knowing why you had to copy the logic of
usb_wakeup_notification, I can't respond to this.
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
