[PATCH] usb: Make USB persist default configurable

[PATCH] usb: Make USB persist default configurable

[Julius Werner]

Commit 9214d1d8 set the USB persist flag as a default for all devices.
This might be desirable for some distributions, but it certainly has its
trade-offs... most importantly, it can significantly increase system
resume time, because the kernel blocks on resuming (and sometimes
resetting) USB devices before it unfreezes userspace.

This patch introduces a new config option CONFIG_USB_DEFAULT_PERSIST,
which allows distributions to make this decision on their own without
the need to carry a custom patch or revert the kernel's setting in
userspace.

Signed-off-by: Julius Werner <jwerner@chromium.org>
---
 drivers/usb/core/Kconfig  | 14 ++++++++++++++
 drivers/usb/core/quirks.c | 16 +++++-----------
 2 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/core/Kconfig b/drivers/usb/core/Kconfig
index f70c1a1..dfc1360 100644
--- a/drivers/usb/core/Kconfig
+++ b/drivers/usb/core/Kconfig
@@ -27,6 +27,20 @@ config USB_ANNOUNCE_NEW_DEVICES
 comment "Miscellaneous USB options"
 	depends on USB
 
+config USB_DEFAULT_PERSIST
+	bool "Enable USB persist by default"
+	depends on USB
+	default y
+	help
+	  Say N here if you don't want USB power session persistance
+	  enabled by default. This will make suspended USB devices that
+	  lose power get reenumerated as if they had been unplugged,
+	  but may reduce your system's resume time and eliminates any
+	  chance of file system corruption by confusing two devices with
+	  the same vendor and product ID. The persist feature can still
+	  be enabled for individual devices through the power/persist
+	  sysfs node. See Documentation/usb/persist.txt for more info.
+
 config USB_DYNAMIC_MINORS
 	bool "Dynamic USB minor allocation"
 	depends on USB
diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
index 3113c1d..ab5638d 100644
--- a/drivers/usb/core/quirks.c
+++ b/drivers/usb/core/quirks.c
@@ -201,20 +201,14 @@ void usb_detect_quirks(struct usb_device *udev)
 		dev_dbg(&udev->dev, "USB quirks for this device: %x\n",
 			udev->quirks);
 
-	/* For the present, all devices default to USB-PERSIST enabled */
-#if 0		/* was: #ifdef CONFIG_PM */
-	/* Hubs are automatically enabled for USB-PERSIST */
-	if (udev->descriptor.bDeviceClass == USB_CLASS_HUB)
+#ifdef CONFIG_USB_DEFAULT_PERSIST
+	if (!(udev->quirks & USB_QUIRK_RESET))
 		udev->persist_enabled = 1;
-
 #else
-	/* In the absence of PM, we can safely enable USB-PERSIST
-	 * for all devices.  It will affect things like hub resets
-	 * and EMF-related port disables.
-	 */
-	if (!(udev->quirks & USB_QUIRK_RESET))
+	/* Hubs are automatically enabled for USB-PERSIST */
+	if (udev->descriptor.bDeviceClass == USB_CLASS_HUB)
 		udev->persist_enabled = 1;
-#endif	/* CONFIG_PM */
+#endif	/* CONFIG_USB_DEFAULT_PERSIST */
 }
 
 void usb_detect_interface_quirks(struct usb_device *udev)
-- 
1.7.12.4

Re: [PATCH] usb: Make USB persist default configurable

[Greg Kroah-Hartman]

On Wed, Mar 13, 2013 at 03:57:31PM -0700, Julius Werner wrote:
> Commit 9214d1d8 set the USB persist flag as a default for all devices.
> This might be desirable for some distributions, but it certainly has its
> trade-offs... most importantly, it can significantly increase system
> resume time, because the kernel blocks on resuming (and sometimes
> resetting) USB devices before it unfreezes userspace.
> 
> This patch introduces a new config option CONFIG_USB_DEFAULT_PERSIST,
> which allows distributions to make this decision on their own without
> the need to carry a custom patch or revert the kernel's setting in
> userspace.

Why can't you just revert this in userspace?  Isn't that easier than
doing a kernel patch and providing an option that we need to now
maintain for pretty much forever?

thanks,

greg k-h

Re: [PATCH] usb: Make USB persist default configurable

[Julius Werner]

> Why can't you just revert this in userspace?  Isn't that easier than
> doing a kernel patch and providing an option that we need to now
> maintain for pretty much forever?

I could solve it in userspace, but that really feels like a hacky
workaround and not a long term solution. It would mean that every new
device starts with persist enabled and stays that way for a few
milliseconds (maybe up to seconds if it's connected on boot), until
userspace gets around to disable it again... opening the possibility
for very weird race conditions and bugs with drivers/devices that
don't work with persist. This default is a policy that really resides
in the kernel, it has changed in the past, and since there is no
definitive better choice for all cases I thought making it
configurable is the right thing to do.

Re: [PATCH] usb: Make USB persist default configurable

[Greg Kroah-Hartman]

On Mon, Mar 18, 2013 at 05:02:19PM -0700, Julius Werner wrote:
> > Why can't you just revert this in userspace?  Isn't that easier than
> > doing a kernel patch and providing an option that we need to now
> > maintain for pretty much forever?
> 
> I could solve it in userspace, but that really feels like a hacky
> workaround and not a long term solution. It would mean that every new
> device starts with persist enabled and stays that way for a few
> milliseconds (maybe up to seconds if it's connected on boot), until
> userspace gets around to disable it again... opening the possibility
> for very weird race conditions and bugs with drivers/devices that
> don't work with persist.

What drivers/devices don't work with persist?  We need to know that now,
otherwise all other distros and users have problems, right?

> This default is a policy that really resides in the kernel, it has
> changed in the past, and since there is no definitive better choice
> for all cases I thought making it configurable is the right thing to
> do.

Too many options can be a bad thing.

I think Alan made this a "always on" option, so I'd like to get his
opinion on it.  Alan?

thanks,

greg k-h

Re: [PATCH] usb: Make USB persist default configurable

[Julius Werner]

> What drivers/devices don't work with persist?  We need to know that now,
> otherwise all other distros and users have problems, right?

We have seen a problem with the LTE modem in the Chromebook Pixel
using the usb/serial/option.c driver, but we are not quite sure of the
root cause yet. It occasionally seems to loose its power session due
to autosuspend and then hit core/hub.c:check_port_resume_type(), where
it gets the reset_resume flag if persist is enabled. However, since
the option driver does not implement the (optional) reset_resume
method, this eventually ends up leaving the device in a state where it
is neither working nor getting cleanly reenumerated.

We are still actively investigating that issue to figure out the
underlying bug (not sure whether it is a general problem with drivers
that do not implement reset_resume... also, we are still only on 3.4
and have not tried to confirm it on 3.8 yet). However, as we generally
hadn't intended persist to be active and this had slipped through our
(admittedly very crude) userspace implementation, I proposed this
patch so that going forward our devices could always have the persist
setting we intend for them at all times.

Re: [PATCH] usb: Make USB persist default configurable

[Alan Stern]

On Mon, 18 Mar 2013, Greg Kroah-Hartman wrote:

> On Mon, Mar 18, 2013 at 05:02:19PM -0700, Julius Werner wrote:
> > > Why can't you just revert this in userspace?  Isn't that easier than
> > > doing a kernel patch and providing an option that we need to now
> > > maintain for pretty much forever?
> > 
> > I could solve it in userspace, but that really feels like a hacky
> > workaround and not a long term solution. It would mean that every new
> > device starts with persist enabled and stays that way for a few
> > milliseconds (maybe up to seconds if it's connected on boot), until
> > userspace gets around to disable it again... opening the possibility
> > for very weird race conditions and bugs with drivers/devices that
> > don't work with persist.
> 
> What drivers/devices don't work with persist?  We need to know that now,
> otherwise all other distros and users have problems, right?
> 
> > This default is a policy that really resides in the kernel, it has
> > changed in the past, and since there is no definitive better choice
> > for all cases I thought making it configurable is the right thing to
> > do.
> 
> Too many options can be a bad thing.
> 
> I think Alan made this a "always on" option, so I'd like to get his
> opinion on it.  Alan?

Originally the "persist" attribute defaulted to "off".  Linus disliked 
this (at least, he disliked it for mass-storage devices) and so at his 
request the default was changed to "on".  There didn't seem to be any 
reason to treat other devices differently from mass-storage devices; 
consequently the default is now "on" for everything.

Julius's commit message mentions the disadvantage of "persist": Resume
times can be increased.  But it doesn't mention the chief advantage:  
Filesystems stored on USB devices are much less likely to be lost
across suspends.

The races mentioned above don't seem to be very dangerous.  How likely
is it that the system will be suspended within a few milliseconds of
probing a new USB device?

As for buggy devices and drivers that can't handle persist, we have
better ways of dealing with them.  Buggy devices can get a quirk flag
(USB_QUIRK_RESET).  Buggy drivers should be fixed.

Alan Stern

Re: [PATCH] usb: Make USB persist default configurable

[Vincent Palatin]

On Tue, Mar 19, 2013 at 7:56 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Mon, 18 Mar 2013, Greg Kroah-Hartman wrote:
>
> > On Mon, Mar 18, 2013 at 05:02:19PM -0700, Julius Werner wrote:
> > > > Why can't you just revert this in userspace?  Isn't that easier than
> > > > doing a kernel patch and providing an option that we need to now
> > > > maintain for pretty much forever?
> > >
> > > I could solve it in userspace, but that really feels like a hacky
> > > workaround and not a long term solution. It would mean that every new
> > > device starts with persist enabled and stays that way for a few
> > > milliseconds (maybe up to seconds if it's connected on boot), until
> > > userspace gets around to disable it again... opening the possibility
> > > for very weird race conditions and bugs with drivers/devices that
> > > don't work with persist.
> >
> > What drivers/devices don't work with persist?  We need to know that now,
> > otherwise all other distros and users have problems, right?
> >
> > > This default is a policy that really resides in the kernel, it has
> > > changed in the past, and since there is no definitive better choice
> > > for all cases I thought making it configurable is the right thing to
> > > do.
> >
> > Too many options can be a bad thing.
> >
> > I think Alan made this a "always on" option, so I'd like to get his
> > opinion on it.  Alan?
>
> Originally the "persist" attribute defaulted to "off".  Linus disliked
> this (at least, he disliked it for mass-storage devices) and so at his
> request the default was changed to "on".  There didn't seem to be any
> reason to treat other devices differently from mass-storage devices;
> consequently the default is now "on" for everything.
>
> Julius's commit message mentions the disadvantage of "persist": Resume
> times can be increased.  But it doesn't mention the chief advantage:
> Filesystems stored on USB devices are much less likely to be lost
> across suspends.
>
> The races mentioned above don't seem to be very dangerous.  How likely
> is it that the system will be suspended within a few milliseconds of
> probing a new USB device?

For laptops, if the suspend/resume is triggered by the lid open/close
detection, this is somewhat likely and bit us in the past :
 the classical use case I have encountered is a back-to-back suspend
triggered by the user opening the lid then closing it again in the
next 2 or 3 seconds because he has changed is mind (damn user...),
might be also triggered by lid hall sensor missing proper debouncing
(but in that case, the mechanical time constant is often shorter than
the latency of resuming USB devices).


>
> As for buggy devices and drivers that can't handle persist, we have
> better ways of dealing with them.  Buggy devices can get a quirk flag
> (USB_QUIRK_RESET).  Buggy drivers should be fixed.
>
> Alan Stern
>

-- 
Vincent

Re: [PATCH] usb: Make USB persist default configurable

[Alan Stern]

On Tue, 19 Mar 2013, Vincent Palatin wrote:

> > The races mentioned above don't seem to be very dangerous.  How likely
> > is it that the system will be suspended within a few milliseconds of
> > probing a new USB device?
> 
> For laptops, if the suspend/resume is triggered by the lid open/close
> detection, this is somewhat likely and bit us in the past :
>  the classical use case I have encountered is a back-to-back suspend
> triggered by the user opening the lid then closing it again in the
> next 2 or 3 seconds because he has changed is mind (damn user...),
> might be also triggered by lid hall sensor missing proper debouncing
> (but in that case, the mechanical time constant is often shorter than
> the latency of resuming USB devices).

Yes, okay, that's true.  If a new USB device is plugged in while the 
lid is shut, and then the lid is opened very briefly, it's possible 
that the system could suspend again before the new device's "persist" 
attribute is updated.  Does that case really matter?  The device isn't 
likely to be in use at that point.

Alan Stern

Re: [PATCH] usb: Make USB persist default configurable

[Julius Werner]

> Yes, okay, that's true.  If a new USB device is plugged in while the
> lid is shut, and then the lid is opened very briefly, it's possible
> that the system could suspend again before the new device's "persist"
> attribute is updated.  Does that case really matter?  The device isn't
> likely to be in use at that point.

It does matter if the device will be used after the next resume,
because that one would use the persist code. If there was a driver
problem it would fail, and if it was a USB stick that is swapped with
a different one of the same model, you could get file system
corruption.

I agree with you that buggy drivers should get fixed (and we are
trying to do that as well), but at the same time we want to be able to
have a system that can keep its policies at all times. We get a lot of
USB problems (especially around suspend/resume), and we don't want to
need to worry every time about which code path we hit and whether one
of those race conditions could have something to do with it. I'm
convinced that having this in the kernel is the cleanest solution for
us, and I just thought it might be useful to standardize a mechanism
for that (I don't really see the maintenance burden in that config
option either, as the persist mechanism itself does not look like it
was going to go away anytime soon).

Re: [PATCH] usb: Make USB persist default configurable

[Alan Stern]

On Tue, 19 Mar 2013, Julius Werner wrote:

> > Yes, okay, that's true.  If a new USB device is plugged in while the
> > lid is shut, and then the lid is opened very briefly, it's possible
> > that the system could suspend again before the new device's "persist"
> > attribute is updated.  Does that case really matter?  The device isn't
> > likely to be in use at that point.
> 
> It does matter if the device will be used after the next resume,
> because that one would use the persist code. If there was a driver
> problem it would fail, and if it was a USB stick that is swapped with
> a different one of the same model, you could get file system
> corruption.

To be honest, when it comes to filesystem corruption and lost data, you
should be more worried about the consequences of leaving "persist"
turned off than of leaving it on.  Be that as it may...

> I agree with you that buggy drivers should get fixed (and we are
> trying to do that as well), but at the same time we want to be able to
> have a system that can keep its policies at all times. We get a lot of
> USB problems (especially around suspend/resume), and we don't want to
> need to worry every time about which code path we hit and whether one
> of those race conditions could have something to do with it. I'm
> convinced that having this in the kernel is the cleanest solution for
> us, and I just thought it might be useful to standardize a mechanism
> for that (I don't really see the maintenance burden in that config
> option either, as the persist mechanism itself does not look like it
> was going to go away anytime soon).

If you feel strongly about this, I don't have any real objection to 
adding the Kconfig option.  Practically everyone will leave it in its 
default state and ignore the whole thing.

Alternatively, you could keep the patch out of mainline, as an in-house
addition to your own kernels.  That would require more effort on your 
part, though.

Alan Stern