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 <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
Pavel
---
commit feccc30d90155bcbc937f87643182a43d25873eb
tree 96394e24075a885f1a8bb3e53203f8397e78ea46
parent 5e6effaed6da94e727cd45f945ad2489af8570b3
author Alan Stern <[email protected]> Mon, 03 Mar 2008 15:15:59 -0500
committer Greg Kroah-Hartman <[email protected]> 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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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);
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 <[email protected]>
---
--- 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);
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
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);
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
From: Linus Torvalds <[email protected]>
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 <[email protected]>
---
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);
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 <[email protected]>
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
On Tue, Jun 10, 2008 at 02:59:43PM -0400, Alan Stern wrote:
> From: Linus Torvalds <[email protected]>
>
> 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 <[email protected]>
>
> ---
>
> 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
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 [email protected] Tue Jun 10 14:31:37 2008
From: Linus Torvalds <[email protected]>
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 <[email protected]>
Cc: Linus Torvalds <[email protected]>, Oliver Neukum <[email protected]>, Pavel Machek <[email protected]>, USB list <[email protected]>, Andrew Morton <[email protected]>, kernel list <[email protected]>, "Rafael J. Wysocki" <[email protected]>
Message-ID: <[email protected]>
From: Linus Torvalds <[email protected]>
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 <[email protected]>
Signed-off-by: Alan Stern <[email protected]>
Cc: Oliver Neukum <[email protected]>
Cc: Pavel Machek <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
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 [email protected] are
usb.current/usb-don-t-use-reset-resume-if-drivers-don-t-support-it.patch