Re: [PATCH] usb: Make USB persist default configurable
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] usb: Make USB persist default configurable
> 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). -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] usb: Make USB persist default configurable
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] usb: Make USB persist default configurable
On Tue, Mar 19, 2013 at 7:56 AM, Alan Stern 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] usb: Make USB persist default configurable
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] usb: Make USB persist default configurable
> 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] usb: Make USB persist default configurable
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] usb: Make USB persist default configurable
> 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] usb: Make USB persist default configurable
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] usb: Make USB persist default configurable
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 --- 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/