USB rejecting sleep

USB rejecting sleep

[Benjamin Herrenschmidt]

Hi David, Alan !

What exactly changed in the recent USB stacks that is causing it to
abort system suspend much more often ? I'm getting lots of user reports
with 2.6.15-rc5 saying that they can't put their internal laptops to
sleep, apparently because a driver doesn't have a suspend method
(internal bluetooth in this case).

It's never been mandatory so far for all drivers of all connected
devices to have a suspend method... didn't we decide back then that
disconneting those was the right way to go ?

Any reason we are rejecting the sleep process for these currently ? A
locking issue that makes disconnecting not yet feasible ? What changed
from the previous version where that worked ?

Cheers,
Ben.

Re: USB rejecting sleep

[David Brownell]

On Sunday 18 December 2005 12:27 pm, Benjamin Herrenschmidt wrote:
> Hi David, Alan !
> 
> What exactly changed in the recent USB stacks that is causing it to
> abort system suspend much more often ? I'm getting lots of user reports
> with 2.6.15-rc5 saying that they can't put their internal laptops to
> sleep, apparently because a driver doesn't have a suspend method
> (internal bluetooth in this case).

Which I hope _did_ generate a bug report to the maintainer of that
bluetooth code.  :)


> It's never been mandatory so far for all drivers of all connected
> devices to have a suspend method... didn't we decide back then that
> disconneting those was the right way to go ?

Right, but the system never stopped self-deadlocking when we did the
disconnect at suspend time.  My notes say "driver core suspend()
calls are made with dev->sem held, so usb_driver_release_interface()
always deadlocks when they try to claim the same lock" and presumably
that's still true.

I guess I didn't realize the consequence of not fixing that as part
of the other PM updates, once I found that the "most natural" fix
was (still?) not possible.


> Any reason we are rejecting the sleep process for these currently ? A
> locking issue that makes disconnecting not yet feasible ? What changed
> from the previous version where that worked ?

The current kernels tighten up the suspend processing and offloaded lots
of stuff to the driver core.  Previous kernels didn't have code that
could care about such stuff, at least without USB_SUSPEND enabled.

So the issue is now how to handle this error case.  I think it should
be possible to just mark the device as disconnected right as soon as
we notice it can't be suspended; resume processing will do the work,
it already does so for real disconnect.  And disconnect paths in USB
drivers are now pretty solid.

- Dave

Re: USB rejecting sleep

[Benjamin Herrenschmidt]

On Sun, 2005-12-18 at 12:58 -0800, David Brownell wrote:
> On Sunday 18 December 2005 12:27 pm, Benjamin Herrenschmidt wrote:
> > Hi David, Alan !
> > 
> > What exactly changed in the recent USB stacks that is causing it to
> > abort system suspend much more often ? I'm getting lots of user reports
> > with 2.6.15-rc5 saying that they can't put their internal laptops to
> > sleep, apparently because a driver doesn't have a suspend method
> > (internal bluetooth in this case).
> 
> Which I hope _did_ generate a bug report to the maintainer of that
> bluetooth code.  :)

I'm working on it :)

> Right, but the system never stopped self-deadlocking when we did the
> disconnect at suspend time.  My notes say "driver core suspend()
> calls are made with dev->sem held, so usb_driver_release_interface()
> always deadlocks when they try to claim the same lock" and presumably
> that's still true.

Ok.

> I guess I didn't realize the consequence of not fixing that as part
> of the other PM updates, once I found that the "most natural" fix
> was (still?) not possible.

Makes sense. Just wanted to be sure.

> So the issue is now how to handle this error case.  I think it should
> be possible to just mark the device as disconnected right as soon as
> we notice it can't be suspended; resume processing will do the work,
> it already does so for real disconnect.  And disconnect paths in USB
> drivers are now pretty solid.

Ok, and so at the parent hub level, we can do either the actual suspend
of the port, or even power the port off if the device got into this
"latent disconnect" state.

Ben.
 

Re: USB rejecting sleep

[Greg KH]

On Mon, Dec 19, 2005 at 07:27:21AM +1100, Benjamin Herrenschmidt wrote:
> Hi David, Alan !
> 
> What exactly changed in the recent USB stacks that is causing it to
> abort system suspend much more often ? I'm getting lots of user reports
> with 2.6.15-rc5 saying that they can't put their internal laptops to
> sleep, apparently because a driver doesn't have a suspend method
> (internal bluetooth in this case).
> 
> It's never been mandatory so far for all drivers of all connected
> devices to have a suspend method... didn't we decide back then that
> disconneting those was the right way to go ?

Yes it is, and I have a patch in my tree now that fixes this up and
keeps the suspend process working properly for usb drivers that do not
have a suspend function.

Hm, I wonder if it should go in for 2.6.15?

thanks,

greg k-h

Re: USB rejecting sleep

[Marcel Holtmann]

Hi Ben,

> > > What exactly changed in the recent USB stacks that is causing it to
> > > abort system suspend much more often ? I'm getting lots of user reports
> > > with 2.6.15-rc5 saying that they can't put their internal laptops to
> > > sleep, apparently because a driver doesn't have a suspend method
> > > (internal bluetooth in this case).
> > 
> > Which I hope _did_ generate a bug report to the maintainer of that
> > bluetooth code.  :)
> 
> I'm working on it :)

did I mention that this driver really needs a rewrite :(

It is full of ugly hacks around the URB structure. The use of the URBs
for the bulk and isoc endpoints is not really how it should look like.
It will consume USB bandwidth even if there are not active data or audio
connections from this device. I am still curious that it is still
working (except for suspend).

Regards

Marcel

Re: USB rejecting sleep

[Benjamin Herrenschmidt]

On Sun, 2005-12-18 at 13:50 -0800, Greg KH wrote:

> Yes it is, and I have a patch in my tree now that fixes this up and
> keeps the suspend process working properly for usb drivers that do not
> have a suspend function.
> 
> Hm, I wonder if it should go in for 2.6.15?

Do you have an URL I can send to those users to test ?

Thanks !
Ben.

Re: USB rejecting sleep

[Greg KH]

On Mon, Dec 19, 2005 at 09:13:50AM +1100, Benjamin Herrenschmidt wrote:
> On Sun, 2005-12-18 at 13:50 -0800, Greg KH wrote:
> 
> > Yes it is, and I have a patch in my tree now that fixes this up and
> > keeps the suspend process working properly for usb drivers that do not
> > have a suspend function.
> > 
> > Hm, I wonder if it should go in for 2.6.15?
> 
> Do you have an URL I can send to those users to test ?

Here's the patch itself, feel free to spread it around.

It's also at:
  kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/patches/usb/usbcore-allow-suspend-resume-even-if-drivers-don-t-support-it.patch

thanks,

greg k-h



>From stern@rowland.harvard.edu Wed Dec 14 09:39:01 2005
Date: Wed, 14 Dec 2005 12:22:17 -0500 (EST)
From: Alan Stern <stern@rowland.harvard.edu>
To: Greg KH <greg@kroah.com>
cc: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2005@gmx.net>
Subject: [PATCH] usbcore: allow suspend/resume even if drivers don't support it
Message-ID: <Pine.LNX.4.44L0.0512141215510.7421-100000@iolanthe.rowland.org>

This patch (as618) changes usbcore to prevent derailing the
suspend/resume sequence when a USB driver doesn't include support for
it.  This is a workaround rather than a true fix; the core needs to be
changed so that URB submissions from suspended drivers can be refused
and outstanding URBs cancelled.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

---
 drivers/usb/core/usb.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

--- gregkh-2.6.orig/drivers/usb/core/usb.c
+++ gregkh-2.6/drivers/usb/core/usb.c
@@ -1029,7 +1029,8 @@ static int usb_generic_suspend(struct de
 			mark_quiesced(intf);
 	} else {
 		// FIXME else if there's no suspend method, disconnect...
-		dev_warn(dev, "no %s?\n", "suspend");
+		dev_warn(dev, "no suspend for driver %s?\n", driver->name);
+		mark_quiesced(intf);
 		status = 0;
 	}
 	return status;
@@ -1057,8 +1058,10 @@ static int usb_generic_resume(struct dev
 	}
 
 	if ((dev->driver == NULL) ||
-	    (dev->driver_data == &usb_generic_driver_data))
+	    (dev->driver_data == &usb_generic_driver_data)) {
+		dev->power.power_state.event = PM_EVENT_FREEZE;
 		return 0;
+	}
 
 	intf = to_usb_interface(dev);
 	driver = to_usb_driver(dev->driver);
@@ -1078,7 +1081,7 @@ static int usb_generic_resume(struct dev
 			mark_quiesced(intf);
 		}
 	} else
-		dev_warn(dev, "no %s?\n", "resume");
+		dev_warn(dev, "no resume for driver %s?\n", driver->name);
 	return 0;
 }
 

Re: USB rejecting sleep

[Alan Stern]

On Sun, 18 Dec 2005, David Brownell wrote:

> > It's never been mandatory so far for all drivers of all connected
> > devices to have a suspend method... didn't we decide back then that
> > disconneting those was the right way to go ?
> 
> Right, but the system never stopped self-deadlocking when we did the
> disconnect at suspend time.  My notes say "driver core suspend()
> calls are made with dev->sem held, so usb_driver_release_interface()
> always deadlocks when they try to claim the same lock" and presumably
> that's still true.
> 
> I guess I didn't realize the consequence of not fixing that as part
> of the other PM updates, once I found that the "most natural" fix
> was (still?) not possible.

There hasn't been a lot of discussion about this.  You know, it probably 
wouldn't be all that hard to add an entry point in the driver core for 
unbinding drivers while the caller holds dev->sem.  In fact, such a 
routine already exists (__device_release_driver) but it's not exported.

There is the little problem that when a USB driver is unbound from an
interface, the USB device's semaphore is supposed to be locked as well as
the interface's.  Fortunately, I think the only driver for which this
matters is the hub driver, which _does_ have a suspend method.


> > Any reason we are rejecting the sleep process for these currently ? A
> > locking issue that makes disconnecting not yet feasible ? What changed
> > from the previous version where that worked ?
> 
> The current kernels tighten up the suspend processing and offloaded lots
> of stuff to the driver core.  Previous kernels didn't have code that
> could care about such stuff, at least without USB_SUSPEND enabled.

The way the failures actually occur is a little bizarre.  You would think
that things would go wrong as soon as the suspend method is found to be
missing.  But that's not what happens.  For some reason the suspend call
_succeeds_, but doesn't mark the interface as actually suspended.  Then
later on, when the whole USB device is getting suspended, usbcore sees
that there's still an active interface and fails the call.

> So the issue is now how to handle this error case.  I think it should
> be possible to just mark the device as disconnected right as soon as
> we notice it can't be suspended; resume processing will do the work,
> it already does so for real disconnect.  And disconnect paths in USB
> drivers are now pretty solid.

Do you think that might be a little drastic?  Disconnect an entire device 
just because one of its interfaces can't be suspended?

Admittedly, we haven't run across very many composite devices with 
independent drivers for various interfaces...  But still, the principle 
isn't good.

Alan Stern

Re: USB rejecting sleep

[Benjamin Herrenschmidt]

On Sun, 2005-12-18 at 14:25 -0800, Greg KH wrote:
> On Mon, Dec 19, 2005 at 09:13:50AM +1100, Benjamin Herrenschmidt wrote:
> > On Sun, 2005-12-18 at 13:50 -0800, Greg KH wrote:
> > 
> > > Yes it is, and I have a patch in my tree now that fixes this up and
> > > keeps the suspend process working properly for usb drivers that do not
> > > have a suspend function.
> > > 
> > > Hm, I wonder if it should go in for 2.6.15?
> > 
> > Do you have an URL I can send to those users to test ?
> 
> Here's the patch itself, feel free to spread it around.
> 
> It's also at:
>   kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/patches/usb/usbcore-allow-suspend-resume-even-if-drivers-don-t-support-it.patch

Initial user reports are that it fixes the problem with the Bluetooth
driver preventing newer powerbooks from sleeping.

Thanks !
Ben.

Re: USB rejecting sleep

[Benjamin Herrenschmidt]

On Sun, 2005-12-18 at 14:25 -0800, Greg KH wrote:
> On Mon, Dec 19, 2005 at 09:13:50AM +1100, Benjamin Herrenschmidt wrote:
> > On Sun, 2005-12-18 at 13:50 -0800, Greg KH wrote:
> > 
> > > Yes it is, and I have a patch in my tree now that fixes this up and
> > > keeps the suspend process working properly for usb drivers that do not
> > > have a suspend function.
> > > 
> > > Hm, I wonder if it should go in for 2.6.15?
> > 
> > Do you have an URL I can send to those users to test ?
> 
> Here's the patch itself, feel free to spread it around.
> 
> It's also at:
>   kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/patches/usb/usbcore-allow-suspend-resume-even-if-drivers-don-t-support-it.patch

Ok, I did a bit more tests here with a Keyspan adapter on my laptop
(well known driver for not having the suspend/resume routines).

The good thing is with the patch, the machine goes to sleep. However,
the device is not disconnected/reconnected. What happens it that the bus
gets suspended anyway and the driver stays around (possibly getting
errors on some URBs).

This is fine, but not optimal, since that means most of the time that
the device will not work on resume unless disconnected/reconnected. (For
keyspan, it seems that the HW does support the suspend state, thus it's
just a matter of closing/re-opening the port, I suppose it would be easy
enough to fix the driver).

So this patch is good for it doesn't prevent sleep anymore, but it also
doesn't do what we decided it should do. I think David is right that we
should be able to disconnect the device without actually removing the
device & driver from sysfs, just let that happen at resume time.

Ben.

Re: USB rejecting sleep

[Alan Stern]

On Mon, 19 Dec 2005, Benjamin Herrenschmidt wrote:

> Ok, I did a bit more tests here with a Keyspan adapter on my laptop
> (well known driver for not having the suspend/resume routines).
> 
> The good thing is with the patch, the machine goes to sleep. However,
> the device is not disconnected/reconnected. What happens it that the bus
> gets suspended anyway and the driver stays around (possibly getting
> errors on some URBs).

Yes, that's what is supposed to happen with that patch.

> This is fine, but not optimal, since that means most of the time that
> the device will not work on resume unless disconnected/reconnected. (For
> keyspan, it seems that the HW does support the suspend state, thus it's
> just a matter of closing/re-opening the port, I suppose it would be easy
> enough to fix the driver).
> 
> So this patch is good for it doesn't prevent sleep anymore, but it also
> doesn't do what we decided it should do. I think David is right that we
> should be able to disconnect the device without actually removing the
> device & driver from sysfs, just let that happen at resume time.

Of course the patch doesn't do what we talked about.  It says so right in 
the Changelog comment.

I disagree with the idea of disconnecting the device.  The right thing to 
do is what David wanted all along: unbind the driver.  This would require 
only a small change to the driver core.

It's too late for me to work on this now, but maybe tomorrow I'll have to 
a chance to write something.

Alan Stern

Re: USB rejecting sleep

[Benjamin Herrenschmidt]

On Sun, 2005-12-18 at 22:11 -0500, Alan Stern wrote:

> I disagree with the idea of disconnecting the device.  The right thing to 
> do is what David wanted all along: unbind the driver.  This would require 
> only a small change to the driver core.
> 
> It's too late for me to work on this now, but maybe tomorrow I'll have to 
> a chance to write something.

Why not also disconnect the device ? That will guarantee that when
coming back from sleep, the driver will re-discover a fresh new device
that has properly been reset no ? Instead of a device potentially
crashed because it didn't handle the suspend/resume transition
properly...

Ben.

Re: USB rejecting sleep

[Alan Stern]

On Mon, 19 Dec 2005, Benjamin Herrenschmidt wrote:

> On Sun, 2005-12-18 at 22:11 -0500, Alan Stern wrote:
> 
> > I disagree with the idea of disconnecting the device.  The right thing to 
> > do is what David wanted all along: unbind the driver.  This would require 
> > only a small change to the driver core.
> > 
> > It's too late for me to work on this now, but maybe tomorrow I'll have to 
> > a chance to write something.
> 
> Why not also disconnect the device ? That will guarantee that when
> coming back from sleep, the driver will re-discover a fresh new device
> that has properly been reset no ? Instead of a device potentially
> crashed because it didn't handle the suspend/resume transition
> properly...

I don't want to disconnect the device because there may be other drivers 
bound to other interfaces that are working just fine.  There's no need to 
mess them up.  Unbinding the misbehaving driver should be okay; it's no 
worse than doing rmmod before the suspend.

Alan Stern

Re: USB rejecting sleep

[Pavel Machek]

Hi!

> > What exactly changed in the recent USB stacks that is causing it to
> > abort system suspend much more often ? I'm getting lots of user reports
> > with 2.6.15-rc5 saying that they can't put their internal laptops to
> > sleep, apparently because a driver doesn't have a suspend method
> > (internal bluetooth in this case).
> > 
> > It's never been mandatory so far for all drivers of all connected
> > devices to have a suspend method... didn't we decide back then that
> > disconneting those was the right way to go ?
> 
> Yes it is, and I have a patch in my tree now that fixes this up and
> keeps the suspend process working properly for usb drivers that do not
> have a suspend function.
> 
> Hm, I wonder if it should go in for 2.6.15?

It would be nice to have some fixes in 2.6.15, so we are
not swamped with bugreports. Its a regression after all.

-- 
Thanks, Sharp!

Re: USB rejecting sleep

[Greg KH]

On Thu, Dec 22, 2005 at 04:02:44PM +0000, Pavel Machek wrote:
> Hi!
> 
> > > What exactly changed in the recent USB stacks that is causing it to
> > > abort system suspend much more often ? I'm getting lots of user reports
> > > with 2.6.15-rc5 saying that they can't put their internal laptops to
> > > sleep, apparently because a driver doesn't have a suspend method
> > > (internal bluetooth in this case).
> > > 
> > > It's never been mandatory so far for all drivers of all connected
> > > devices to have a suspend method... didn't we decide back then that
> > > disconneting those was the right way to go ?
> > 
> > Yes it is, and I have a patch in my tree now that fixes this up and
> > keeps the suspend process working properly for usb drivers that do not
> > have a suspend function.
> > 
> > Hm, I wonder if it should go in for 2.6.15?
> 
> It would be nice to have some fixes in 2.6.15, so we are
> not swamped with bugreports. Its a regression after all.

It's already in 2.6.15-rc7.

thanks,

greg k-h