On Wed, 13 Apr 2011, Sarah Sharp wrote:
> On Wed, Apr 13, 2011 at 05:56:48PM -0400, Alan Stern wrote:
> > On Wed, 13 Apr 2011, Sarah Sharp wrote:
> >
> > > Cai, will you try the following patch with Andiry's work-around patch,
> > > but not with Alan's patch?
> > >
> > > This solves the 2nd hibernate failure for me. I think Alan's patch was
> > > masking underlying issues when the USB 3.0 roothub was not marked as
> > > having lost power. The USB core really thought the USB 3.0 hard drive
> > > was still there, which is why can_submit was set. Alan, do you think
> > > your patch is still needed?
> >
> > No, it's not needed, but something else is. The patch was written to
> > solve a real problem: What should we do if a device is unplugged during
> > a system sleep transition? Since khubd gets frozen before the
> > transition starts, the device can't be unregistered -- and it can't be
> > suspended.
> >
> > What do you think of the patch below instead? It actually does a
> > better job of solving the underlying problem... although it does a
> > worse job of reducing the code size. :-)
> >
> > Alan Stern
> >
> >
> >
> > Index: usb-2.6/drivers/usb/core/hub.c
> > ===================================================================
> > --- usb-2.6.orig/drivers/usb/core/hub.c
> > +++ usb-2.6/drivers/usb/core/hub.c
> > @@ -2296,6 +2296,16 @@ int usb_port_suspend(struct usb_device *
> > USB_DEVICE_REMOTE_WAKEUP, 0,
> > NULL, 0,
> > USB_CTRL_SET_TIMEOUT);
> > +
> > + /* Failure to suspend is okay if the port isn't enabled */
> > + if (!(msg.event & PM_EVENT_AUTO)) {
> > + u16 pstatus, pchange;
> > +
> > + pstatus = 0;
> > + hub_port_status(hub, port1, &pstatus, &pchange);
> > + if (!(pstatus & USB_PORT_STAT_ENABLE))
> > + status = 0;
> > + }
> > } else {
> > /* device has up to 10 msec to fully suspend */
> > dev_dbg(&udev->dev, "usb %ssuspend\n",
On thinking about this more, I've decided that checking the port status
isn't needed here. If the port is being suspended because of a system
sleep, we should always report success. Like this:
--- usb-2.6.orig/drivers/usb/core/hub.c
+++ usb-2.6/drivers/usb/core/hub.c
@@ -2296,6 +2296,10 @@ int usb_port_suspend(struct usb_device *
USB_DEVICE_REMOTE_WAKEUP, 0,
NULL, 0,
USB_CTRL_SET_TIMEOUT);
+
+ /* System sleep transitions should never fail */
+ if (!(msg.event & PM_EVENT_AUTO))
+ status = 0;
} else {
/* device has up to 10 msec to fully suspend */
dev_dbg(&udev->dev, "usb %ssuspend\n",
Once this change is made, there'll be no reason to keep the test in
hub_suspend(). Which answers your question in the second paragraph of
this email.
> So we'll only return a successful status if this isn't an auto-suspend,
> and the port is disabled, correct?
With the revised patch, we'll return a successful status for all
system suspend and hibernate calls. For autosuspend, we'll return a
successful status only if the port suspend actually succeeded.
> Remember that USB 3.0 ports can't be
> disabled, because USB 3.0 hubs don't define a ClearPortFeature
> PORT_ENABLE like USB 2.0 hubs do.
Not true. USB-3 ports can't be disabled by the host, but they are
automatically disabled when the cable is unplugged. However this is a
moot point now...
> This won't help for any USB 3.0
> devices.
It will help if a USB-3 device is unplugged (or electrically
disconnected) while a system sleep transition is underway. See
http://marc.info/?l=linux-usb&m=129771116401727&w=2
for an example where this actually happened with a USB-2 device. In
the attached log, the suspend starts at 63.785917 and the disconnect
occurs at 64.078847, causing the suspend to fail at 64.179709.
(Presumably the Bluetooth device at 1-1.4 was being autoresumed in
order to change its remote wakeup setting.)
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
