2.6.25-rc6: CONFIG_USB_PERSIST forced on

2.6.25-rc6: CONFIG_USB_PERSIST forced on

[Pavel Machek]

Hi!

In 2.6.26-rc5, 

-config USB_PERSIST
-       bool "USB device persistence during system suspend (DANGEROUS)"

is now on. Given that it was previously marked "DANGEROUS", that seems
quite a change to me.

-
-               WARNING: This option can be dangerous!
-
-         If a USB device is replaced by another of the same type while
-         the system is asleep, there's a good chance the kernel won't
-         detect the change.  Likewise if the media in a USB storage
-         device is replaced.  When this happens it's almost certain to
-         cause data corruption and maybe even crash your system.
-
-         If you are unsure, say N here.

Besides, it seems to have broken usblp hibernation support, and maybe
other devices that does not have reset_resume() present. (Big thanks
for Oliver for doing investigation).

[Or is it that now USB_PERSIST is conditional on /sys fs setting, so
while setting it on individual device is dangerous, it is still N by
default? Changelog does not tell me...?]

Commit is:

USB: remove CONFIG_USB_PERSIST setting

This patch (as1047) removes the USB_PERSIST Kconfig option, enabling
it permanently.  It also prevents the power/persist attribute from
being created for hub devices; there's no point in having it since
USB-PERSIST is always turned on for hubs.

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

								Pavel
---
commit feccc30d90155bcbc937f87643182a43d25873eb
tree 96394e24075a885f1a8bb3e53203f8397e78ea46
parent 5e6effaed6da94e727cd45f945ad2489af8570b3
author Alan Stern <stern@rowland.harvard.edu> Mon, 03 Mar 2008 15:15:59 -0500
committer Greg Kroah-Hartman <gregkh@suse.de> Thu, 24 Apr 2008 21:16:32 -0700



-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: 2.6.25-rc6: CONFIG_USB_PERSIST forced on

[Alan Stern]

On Fri, 6 Jun 2008, Pavel Machek wrote:

> Hi!
> 
> In 2.6.26-rc5, 
> 
> -config USB_PERSIST
> -       bool "USB device persistence during system suspend (DANGEROUS)"
> 
> is now on. Given that it was previously marked "DANGEROUS", that seems
> quite a change to me.

It is.  Basically I did it because Linus asked for it -- or at least,
for something like it.  He felt that the danger of not having
USB_PERSIST when you need it is worse than the danger of having it when
you don't want it.

> Besides, it seems to have broken usblp hibernation support, and maybe
> other devices that does not have reset_resume() present. (Big thanks
> for Oliver for doing investigation).

URL?  It's hard to see how lack of reset-resume support would mess 
anything up worse than it would be without USB_PERSIST.

> [Or is it that now USB_PERSIST is conditional on /sys fs setting, so
> while setting it on individual device is dangerous, it is still N by
> default? Changelog does not tell me...?]

USB_PERSIST is (and has always been) conditional on the power/persist
sysfs attribute setting.  The difference is that now the setting starts
out as On whereas before it started out as Off, and there no longer is
a Kconfig option to remove USB_PERSIST entirely.

Alan Stern

Re: 2.6.25-rc6: CONFIG_USB_PERSIST forced on

[Pavel Machek]

On Fri 2008-06-06 10:39:19, Alan Stern wrote:
> On Fri, 6 Jun 2008, Pavel Machek wrote:
> 
> > Hi!
> > 
> > In 2.6.26-rc5, 
> > 
> > -config USB_PERSIST
> > -       bool "USB device persistence during system suspend (DANGEROUS)"
> > 
> > is now on. Given that it was previously marked "DANGEROUS", that seems
> > quite a change to me.
> 
> It is.  Basically I did it because Linus asked for it -- or at least,
> for something like it.  He felt that the danger of not having
> USB_PERSIST when you need it is worse than the danger of having it when
> you don't want it.

Well, not-usb-persists means we force-unplug disks during suspend,
after syncing them. Not _too_ bad.

If you unplug disk while hibernated, modify it, and plug it back,
youget _silent_ filesystem corruption. I call that bad.

> > Besides, it seems to have broken usblp hibernation support, and maybe
> > other devices that does not have reset_resume() present. (Big thanks
> > for Oliver for doing investigation).
> 
> URL?  It's hard to see how lack of reset-resume support would mess 
> anything up worse than it would be without USB_PERSIST.

usblp driver sets 'suspended' flag in .suspend(), and resets it in
.resume() -- except that with USB_PERSIST .resume() is not called :-(.

I have novell bugzilla bug number somewhere...

> > [Or is it that now USB_PERSIST is conditional on /sys fs setting, so
> > while setting it on individual device is dangerous, it is still N by
> > default? Changelog does not tell me...?]
> 
> USB_PERSIST is (and has always been) conditional on the power/persist
> sysfs attribute setting.  The difference is that now the setting starts
> out as On whereas before it started out as Off, and there no longer is
> a Kconfig option to remove USB_PERSIST entirely.

I consider the default to be dangerous here.

							Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: 2.6.25-rc6: CONFIG_USB_PERSIST forced on

[Linus Torvalds]

On Sat, 7 Jun 2008, Pavel Machek wrote:
> 
> Well, not-usb-persists means we force-unplug disks during suspend,
> after syncing them. Not _too_ bad.
> 
> If you unplug disk while hibernated, modify it, and plug it back,
> youget _silent_ filesystem corruption. I call that bad.

.. and if the USB layer unplugs them unconditionally while the filesystem 
is mounted, you _unconditionally_ get a system that doesn't work.

I call that worse.

Much worse.

			Linus

Re: 2.6.25-rc6: CONFIG_USB_PERSIST forced on

[Pavel Machek]

Hi!

On Sat 2008-06-07 15:26:12, Linus Torvalds wrote:
> On Sat, 7 Jun 2008, Pavel Machek wrote:
> > Well, not-usb-persists means we force-unplug disks during suspend,
> > after syncing them. Not _too_ bad.
> > 
> > If you unplug disk while hibernated, modify it, and plug it back,
> > youget _silent_ filesystem corruption. I call that bad.
> 
> .. and if the USB layer unplugs them unconditionally while the filesystem 
> is mounted, you _unconditionally_ get a system that doesn't work.

If your device did not loose power during s2ram, it should just work.

There's a tweak you can set in /sys you can set if your device is not
removable, you can set it for non-removable devices that _do_ loose
power.

Besides, it seems to break suspend/resume of printers, and probably
all the drivers that do not have reset_resume() method. That's
actually a regression.

https://bugzilla.novell.com/show_bug.cgi?id=394820

								Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: 2.6.25-rc6: CONFIG_USB_PERSIST forced on

[Alan Stern]

On Mon, 9 Jun 2008, Pavel Machek wrote:

> Besides, it seems to break suspend/resume of printers, and probably
> all the drivers that do not have reset_resume() method. That's
> actually a regression.
> 
> https://bugzilla.novell.com/show_bug.cgi?id=394820

The right way to fix this is to add reset_resume to the printer driver.  

More generally, usbcore should be changed so that drivers which don't 
support reset_resume get unbound from their devices when a reset-resume 
occurs.  I wrote something to do that last year, but with all the 
changes going on in the driver core it is now obsolete.

Alan Stern

Re: 2.6.25-rc6: CONFIG_USB_PERSIST forced on

[Oliver Neukum]

Am Montag 09 Juni 2008 17:03:10 schrieb Alan Stern:
> On Mon, 9 Jun 2008, Pavel Machek wrote:
> 
> > Besides, it seems to break suspend/resume of printers, and probably
> > all the drivers that do not have reset_resume() method. That's
> > actually a regression.
> > 
> > https://bugzilla.novell.com/show_bug.cgi?id=394820
> 
> The right way to fix this is to add reset_resume to the printer driver.  

reset_resume() is supposed to restore all state. The printer driver does
not know which state a printer is in, except for the trivial case of the
printer not being in use, as it doesn't know the meaning of the data
going to the printer.

You might argue that you deserve what you get when you hibernate
while printing, but then it makes no sense to implement it anyhow,
disconnection and reconnection work just as well and are cleaner.
The same is true for many devices.

	Regards
		Oliver

Re: 2.6.25-rc6: CONFIG_USB_PERSIST forced on

[Alan Stern]

On Mon, 9 Jun 2008, Oliver Neukum wrote:

> Am Montag 09 Juni 2008 17:03:10 schrieb Alan Stern:
> > On Mon, 9 Jun 2008, Pavel Machek wrote:
> > 
> > > Besides, it seems to break suspend/resume of printers, and probably
> > > all the drivers that do not have reset_resume() method. That's
> > > actually a regression.
> > > 
> > > https://bugzilla.novell.com/show_bug.cgi?id=394820
> > 
> > The right way to fix this is to add reset_resume to the printer driver.  
> 
> reset_resume() is supposed to restore all state. The printer driver does
> not know which state a printer is in, except for the trivial case of the
> printer not being in use, as it doesn't know the meaning of the data
> going to the printer.
> 
> You might argue that you deserve what you get when you hibernate
> while printing, but then it makes no sense to implement it anyhow,
> disconnection and reconnection work just as well and are cleaner.
> The same is true for many devices.

In which case the correct approach is the second one I mentioned (which 
you omitted in your reply): Make usbcore unbind drivers that don't 
support reset_resume.

Alan Stern

Re: 2.6.25-rc6: CONFIG_USB_PERSIST forced on

[Greg KH]

On Mon, Jun 09, 2008 at 11:44:21AM -0400, Alan Stern wrote:
> On Mon, 9 Jun 2008, Oliver Neukum wrote:
> 
> > Am Montag 09 Juni 2008 17:03:10 schrieb Alan Stern:
> > > On Mon, 9 Jun 2008, Pavel Machek wrote:
> > > 
> > > > Besides, it seems to break suspend/resume of printers, and probably
> > > > all the drivers that do not have reset_resume() method. That's
> > > > actually a regression.
> > > > 
> > > > https://bugzilla.novell.com/show_bug.cgi?id=394820
> > > 
> > > The right way to fix this is to add reset_resume to the printer driver.  
> > 
> > reset_resume() is supposed to restore all state. The printer driver does
> > not know which state a printer is in, except for the trivial case of the
> > printer not being in use, as it doesn't know the meaning of the data
> > going to the printer.
> > 
> > You might argue that you deserve what you get when you hibernate
> > while printing, but then it makes no sense to implement it anyhow,
> > disconnection and reconnection work just as well and are cleaner.
> > The same is true for many devices.
> 
> In which case the correct approach is the second one I mentioned (which 
> you omitted in your reply): Make usbcore unbind drivers that don't 
> support reset_resume.

That sounds reasonable to me.  Oliver or Pavel, care to try this out?

thanks,

greg k-h

Re: 2.6.25-rc6: CONFIG_USB_PERSIST forced on

[Pavel Machek]

On Mon 2008-06-09 12:50:15, Greg KH wrote:
> On Mon, Jun 09, 2008 at 11:44:21AM -0400, Alan Stern wrote:
> > On Mon, 9 Jun 2008, Oliver Neukum wrote:
> > 
> > > Am Montag 09 Juni 2008 17:03:10 schrieb Alan Stern:
> > > > On Mon, 9 Jun 2008, Pavel Machek wrote:
> > > > 
> > > > > Besides, it seems to break suspend/resume of printers, and probably
> > > > > all the drivers that do not have reset_resume() method. That's
> > > > > actually a regression.
> > > > > 
> > > > > https://bugzilla.novell.com/show_bug.cgi?id=394820
> > > > 
> > > > The right way to fix this is to add reset_resume to the printer driver.  
> > > 
> > > reset_resume() is supposed to restore all state. The printer driver does
> > > not know which state a printer is in, except for the trivial case of the
> > > printer not being in use, as it doesn't know the meaning of the data
> > > going to the printer.
> > > 
> > > You might argue that you deserve what you get when you hibernate
> > > while printing, but then it makes no sense to implement it anyhow,
> > > disconnection and reconnection work just as well and are cleaner.
> > > The same is true for many devices.
> > 
> > In which case the correct approach is the second one I mentioned (which 
> > you omitted in your reply): Make usbcore unbind drivers that don't 
> > support reset_resume.
> 
> That sounds reasonable to me.  Oliver or Pavel, care to try this out?

I'm not an USB hacker, and 2.6.26 release is pretty near. I do have
USB printer somewhere around, maybe it still works.

But problem is _not_ limited to usblp. usblp is just a part of
problem, the one we caught in testing.

reset_resume() is only implemented in storage/usb.c... while resume()
is pretty widespread in the usb drivers.

I believe we should just revert the "CONFIG_USB_PERSIST force on"
patch, and solve this properly in 2.6.27.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: 2.6.25-rc6: CONFIG_USB_PERSIST forced on

[Linus Torvalds]

On Mon, 9 Jun 2008, Pavel Machek wrote:
> 
> I believe we should just revert the "CONFIG_USB_PERSIST force on"
> patch, and solve this properly in 2.6.27.

Why?

The code would _still_ be buggy with that revert in place. You have to 
enable CONFIG_USB_PERSIST just to get even the _possibility_ of the sane 
behavior. 

If there is a problem with usblp, it just needs to be fixed. 

		Linus

Re: 2.6.25-rc6: CONFIG_USB_PERSIST forced on

[Oliver Neukum]

Am Montag 09 Juni 2008 23:39:10 schrieb Linus Torvalds:
> 
> On Mon, 9 Jun 2008, Pavel Machek wrote:
> > 
> > I believe we should just revert the "CONFIG_USB_PERSIST force on"
> > patch, and solve this properly in 2.6.27.
> 
> Why?
> 
> The code would _still_ be buggy with that revert in place. You have to 
> enable CONFIG_USB_PERSIST just to get even the _possibility_ of the sane 
> behavior. 
> 
> If there is a problem with usblp, it just needs to be fixed. 

There's a problem with every driver that does not implement reset_resume().
That's the majority of drivers.

	Regards
		Oliver

Re: 2.6.25-rc6: CONFIG_USB_PERSIST forced on

[Pavel Machek]

On Mon 2008-06-09 14:39:10, Linus Torvalds wrote:
> 
> 
> On Mon, 9 Jun 2008, Pavel Machek wrote:
> > 
> > I believe we should just revert the "CONFIG_USB_PERSIST force on"
> > patch, and solve this properly in 2.6.27.
> 
> Why?
> 
> The code would _still_ be buggy with that revert in place. You have to 
> enable CONFIG_USB_PERSIST just to get even the _possibility_ of the sane 
> behavior. 
> 
> If there is a problem with usblp, it just needs to be fixed. 

With USB_PERSIST on, you have problem on all drivers but usb-storage,
AFAICT... because usb-storage seems to be the only driver implementing
reset_resume.

I guess it is possible to do something like "if reset_resume() is
unavailable, try plain resume()" in usb/driver.c, but I'd really
changes to the suspend/resume callback to go in -rc1 so that they are
tested properly, and not hot-patch it now.
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: 2.6.25-rc6: CONFIG_USB_PERSIST forced on

[Linus Torvalds]

On Mon, 9 Jun 2008, Pavel Machek wrote:
> 
> With USB_PERSIST on, you have problem on all drivers but usb-storage,
> AFAICT... because usb-storage seems to be the only driver implementing
> reset_resume.

So let's *fix* it. It's not as if USB_PERSIST is new - it was there 
for well over a year. Disabling it yet again will obviously not fix 
*anything*.

> I guess it is possible to do something like "if reset_resume() is
> unavailable, try plain resume()" in usb/driver.c, but I'd really
> changes to the suspend/resume callback to go in -rc1 so that they are
> tested properly, and not hot-patch it now.

And how many people have actually even reported the problem? And how big 
do you seriously expect the patch to be?

This isn't even a regression - it's behavior that has always been there. 
It's just that now the behavior is much more exposed, and by being more 
exposed, people now see a bug that they didn't see before. Let's just try 
to *fix* it first, especially since the guesses about how to fix it have 
so far been fairly simple.

For example, if it's really just a "if we don't have a reset_resume() 
function, use the regular resume instead", does this trivial patch make a 
difference? I dunno, but it seems to be what people are saying we should 
just try. Much simpler than a revert.

		Linus

---
 drivers/usb/core/driver.c |   16 +++++-----------
 1 files changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index 1e56f1c..893a1fe 100644
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -885,17 +885,11 @@ static int usb_resume_interface(struct usb_interface *intf, int reset_resume)
 	}
 	driver = to_usb_driver(intf->dev.driver);
 
-	if (reset_resume) {
-		if (driver->reset_resume) {
-			status = driver->reset_resume(intf);
-			if (status)
-				dev_err(&intf->dev, "%s error %d\n",
-						"reset_resume", status);
-		} else {
-			/* status = -EOPNOTSUPP; */
-			dev_warn(&intf->dev, "no %s for driver %s?\n",
-					"reset_resume", driver->name);
-		}
+	if (reset_resume && driver->reset_resume) {
+		status = driver->reset_resume(intf);
+		if (status)
+			dev_err(&intf->dev, "%s error %d\n",
+					"reset_resume", status);
 	} else {
 		if (driver->resume) {
 			status = driver->resume(intf);

Re: 2.6.25-rc6: CONFIG_USB_PERSIST forced on

[Oliver Neukum]

Am Montag 09 Juni 2008 23:56:51 schrieb Pavel Machek:
> On Mon 2008-06-09 14:39:10, Linus Torvalds wrote:
> > 
> > 
> > On Mon, 9 Jun 2008, Pavel Machek wrote:
> > > 
> > > I believe we should just revert the "CONFIG_USB_PERSIST force on"
> > > patch, and solve this properly in 2.6.27.
> > 
> > Why?
> > 
> > The code would _still_ be buggy with that revert in place. You have to 
> > enable CONFIG_USB_PERSIST just to get even the _possibility_ of the sane 
> > behavior. 
> > 
> > If there is a problem with usblp, it just needs to be fixed. 
> 
> With USB_PERSIST on, you have problem on all drivers but usb-storage,
> AFAICT... because usb-storage seems to be the only driver implementing
> reset_resume.
> 
> I guess it is possible to do something like "if reset_resume() is
> unavailable, try plain resume()" in usb/driver.c, but I'd really
> changes to the suspend/resume callback to go in -rc1 so that they are
> tested properly, and not hot-patch it now.

If a hotfix it must be, here's my take. It works for me, but it isn't
tested well.
Alan, what do you think?

	Regards
		Oliver

Signed-off-by: Oliver Neukum <oneukum@suse.de>

---

--- linux-2.6.25/drivers/usb/core/hub.c	2008-06-09 15:15:13.105058000 +0200
+++ alt/drivers/usb/core/hub.c	2008-06-09 15:43:29.964503000 +0200
@@ -648,7 +648,9 @@ static void hub_stop(struct usb_hub *hub
 static void hub_restart(struct usb_hub *hub, int type)
 {
 	struct usb_device *hdev = hub->hdev;
-	int port1;
+	struct usb_interface *intf;
+	struct usb_driver *driver;
+	int port1, i;
 
 	/* Check each of the children to see if they require
 	 * USB-PERSIST handling or disconnection.  Also check
@@ -692,6 +694,14 @@ static void hub_restart(struct usb_hub *
 		 */
 		if (udev->persist_enabled && status == 0 &&
 				!(portstatus & USB_PORT_STAT_ENABLE)) {
+
+			/* check for devices not supporting reset_resume */
+			for (i = 0; i < udev->actconfig->desc.bNumInterfaces; i++) {
+				intf = udev->actconfig->interface[i];
+				driver = to_usb_driver(intf->dev.driver);
+				if (!driver || !driver->reset_resume)
+					goto skip_reset_resume;
+			}
 			if (portchange & USB_PORT_STAT_C_ENABLE)
 				clear_port_feature(hub->hdev, port1,
 						USB_PORT_FEAT_C_ENABLE);
@@ -705,8 +715,11 @@ static void hub_restart(struct usb_hub *
 		 * but as we may not lock the child device here
 		 * we have to do a "logical" disconnect.
 		 */
-		else if (type == HUB_RESET_RESUME)
-			hub_port_logical_disconnect(hub, port1);
+		else {
+skip_reset_resume:
+			if (type == HUB_RESET_RESUME)
+				hub_port_logical_disconnect(hub, port1);
+		}
 	}
 
 	hub_activate(hub);

Re: 2.6.25-rc6: CONFIG_USB_PERSIST forced on

[Alan Stern]

On Tue, 10 Jun 2008, Oliver Neukum wrote:

> Am Montag 09 Juni 2008 23:56:51 schrieb Pavel Machek:
> > On Mon 2008-06-09 14:39:10, Linus Torvalds wrote:
> > > 
> > > 
> > > On Mon, 9 Jun 2008, Pavel Machek wrote:
> > > > 
> > > > I believe we should just revert the "CONFIG_USB_PERSIST force on"
> > > > patch, and solve this properly in 2.6.27.
> > > 
> > > Why?
> > > 
> > > The code would _still_ be buggy with that revert in place. You have to 
> > > enable CONFIG_USB_PERSIST just to get even the _possibility_ of the sane 
> > > behavior. 
> > > 
> > > If there is a problem with usblp, it just needs to be fixed. 
> > 
> > With USB_PERSIST on, you have problem on all drivers but usb-storage,
> > AFAICT... because usb-storage seems to be the only driver implementing
> > reset_resume.
> > 
> > I guess it is possible to do something like "if reset_resume() is
> > unavailable, try plain resume()" in usb/driver.c, but I'd really
> > changes to the suspend/resume callback to go in -rc1 so that they are
> > tested properly, and not hot-patch it now.
> 
> If a hotfix it must be, here's my take. It works for me, but it isn't
> tested well.
> Alan, what do you think?

I don't have time right now to check it; will do so later today.  At 
first glance it seems okay -- it doesn't cover absolutely all the 
cases, but it does cover system sleep transitions.  And it's clearly 
better than what Linus proposed.

Alan Stern

Re: 2.6.25-rc6: CONFIG_USB_PERSIST forced on

[Linus Torvalds]

On Tue, 10 Jun 2008, Oliver Neukum wrote:
> 
> If a hotfix it must be, here's my take. It works for me, but it isn't
> tested well.
> Alan, what do you think?

Hmm. I hate making functions more complex. Especially if it's an area 
where we'd expect it to change in the future (ie I think we should aim for 
taking into account whether a device is actually open or not). So here's a 
cleaned-up alternative of your approach with the whole "is it persistent" 
logic separated out into a function of its own.

Then, some day, if we get back-pointers to open devices, or we get drievr 
hooks to say "am I mounted" or something, we have a logical place to do 
those things.

Is there perhaps already a way to know whether a driver is actually 
*active* or not (ie not just registered, but somebody has then opened it)? 
I guess there isn't. Oh, well.

		Linus

---
 drivers/usb/core/hub.c |   44 ++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 8eb4da3..3b0b58e 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -644,6 +644,46 @@ static void hub_stop(struct usb_hub *hub)
 
 #ifdef CONFIG_PM
 
+static int persistent_device(struct usb_device *udev)
+{
+	int i, retval;
+	struct usb_host_config *actconfig;
+
+	/* Explicitly not marked persistent? */
+	if (!udev->persist_enabled)
+		return 0;
+
+	/* No active config? */
+	actconfig = udev->actconfig;
+	if (!actconfig)
+		return 0;
+
+	/* FIXME! We should check whether it's open here or not! */
+
+	/*
+	 * Check that all drivers on the active interface have a
+	 * 'reset_resume' entrypoint
+	 */
+	retval = 0;
+	for (i = 0; i < actconfig->desc.bNumInterfaces; i++) {
+		struct usb_interface *intf;
+		struct usb_driver *driver;
+
+		intf = actconfig->interface[i];
+		driver = to_usb_driver(intf->dev.driver);
+		if (!driver)
+			continue;
+		if (!driver->reset_resume)
+			return 0;
+		/*
+		 * We have at least one driver, and that one
+		 * supports 'reset_resume'
+		 */
+		retval = 1;
+	}
+	return retval;
+}
+
 static void hub_restart(struct usb_hub *hub, int type)
 {
 	struct usb_device *hdev = hub->hdev;
@@ -689,8 +729,8 @@ static void hub_restart(struct usb_hub *hub, int type)
 		 * turn off the various status changes to prevent
 		 * khubd from disconnecting it later.
 		 */
-		if (udev->persist_enabled && status == 0 &&
-				!(portstatus & USB_PORT_STAT_ENABLE)) {
+		if (status == 0 && !(portstatus & USB_PORT_STAT_ENABLE) &&
+		    persistent_device(udev)) {
 			if (portchange & USB_PORT_STAT_C_ENABLE)
 				clear_port_feature(hub->hdev, port1,
 						USB_PORT_FEAT_C_ENABLE);

Re: 2.6.25-rc6: CONFIG_USB_PERSIST forced on

[Linus Torvalds]

On Tue, 10 Jun 2008, Linus Torvalds wrote:
> +
> +		intf = actconfig->interface[i];
> +		driver = to_usb_driver(intf->dev.driver);
> +		if (!driver)
> +			continue;

This is wrong. It should be

	if (!intf->dev.driver)
		continue;
	driver = to_usb_driver(intf->dev.driver);

because doing the check after the "to_usb_driver()" conversion is too 
late. I copied the bug from Oliver's version.

		Linus

[PATCH] USB: don't use reset-resume if drivers don't support it

[Alan Stern]

From: Linus Torvalds <torvalds@linux-foundation.org>

This patch tries to identify which devices are able to accept
reset-resume handling, by checking that there is at least one
interface driver bound and that all of the drivers have a reset_resume
method defined.  If these conditions don't hold then during resume
processing, the device is logicall disconnected.

This is only a temporary fix.  Later on we will explicitly unbind
drivers that can't handle reset-resumes.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

---

Greg:

This patch really is a temporary fix meant only for 2.6.26.  There
already are changes in the gregkh development tree which clash with
this, and I intend to submit separately a real fix for the problem
(i.e., unbind drivers that don't have suspend, resume, reset_resume,
pre_reset, or post_reset methods, as needed).

Is there any way this can be added to 2.6.26-rc while leaving it out of 
the gregkh tree?

Alan Stern



Index: 2.6.26-rc5/drivers/usb/core/hub.c
===================================================================
--- 2.6.26-rc5.orig/drivers/usb/core/hub.c
+++ 2.6.26-rc5/drivers/usb/core/hub.c
@@ -644,6 +644,48 @@ static void hub_stop(struct usb_hub *hub
 
 #ifdef CONFIG_PM
 
+/* Try to identify which devices need USB-PERSIST handling */
+static int persistent_device(struct usb_device *udev)
+{
+	int i;
+	int retval;
+	struct usb_host_config *actconfig;
+
+	/* Explicitly not marked persistent? */
+	if (!udev->persist_enabled)
+		return 0;
+
+	/* No active config? */
+	actconfig = udev->actconfig;
+	if (!actconfig)
+		return 0;
+
+	/* FIXME! We should check whether it's open here or not! */
+
+	/*
+	 * Check that all the interface drivers have a
+	 * 'reset_resume' entrypoint
+	 */
+	retval = 0;
+	for (i = 0; i < actconfig->desc.bNumInterfaces; i++) {
+		struct usb_interface *intf;
+		struct usb_driver *driver;
+
+		intf = actconfig->interface[i];
+		if (!intf->dev.driver)
+			continue;
+		driver = to_usb_driver(intf->dev.driver);
+		if (!driver->reset_resume)
+			return 0;
+		/*
+		 * We have at least one driver, and that one
+		 * has a reset_resume method.
+		 */
+		retval = 1;
+	}
+	return retval;
+}
+
 static void hub_restart(struct usb_hub *hub, int type)
 {
 	struct usb_device *hdev = hub->hdev;
@@ -689,8 +731,8 @@ static void hub_restart(struct usb_hub *
 		 * turn off the various status changes to prevent
 		 * khubd from disconnecting it later.
 		 */
-		if (udev->persist_enabled && status == 0 &&
-				!(portstatus & USB_PORT_STAT_ENABLE)) {
+		if (status == 0 && !(portstatus & USB_PORT_STAT_ENABLE) &&
+				persistent_device(udev)) {
 			if (portchange & USB_PORT_STAT_C_ENABLE)
 				clear_port_feature(hub->hdev, port1,
 						USB_PORT_FEAT_C_ENABLE);

Re: [PATCH] USB: don't use reset-resume if drivers don't support it

[Linus Torvalds]

On Tue, 10 Jun 2008, Alan Stern wrote:
> 
> This is only a temporary fix.  Later on we will explicitly unbind
> drivers that can't handle reset-resumes.
> 
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

And feel free to add my signed-off on it too, assuming somebody who saw 
the problems has actually tested it.

And thanks for picking up the fix and creating an updated patch.

		Linus

Re: [PATCH] USB: don't use reset-resume if drivers don't support it

[Greg KH]

On Tue, Jun 10, 2008 at 02:59:43PM -0400, Alan Stern wrote:
> From: Linus Torvalds <torvalds@linux-foundation.org>
> 
> This patch tries to identify which devices are able to accept
> reset-resume handling, by checking that there is at least one
> interface driver bound and that all of the drivers have a reset_resume
> method defined.  If these conditions don't hold then during resume
> processing, the device is logicall disconnected.
> 
> This is only a temporary fix.  Later on we will explicitly unbind
> drivers that can't handle reset-resumes.
> 
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> 
> ---
> 
> Greg:
> 
> This patch really is a temporary fix meant only for 2.6.26.  There
> already are changes in the gregkh development tree which clash with
> this, and I intend to submit separately a real fix for the problem
> (i.e., unbind drivers that don't have suspend, resume, reset_resume,
> pre_reset, or post_reset methods, as needed).
> 
> Is there any way this can be added to 2.6.26-rc while leaving it out of 
> the gregkh tree?

Yes I can, but note, the gregkh tree gets rebased on what goes into -rc,
so hopefully it will not conflict too much :)

thanks,

greg k-h

patch usb-don-t-use-reset-resume-if-drivers-don-t-support-it.patch added to gregkh-2.6 tree

[gregkh]

This is a note to let you know that I've just added the patch titled

     Subject: USB: don't use reset-resume if drivers don't support it

to my gregkh-2.6 tree.  Its filename is

     usb-don-t-use-reset-resume-if-drivers-don-t-support-it.patch

This tree can be found at 
    http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/patches/


>From stern@rowland.harvard.edu  Tue Jun 10 14:31:37 2008
From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Tue, 10 Jun 2008 14:59:43 -0400 (EDT)
Subject: USB: don't use reset-resume if drivers don't support it
To: Greg KH <greg@kroah.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,  Oliver Neukum <oliver@neukum.org>, Pavel Machek <pavel@suse.cz>,  USB list <linux-usb@vger.kernel.org>,  Andrew Morton <akpm@linuxfoundation.org>,  kernel list <linux-kernel@vger.kernel.org>,  "Rafael J. Wysocki" <rjw@sisk.pl>
Message-ID: <Pine.LNX.4.44L0.0806101454480.3658-100000@iolanthe.rowland.org>

From: Linus Torvalds <torvalds@linux-foundation.org>

This patch tries to identify which devices are able to accept
reset-resume handling, by checking that there is at least one
interface driver bound and that all of the drivers have a reset_resume
method defined.  If these conditions don't hold then during resume
processing, the device is logicall disconnected.

This is only a temporary fix.  Later on we will explicitly unbind
drivers that can't handle reset-resumes.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
Cc: Oliver Neukum <oliver@neukum.org>
Cc: Pavel Machek <pavel@suse.cz>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

---
 drivers/usb/core/hub.c |   46 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 44 insertions(+), 2 deletions(-)

--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -644,6 +644,48 @@ static void hub_stop(struct usb_hub *hub
 
 #ifdef CONFIG_PM
 
+/* Try to identify which devices need USB-PERSIST handling */
+static int persistent_device(struct usb_device *udev)
+{
+	int i;
+	int retval;
+	struct usb_host_config *actconfig;
+
+	/* Explicitly not marked persistent? */
+	if (!udev->persist_enabled)
+		return 0;
+
+	/* No active config? */
+	actconfig = udev->actconfig;
+	if (!actconfig)
+		return 0;
+
+	/* FIXME! We should check whether it's open here or not! */
+
+	/*
+	 * Check that all the interface drivers have a
+	 * 'reset_resume' entrypoint
+	 */
+	retval = 0;
+	for (i = 0; i < actconfig->desc.bNumInterfaces; i++) {
+		struct usb_interface *intf;
+		struct usb_driver *driver;
+
+		intf = actconfig->interface[i];
+		if (!intf->dev.driver)
+			continue;
+		driver = to_usb_driver(intf->dev.driver);
+		if (!driver->reset_resume)
+			return 0;
+		/*
+		 * We have at least one driver, and that one
+		 * has a reset_resume method.
+		 */
+		retval = 1;
+	}
+	return retval;
+}
+
 static void hub_restart(struct usb_hub *hub, int type)
 {
 	struct usb_device *hdev = hub->hdev;
@@ -689,8 +731,8 @@ static void hub_restart(struct usb_hub *
 		 * turn off the various status changes to prevent
 		 * khubd from disconnecting it later.
 		 */
-		if (udev->persist_enabled && status == 0 &&
-				!(portstatus & USB_PORT_STAT_ENABLE)) {
+		if (status == 0 && !(portstatus & USB_PORT_STAT_ENABLE) &&
+				persistent_device(udev)) {
 			if (portchange & USB_PORT_STAT_C_ENABLE)
 				clear_port_feature(hub->hdev, port1,
 						USB_PORT_FEAT_C_ENABLE);


Patches currently in gregkh-2.6 which might be from torvalds@linux-foundation.org are

usb.current/usb-don-t-use-reset-resume-if-drivers-don-t-support-it.patch

Re: 2.6.25-rc6: CONFIG_USB_PERSIST forced on

[Oliver Neukum]

Am Montag 09 Juni 2008 17:44:21 schrieb Alan Stern:
> > You might argue that you deserve what you get when you hibernate
> > while printing, but then it makes no sense to implement it anyhow,
> > disconnection and reconnection work just as well and are cleaner.
> > The same is true for many devices.
> 
> In which case the correct approach is the second one I mentioned (which 
> you omitted in your reply): Make usbcore unbind drivers that don't 
> support reset_resume.

Good news.
Do you have such a patch? The current default is very drastic.

	Regards
		Oliver

Re: 2.6.25-rc6: CONFIG_USB_PERSIST forced on

[Linus Torvalds]

On Mon, 9 Jun 2008, Pavel Machek wrote:
> 
> If your device did not loose power during s2ram, it should just work.

Tough. And if pigs had wings, they could fly. What's your point?

> There's a tweak you can set in /sys you can set if your device is not
> removable, you can set it for non-removable devices that _do_ loose
> power.

Yes, and it's totally pointless. I had this whole discussion already. 
That flag is too hard to find for any normal person to be useful, and the 
fact is, if you hold the device mounted over a suspend, it should be set 
by default anyway.

> Besides, it seems to break suspend/resume of printers, and probably
> all the drivers that do not have reset_resume() method. That's
> actually a regression.
> 
> https://bugzilla.novell.com/show_bug.cgi?id=394820

So printers shouldn't do it, since they aren't mounted. Neither should 
mice etc things. What does that have to do with block devices?

		Linus