Re: [PATCH] usb: xhci: Disable runtime PM suspend for quirky controllers.
Hi Sarah, I will resubmit the patch with these changes shortly. On Fri, Aug 9, 2013 at 10:22 AM, Sarah Sharp wrote: > Hi Shawn, > > I noticed that the ChromeOS kernel tree is still using this particular > patch, and thought it was probably time to revisit it. > > On Sat, May 25, 2013 at 09:57:57AM -0700, Shawn Nematbakhsh wrote: >> Hi Sarah and Alan, >> >> Thanks for the comments. I will make the following revisions: >> >> 1. Call pm_runtime_get_noresume only when the first device is connected, >> and call pm_runtime_put when the last device is disconnected. >> 2. Wrap the calls in an ifdef CONFIG_USB_DEFAULT_PERSIST. >> 3. Make sure that all pm_runtime_get_noresume calls have a corresponding >> pm_runtime_put somewhere (originally the intent was to disable runtime >> suspend "forever", but no longer). >> >> In principle, would the patch be acceptable with these revisions? > > The thread petered off with all other options turning out to be > dead-ends, so yes, if you made those changes, you could get that patch > upstream. I would like the ChromeOS kernel to be as close to upstream > as possible, so please resubmit this patch with those changes. > > Thanks, > Sarah Sharp > >> >> On Sat, May 25, 2013 at 7:11 AM, Alan Stern wrote: >> >> > On Fri, 24 May 2013, Sarah Sharp wrote: >> > >> > > On Fri, May 24, 2013 at 11:12:57AM -0700, Shawn Nematbakhsh wrote: >> > > > If a USB controller with XHCI_RESET_ON_RESUME goes to runtime suspend, >> > > > a reset will be performed upon runtime resume. Any previously suspended >> > > > devices attached to the controller will be re-enumerated at this time. >> > > > This will cause problems, for example, if an open system call on the >> > > > device triggered the resume (the open call will fail). >> > > >> > > That's ugly, but disabling runtime PM is going to have a big impact on >> > > the power consumption of those systems. >> > > >> > > Alan, do you think this is really the right thing to be doing here? It >> > > feels like userspace should just be able to deal with devices >> > > disconnecting on resume. After all, there are lots of USB devices that >> > > can't handle USB device suspend at all. >> > >> > This is a complicated issue. It depends on the runtime PM settings for >> > both the device and the host controller. >> > >> > As just one aspect, consider the fact that if it wants to, userspace >> > can already prevent the controller from going into runtime suspend. >> > Always preventing this at the kernel level, even when no devices are >> > plugged in, does seem too heavy-handed. >> > >> > > Shouldn't userspace just disable runtime PM for the USB device classes >> > > that don't have a reset resume callback? >> > >> > That's not so easy, because the kernel changes over time. Userspace >> > has no general way to tell which drivers have reset-resume support. >> > >> > > > Note that this change is only relevant when persist_enabled is not set >> > > > for USB devices. >> > > >> > > Could we at least wrap the call in an ifdef CONFIG_USB_DEFAULT_PERSIST? >> > > That way if people have USB persist turned off in their configuration, >> > > their host will still be able to suspend. >> > >> > Not just that; the patch is incorrect on the face of it... >> > >> > > > @@ -4687,6 +4687,12 @@ int xhci_gen_setup(struct usb_hcd *hcd, >> > xhci_get_quirks_t get_quirks) >> > > > >> > > > get_quirks(dev, xhci); >> > > > >> > > > + /* If we are resetting upon resume, we must disable runtime PM. >> > > > +* Otherwise, an open() syscall to a device on our runtime >> > suspended >> > > > +* controller will trigger controller reset and device >> > re-enumeration */ >> > > > + if (xhci->quirks & XHCI_RESET_ON_RESUME) >> > > > + pm_runtime_get_noresume(dev); >> > > > + >> > >> > It adds a pm_runtime_get call with no corresponding pm_runtime_put. >> > >> > 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: xhci: Disable runtime PM suspend for quirky controllers.
Hi Sarah, I will resubmit the patch with these changes shortly. On Fri, Aug 9, 2013 at 10:22 AM, Sarah Sharp sarah.a.sh...@linux.intel.com wrote: Hi Shawn, I noticed that the ChromeOS kernel tree is still using this particular patch, and thought it was probably time to revisit it. On Sat, May 25, 2013 at 09:57:57AM -0700, Shawn Nematbakhsh wrote: Hi Sarah and Alan, Thanks for the comments. I will make the following revisions: 1. Call pm_runtime_get_noresume only when the first device is connected, and call pm_runtime_put when the last device is disconnected. 2. Wrap the calls in an ifdef CONFIG_USB_DEFAULT_PERSIST. 3. Make sure that all pm_runtime_get_noresume calls have a corresponding pm_runtime_put somewhere (originally the intent was to disable runtime suspend forever, but no longer). In principle, would the patch be acceptable with these revisions? The thread petered off with all other options turning out to be dead-ends, so yes, if you made those changes, you could get that patch upstream. I would like the ChromeOS kernel to be as close to upstream as possible, so please resubmit this patch with those changes. Thanks, Sarah Sharp On Sat, May 25, 2013 at 7:11 AM, Alan Stern st...@rowland.harvard.eduwrote: On Fri, 24 May 2013, Sarah Sharp wrote: On Fri, May 24, 2013 at 11:12:57AM -0700, Shawn Nematbakhsh wrote: If a USB controller with XHCI_RESET_ON_RESUME goes to runtime suspend, a reset will be performed upon runtime resume. Any previously suspended devices attached to the controller will be re-enumerated at this time. This will cause problems, for example, if an open system call on the device triggered the resume (the open call will fail). That's ugly, but disabling runtime PM is going to have a big impact on the power consumption of those systems. Alan, do you think this is really the right thing to be doing here? It feels like userspace should just be able to deal with devices disconnecting on resume. After all, there are lots of USB devices that can't handle USB device suspend at all. This is a complicated issue. It depends on the runtime PM settings for both the device and the host controller. As just one aspect, consider the fact that if it wants to, userspace can already prevent the controller from going into runtime suspend. Always preventing this at the kernel level, even when no devices are plugged in, does seem too heavy-handed. Shouldn't userspace just disable runtime PM for the USB device classes that don't have a reset resume callback? That's not so easy, because the kernel changes over time. Userspace has no general way to tell which drivers have reset-resume support. Note that this change is only relevant when persist_enabled is not set for USB devices. Could we at least wrap the call in an ifdef CONFIG_USB_DEFAULT_PERSIST? That way if people have USB persist turned off in their configuration, their host will still be able to suspend. Not just that; the patch is incorrect on the face of it... @@ -4687,6 +4687,12 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks) get_quirks(dev, xhci); + /* If we are resetting upon resume, we must disable runtime PM. +* Otherwise, an open() syscall to a device on our runtime suspended +* controller will trigger controller reset and device re-enumeration */ + if (xhci-quirks XHCI_RESET_ON_RESUME) + pm_runtime_get_noresume(dev); + It adds a pm_runtime_get call with no corresponding pm_runtime_put. 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: xhci: Disable runtime PM suspend for quirky controllers.
Hi Shawn, I noticed that the ChromeOS kernel tree is still using this particular patch, and thought it was probably time to revisit it. On Sat, May 25, 2013 at 09:57:57AM -0700, Shawn Nematbakhsh wrote: > Hi Sarah and Alan, > > Thanks for the comments. I will make the following revisions: > > 1. Call pm_runtime_get_noresume only when the first device is connected, > and call pm_runtime_put when the last device is disconnected. > 2. Wrap the calls in an ifdef CONFIG_USB_DEFAULT_PERSIST. > 3. Make sure that all pm_runtime_get_noresume calls have a corresponding > pm_runtime_put somewhere (originally the intent was to disable runtime > suspend "forever", but no longer). > > In principle, would the patch be acceptable with these revisions? The thread petered off with all other options turning out to be dead-ends, so yes, if you made those changes, you could get that patch upstream. I would like the ChromeOS kernel to be as close to upstream as possible, so please resubmit this patch with those changes. Thanks, Sarah Sharp > > On Sat, May 25, 2013 at 7:11 AM, Alan Stern wrote: > > > On Fri, 24 May 2013, Sarah Sharp wrote: > > > > > On Fri, May 24, 2013 at 11:12:57AM -0700, Shawn Nematbakhsh wrote: > > > > If a USB controller with XHCI_RESET_ON_RESUME goes to runtime suspend, > > > > a reset will be performed upon runtime resume. Any previously suspended > > > > devices attached to the controller will be re-enumerated at this time. > > > > This will cause problems, for example, if an open system call on the > > > > device triggered the resume (the open call will fail). > > > > > > That's ugly, but disabling runtime PM is going to have a big impact on > > > the power consumption of those systems. > > > > > > Alan, do you think this is really the right thing to be doing here? It > > > feels like userspace should just be able to deal with devices > > > disconnecting on resume. After all, there are lots of USB devices that > > > can't handle USB device suspend at all. > > > > This is a complicated issue. It depends on the runtime PM settings for > > both the device and the host controller. > > > > As just one aspect, consider the fact that if it wants to, userspace > > can already prevent the controller from going into runtime suspend. > > Always preventing this at the kernel level, even when no devices are > > plugged in, does seem too heavy-handed. > > > > > Shouldn't userspace just disable runtime PM for the USB device classes > > > that don't have a reset resume callback? > > > > That's not so easy, because the kernel changes over time. Userspace > > has no general way to tell which drivers have reset-resume support. > > > > > > Note that this change is only relevant when persist_enabled is not set > > > > for USB devices. > > > > > > Could we at least wrap the call in an ifdef CONFIG_USB_DEFAULT_PERSIST? > > > That way if people have USB persist turned off in their configuration, > > > their host will still be able to suspend. > > > > Not just that; the patch is incorrect on the face of it... > > > > > > @@ -4687,6 +4687,12 @@ int xhci_gen_setup(struct usb_hcd *hcd, > > xhci_get_quirks_t get_quirks) > > > > > > > > get_quirks(dev, xhci); > > > > > > > > + /* If we are resetting upon resume, we must disable runtime PM. > > > > +* Otherwise, an open() syscall to a device on our runtime > > suspended > > > > +* controller will trigger controller reset and device > > re-enumeration */ > > > > + if (xhci->quirks & XHCI_RESET_ON_RESUME) > > > > + pm_runtime_get_noresume(dev); > > > > + > > > > It adds a pm_runtime_get call with no corresponding pm_runtime_put. > > > > 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: xhci: Disable runtime PM suspend for quirky controllers.
Hi Shawn, I noticed that the ChromeOS kernel tree is still using this particular patch, and thought it was probably time to revisit it. On Sat, May 25, 2013 at 09:57:57AM -0700, Shawn Nematbakhsh wrote: Hi Sarah and Alan, Thanks for the comments. I will make the following revisions: 1. Call pm_runtime_get_noresume only when the first device is connected, and call pm_runtime_put when the last device is disconnected. 2. Wrap the calls in an ifdef CONFIG_USB_DEFAULT_PERSIST. 3. Make sure that all pm_runtime_get_noresume calls have a corresponding pm_runtime_put somewhere (originally the intent was to disable runtime suspend forever, but no longer). In principle, would the patch be acceptable with these revisions? The thread petered off with all other options turning out to be dead-ends, so yes, if you made those changes, you could get that patch upstream. I would like the ChromeOS kernel to be as close to upstream as possible, so please resubmit this patch with those changes. Thanks, Sarah Sharp On Sat, May 25, 2013 at 7:11 AM, Alan Stern st...@rowland.harvard.eduwrote: On Fri, 24 May 2013, Sarah Sharp wrote: On Fri, May 24, 2013 at 11:12:57AM -0700, Shawn Nematbakhsh wrote: If a USB controller with XHCI_RESET_ON_RESUME goes to runtime suspend, a reset will be performed upon runtime resume. Any previously suspended devices attached to the controller will be re-enumerated at this time. This will cause problems, for example, if an open system call on the device triggered the resume (the open call will fail). That's ugly, but disabling runtime PM is going to have a big impact on the power consumption of those systems. Alan, do you think this is really the right thing to be doing here? It feels like userspace should just be able to deal with devices disconnecting on resume. After all, there are lots of USB devices that can't handle USB device suspend at all. This is a complicated issue. It depends on the runtime PM settings for both the device and the host controller. As just one aspect, consider the fact that if it wants to, userspace can already prevent the controller from going into runtime suspend. Always preventing this at the kernel level, even when no devices are plugged in, does seem too heavy-handed. Shouldn't userspace just disable runtime PM for the USB device classes that don't have a reset resume callback? That's not so easy, because the kernel changes over time. Userspace has no general way to tell which drivers have reset-resume support. Note that this change is only relevant when persist_enabled is not set for USB devices. Could we at least wrap the call in an ifdef CONFIG_USB_DEFAULT_PERSIST? That way if people have USB persist turned off in their configuration, their host will still be able to suspend. Not just that; the patch is incorrect on the face of it... @@ -4687,6 +4687,12 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks) get_quirks(dev, xhci); + /* If we are resetting upon resume, we must disable runtime PM. +* Otherwise, an open() syscall to a device on our runtime suspended +* controller will trigger controller reset and device re-enumeration */ + if (xhci-quirks XHCI_RESET_ON_RESUME) + pm_runtime_get_noresume(dev); + It adds a pm_runtime_get call with no corresponding pm_runtime_put. 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: xhci: Disable runtime PM suspend for quirky controllers.
On Wednesday 29 May 2013 16:32:38 Don Zickus wrote: > On Wed, May 29, 2013 at 12:38:28PM -0700, Sarah Sharp wrote: > > On Tue, May 28, 2013 at 02:41:18PM -0700, Julius Werner wrote: > > > The policy we want to achieve is to disable runtime PM iff there is a > > > device connected that doesn't have persist_enabled or a reset_resume() > > > handler and whose parent/root hub resets on resume, right? > > > > Makes sense. However, not all distros may want that policy, so there > > should be a way to change that policy via sysfs. Some distros may > > choose to take the power savings over having a particular USB device > > work, especially in the server market. > > > > Don, Oliver, what do you think of this patch: > > http://marc.info/?l=linux-usb=136941922715772=2 > > That is limited only to certain controllers right? RHEL6 doesn't support > runtime suspend, so we don't hear to many complaints. Most of our server > customers don't have much plugged into USB, so I don't expect much > problems there. Our laptop customers prefer the power savings, but I > don't know how many of them have chipsets with XHCI_RESET_ON_RESUME. Power savings are good, but reliability is better. For what it's worth,ior I like the patch. It is a logical extension of the current behavior. Regards Oliver -- 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: xhci: Disable runtime PM suspend for quirky controllers.
On Wednesday 29 May 2013 16:32:38 Don Zickus wrote: On Wed, May 29, 2013 at 12:38:28PM -0700, Sarah Sharp wrote: On Tue, May 28, 2013 at 02:41:18PM -0700, Julius Werner wrote: The policy we want to achieve is to disable runtime PM iff there is a device connected that doesn't have persist_enabled or a reset_resume() handler and whose parent/root hub resets on resume, right? Makes sense. However, not all distros may want that policy, so there should be a way to change that policy via sysfs. Some distros may choose to take the power savings over having a particular USB device work, especially in the server market. Don, Oliver, what do you think of this patch: http://marc.info/?l=linux-usbm=136941922715772w=2 That is limited only to certain controllers right? RHEL6 doesn't support runtime suspend, so we don't hear to many complaints. Most of our server customers don't have much plugged into USB, so I don't expect much problems there. Our laptop customers prefer the power savings, but I don't know how many of them have chipsets with XHCI_RESET_ON_RESUME. Power savings are good, but reliability is better. For what it's worth,ior I like the patch. It is a logical extension of the current behavior. Regards Oliver -- 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: xhci: Disable runtime PM suspend for quirky controllers.
On Wed, May 29, 2013 at 12:38:28PM -0700, Sarah Sharp wrote: > On Tue, May 28, 2013 at 02:41:18PM -0700, Julius Werner wrote: > > The policy we want to achieve is to disable runtime PM iff there is a > > device connected that doesn't have persist_enabled or a reset_resume() > > handler and whose parent/root hub resets on resume, right? > > Makes sense. However, not all distros may want that policy, so there > should be a way to change that policy via sysfs. Some distros may > choose to take the power savings over having a particular USB device > work, especially in the server market. > > Don, Oliver, what do you think of this patch: > http://marc.info/?l=linux-usb=136941922715772=2 That is limited only to certain controllers right? RHEL6 doesn't support runtime suspend, so we don't hear to many complaints. Most of our server customers don't have much plugged into USB, so I don't expect much problems there. Our laptop customers prefer the power savings, but I don't know how many of them have chipsets with XHCI_RESET_ON_RESUME. > > Julius is proposing to limit the scope of the patch a bit, but the > impact will still be that TI hosts will be active more often than not. Hmm, for some reason I don't see TI having the XHCI_RESET_ON_RESUME quirk set, just VIA and ETRON. Neither of which seem to be normally shipped with servers. Cheers, Don -- 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: xhci: Disable runtime PM suspend for quirky controllers.
On Wed, 29 May 2013, Sarah Sharp wrote: > On Tue, May 28, 2013 at 02:41:18PM -0700, Julius Werner wrote: > > The policy we want to achieve is to disable runtime PM iff there is a > > device connected that doesn't have persist_enabled or a reset_resume() > > handler and whose parent/root hub resets on resume, right? > > Makes sense. However, not all distros may want that policy, so there > should be a way to change that policy via sysfs. Some distros may > choose to take the power savings over having a particular USB device > work, especially in the server market. > > Don, Oliver, what do you think of this patch: > http://marc.info/?l=linux-usb=136941922715772=2 > > Julius is proposing to limit the scope of the patch a bit, but the > impact will still be that TI hosts will be active more often than not. In many cases, the usual default of allowing the root hub to autosuspend will be acceptable. In cases where it isn't, I think we will have to rely on userspace to tell us. The simplest way is for userspace to forbid autosuspend. That may not be flexible enough, but at this point there doesn't seem to be any way for the kernel to include all the different policies that userspace might want. All we can do is make the information available. There already is a "quirks" attribute in sysfs, but it's probably not good enough for this. The contents are subject to change if we renumber the QUIRK bits. Maybe something more like the "avoid_reset" attribute. A problem with registering sysfs attributes from within xhci-hcd is that they won't become visible until some time after the device is registered. If a udev script runs too quickly, it won't see the attribute. > > So couldn't > > we remove the HCD-specific XHCI_RESET_ON_RESUME and set the (existing) > > generic USB_QUIRK_RESET_RESUME on the root hub instead? Then we could > > handle all of this in the USB core (during device initialization and > > when changing persist_enabled through sysfs) by just checking for > > udev->reset_resume on all parent hubs of the device in question (and > > use pm_runtime_get/put() on said device to prevent its parents from > > suspending as appropriate). > > Alan, what happens if we set USB_QUIRK_RESET_RESUME on the roothub? > I don't think that currently translates into the host controller's Reset > register getting written, which is what I think Julius is proposing. Hmmm. Now that I look more closely, setting the RESET_RESUME quirk on the root hub would prevent it from ever going into runtime suspend, which is not what we are after. (The quirk disables autosuspend for devices whose drivers don't support reset-resume or require remote wakeup.) Oh, well. 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: xhci: Disable runtime PM suspend for quirky controllers.
On Tue, May 28, 2013 at 02:41:18PM -0700, Julius Werner wrote: > The policy we want to achieve is to disable runtime PM iff there is a > device connected that doesn't have persist_enabled or a reset_resume() > handler and whose parent/root hub resets on resume, right? Makes sense. However, not all distros may want that policy, so there should be a way to change that policy via sysfs. Some distros may choose to take the power savings over having a particular USB device work, especially in the server market. Don, Oliver, what do you think of this patch: http://marc.info/?l=linux-usb=136941922715772=2 Julius is proposing to limit the scope of the patch a bit, but the impact will still be that TI hosts will be active more often than not. > So couldn't > we remove the HCD-specific XHCI_RESET_ON_RESUME and set the (existing) > generic USB_QUIRK_RESET_RESUME on the root hub instead? Then we could > handle all of this in the USB core (during device initialization and > when changing persist_enabled through sysfs) by just checking for > udev->reset_resume on all parent hubs of the device in question (and > use pm_runtime_get/put() on said device to prevent its parents from > suspending as appropriate). Alan, what happens if we set USB_QUIRK_RESET_RESUME on the roothub? I don't think that currently translates into the host controller's Reset register getting written, which is what I think Julius is proposing. Sarah Sharp -- 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: xhci: Disable runtime PM suspend for quirky controllers.
On Tue, 28 May 2013, Julius Werner wrote: > The policy we want to achieve is to disable runtime PM iff there is a > device connected that doesn't have persist_enabled or a reset_resume() > handler and whose parent/root hub resets on resume, right? So couldn't Probably just root hub, not parent. A non-root hub that resets upon resume wouldn't be a good idea. Also, we know in advance that the hub driver _does_ have a reset-resume handler. > we remove the HCD-specific XHCI_RESET_ON_RESUME and set the (existing) > generic USB_QUIRK_RESET_RESUME on the root hub instead? Then we could > handle all of this in the USB core (during device initialization and > when changing persist_enabled through sysfs) by just checking for > udev->reset_resume on all parent hubs of the device in question (and > use pm_runtime_get/put() on said device to prevent its parents from > suspending as appropriate). This sounds too intricate to me. You might want to prevent resets even if the device does support reset-resume, because they consume time. Or you might not care about resets even if persist isn't enabled (consider a USB mouse, for example). I agree that setting the RESET_RESUME quirk on the root hub is a good way to represent the situation. And perhaps the kernel could implement a useful default policy -- but userspace should ultimately remain in control. 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: xhci: Disable runtime PM suspend for quirky controllers.
On Tue, 28 May 2013, Julius Werner wrote: The policy we want to achieve is to disable runtime PM iff there is a device connected that doesn't have persist_enabled or a reset_resume() handler and whose parent/root hub resets on resume, right? So couldn't Probably just root hub, not parent. A non-root hub that resets upon resume wouldn't be a good idea. Also, we know in advance that the hub driver _does_ have a reset-resume handler. we remove the HCD-specific XHCI_RESET_ON_RESUME and set the (existing) generic USB_QUIRK_RESET_RESUME on the root hub instead? Then we could handle all of this in the USB core (during device initialization and when changing persist_enabled through sysfs) by just checking for udev-reset_resume on all parent hubs of the device in question (and use pm_runtime_get/put() on said device to prevent its parents from suspending as appropriate). This sounds too intricate to me. You might want to prevent resets even if the device does support reset-resume, because they consume time. Or you might not care about resets even if persist isn't enabled (consider a USB mouse, for example). I agree that setting the RESET_RESUME quirk on the root hub is a good way to represent the situation. And perhaps the kernel could implement a useful default policy -- but userspace should ultimately remain in control. 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: xhci: Disable runtime PM suspend for quirky controllers.
On Tue, May 28, 2013 at 02:41:18PM -0700, Julius Werner wrote: The policy we want to achieve is to disable runtime PM iff there is a device connected that doesn't have persist_enabled or a reset_resume() handler and whose parent/root hub resets on resume, right? Makes sense. However, not all distros may want that policy, so there should be a way to change that policy via sysfs. Some distros may choose to take the power savings over having a particular USB device work, especially in the server market. Don, Oliver, what do you think of this patch: http://marc.info/?l=linux-usbm=136941922715772w=2 Julius is proposing to limit the scope of the patch a bit, but the impact will still be that TI hosts will be active more often than not. So couldn't we remove the HCD-specific XHCI_RESET_ON_RESUME and set the (existing) generic USB_QUIRK_RESET_RESUME on the root hub instead? Then we could handle all of this in the USB core (during device initialization and when changing persist_enabled through sysfs) by just checking for udev-reset_resume on all parent hubs of the device in question (and use pm_runtime_get/put() on said device to prevent its parents from suspending as appropriate). Alan, what happens if we set USB_QUIRK_RESET_RESUME on the roothub? I don't think that currently translates into the host controller's Reset register getting written, which is what I think Julius is proposing. Sarah Sharp -- 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: xhci: Disable runtime PM suspend for quirky controllers.
On Wed, 29 May 2013, Sarah Sharp wrote: On Tue, May 28, 2013 at 02:41:18PM -0700, Julius Werner wrote: The policy we want to achieve is to disable runtime PM iff there is a device connected that doesn't have persist_enabled or a reset_resume() handler and whose parent/root hub resets on resume, right? Makes sense. However, not all distros may want that policy, so there should be a way to change that policy via sysfs. Some distros may choose to take the power savings over having a particular USB device work, especially in the server market. Don, Oliver, what do you think of this patch: http://marc.info/?l=linux-usbm=136941922715772w=2 Julius is proposing to limit the scope of the patch a bit, but the impact will still be that TI hosts will be active more often than not. In many cases, the usual default of allowing the root hub to autosuspend will be acceptable. In cases where it isn't, I think we will have to rely on userspace to tell us. The simplest way is for userspace to forbid autosuspend. That may not be flexible enough, but at this point there doesn't seem to be any way for the kernel to include all the different policies that userspace might want. All we can do is make the information available. There already is a quirks attribute in sysfs, but it's probably not good enough for this. The contents are subject to change if we renumber the QUIRK bits. Maybe something more like the avoid_reset attribute. A problem with registering sysfs attributes from within xhci-hcd is that they won't become visible until some time after the device is registered. If a udev script runs too quickly, it won't see the attribute. So couldn't we remove the HCD-specific XHCI_RESET_ON_RESUME and set the (existing) generic USB_QUIRK_RESET_RESUME on the root hub instead? Then we could handle all of this in the USB core (during device initialization and when changing persist_enabled through sysfs) by just checking for udev-reset_resume on all parent hubs of the device in question (and use pm_runtime_get/put() on said device to prevent its parents from suspending as appropriate). Alan, what happens if we set USB_QUIRK_RESET_RESUME on the roothub? I don't think that currently translates into the host controller's Reset register getting written, which is what I think Julius is proposing. Hmmm. Now that I look more closely, setting the RESET_RESUME quirk on the root hub would prevent it from ever going into runtime suspend, which is not what we are after. (The quirk disables autosuspend for devices whose drivers don't support reset-resume or require remote wakeup.) Oh, well. 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: xhci: Disable runtime PM suspend for quirky controllers.
On Wed, May 29, 2013 at 12:38:28PM -0700, Sarah Sharp wrote: On Tue, May 28, 2013 at 02:41:18PM -0700, Julius Werner wrote: The policy we want to achieve is to disable runtime PM iff there is a device connected that doesn't have persist_enabled or a reset_resume() handler and whose parent/root hub resets on resume, right? Makes sense. However, not all distros may want that policy, so there should be a way to change that policy via sysfs. Some distros may choose to take the power savings over having a particular USB device work, especially in the server market. Don, Oliver, what do you think of this patch: http://marc.info/?l=linux-usbm=136941922715772w=2 That is limited only to certain controllers right? RHEL6 doesn't support runtime suspend, so we don't hear to many complaints. Most of our server customers don't have much plugged into USB, so I don't expect much problems there. Our laptop customers prefer the power savings, but I don't know how many of them have chipsets with XHCI_RESET_ON_RESUME. Julius is proposing to limit the scope of the patch a bit, but the impact will still be that TI hosts will be active more often than not. Hmm, for some reason I don't see TI having the XHCI_RESET_ON_RESUME quirk set, just VIA and ETRON. Neither of which seem to be normally shipped with servers. Cheers, Don -- 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: xhci: Disable runtime PM suspend for quirky controllers.
The policy we want to achieve is to disable runtime PM iff there is a device connected that doesn't have persist_enabled or a reset_resume() handler and whose parent/root hub resets on resume, right? So couldn't we remove the HCD-specific XHCI_RESET_ON_RESUME and set the (existing) generic USB_QUIRK_RESET_RESUME on the root hub instead? Then we could handle all of this in the USB core (during device initialization and when changing persist_enabled through sysfs) by just checking for udev->reset_resume on all parent hubs of the device in question (and use pm_runtime_get/put() on said device to prevent its parents from suspending as appropriate). -- 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: xhci: Disable runtime PM suspend for quirky controllers.
On Sat, May 25, 2013 at 09:57:57AM -0700, Shawn Nematbakhsh wrote: > Hi Sarah and Alan, > > Thanks for the comments. I will make the following revisions: > > 1. Call pm_runtime_get_noresume only when the first device is connected, > and call pm_runtime_put when the last device is disconnected. > 2. Wrap the calls in an ifdef CONFIG_USB_DEFAULT_PERSIST. > 3. Make sure that all pm_runtime_get_noresume calls have a corresponding > pm_runtime_put somewhere (originally the intent was to disable runtime > suspend "forever", but no longer). > > In principle, would the patch be acceptable with these revisions? Maybe, but I still don't like this approach, because it's too heavy-handed. I was considering whether userspace could do something similar to this approach, but with udev rules instead of within the kernel. You could add a udev rule to trigger on USB device insertion, that would disable runtime PM for the host, and a corresponding rule that re-enabled runtime PM when the last USB device was disconnected. I think it could be possible if userspace can get to the DMI information for the system. However, then we open the other can of worms by needing to keep the userspace quirks list in sync with the kernel quirks list. What if we exposed the xHCI quirks through a new quirks file in /sys/bus/usb/devices/usbN/? That would mean userspace doesn't need to keep the quirks list separately. Sarah Sharp -- 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: xhci: Disable runtime PM suspend for quirky controllers.
On Sat, May 25, 2013 at 09:57:57AM -0700, Shawn Nematbakhsh wrote: Hi Sarah and Alan, Thanks for the comments. I will make the following revisions: 1. Call pm_runtime_get_noresume only when the first device is connected, and call pm_runtime_put when the last device is disconnected. 2. Wrap the calls in an ifdef CONFIG_USB_DEFAULT_PERSIST. 3. Make sure that all pm_runtime_get_noresume calls have a corresponding pm_runtime_put somewhere (originally the intent was to disable runtime suspend forever, but no longer). In principle, would the patch be acceptable with these revisions? Maybe, but I still don't like this approach, because it's too heavy-handed. I was considering whether userspace could do something similar to this approach, but with udev rules instead of within the kernel. You could add a udev rule to trigger on USB device insertion, that would disable runtime PM for the host, and a corresponding rule that re-enabled runtime PM when the last USB device was disconnected. I think it could be possible if userspace can get to the DMI information for the system. However, then we open the other can of worms by needing to keep the userspace quirks list in sync with the kernel quirks list. What if we exposed the xHCI quirks through a new quirks file in /sys/bus/usb/devices/usbN/? That would mean userspace doesn't need to keep the quirks list separately. Sarah Sharp -- 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: xhci: Disable runtime PM suspend for quirky controllers.
The policy we want to achieve is to disable runtime PM iff there is a device connected that doesn't have persist_enabled or a reset_resume() handler and whose parent/root hub resets on resume, right? So couldn't we remove the HCD-specific XHCI_RESET_ON_RESUME and set the (existing) generic USB_QUIRK_RESET_RESUME on the root hub instead? Then we could handle all of this in the USB core (during device initialization and when changing persist_enabled through sysfs) by just checking for udev-reset_resume on all parent hubs of the device in question (and use pm_runtime_get/put() on said device to prevent its parents from suspending as appropriate). -- 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: xhci: Disable runtime PM suspend for quirky controllers.
Hi Sarah and Alan, Thanks for the comments. I will make the following revisions: 1. Call pm_runtime_get_noresume only when the first device is connected, and call pm_runtime_put when the last device is disconnected. 2. Wrap the calls in an ifdef CONFIG_USB_DEFAULT_PERSIST. 3. Make sure that all pm_runtime_get_noresume calls have a corresponding pm_runtime_put somewhere (originally the intent was to disable runtime suspend "forever", but no longer). In principle, would the patch be acceptable with these revisions? On Sat, May 25, 2013 at 7:11 AM, Alan Stern wrote: > On Fri, 24 May 2013, Sarah Sharp wrote: > >> On Fri, May 24, 2013 at 11:12:57AM -0700, Shawn Nematbakhsh wrote: >> > If a USB controller with XHCI_RESET_ON_RESUME goes to runtime suspend, >> > a reset will be performed upon runtime resume. Any previously suspended >> > devices attached to the controller will be re-enumerated at this time. >> > This will cause problems, for example, if an open system call on the >> > device triggered the resume (the open call will fail). >> >> That's ugly, but disabling runtime PM is going to have a big impact on >> the power consumption of those systems. >> >> Alan, do you think this is really the right thing to be doing here? It >> feels like userspace should just be able to deal with devices >> disconnecting on resume. After all, there are lots of USB devices that >> can't handle USB device suspend at all. > > This is a complicated issue. It depends on the runtime PM settings for > both the device and the host controller. > > As just one aspect, consider the fact that if it wants to, userspace > can already prevent the controller from going into runtime suspend. > Always preventing this at the kernel level, even when no devices are > plugged in, does seem too heavy-handed. > >> Shouldn't userspace just disable runtime PM for the USB device classes >> that don't have a reset resume callback? > > That's not so easy, because the kernel changes over time. Userspace > has no general way to tell which drivers have reset-resume support. > >> > Note that this change is only relevant when persist_enabled is not set >> > for USB devices. >> >> Could we at least wrap the call in an ifdef CONFIG_USB_DEFAULT_PERSIST? >> That way if people have USB persist turned off in their configuration, >> their host will still be able to suspend. > > Not just that; the patch is incorrect on the face of it... > >> > @@ -4687,6 +4687,12 @@ int xhci_gen_setup(struct usb_hcd *hcd, >> > xhci_get_quirks_t get_quirks) >> > >> > get_quirks(dev, xhci); >> > >> > + /* If we are resetting upon resume, we must disable runtime PM. >> > +* Otherwise, an open() syscall to a device on our runtime suspended >> > +* controller will trigger controller reset and device re-enumeration >> > */ >> > + if (xhci->quirks & XHCI_RESET_ON_RESUME) >> > + pm_runtime_get_noresume(dev); >> > + > > It adds a pm_runtime_get call with no corresponding pm_runtime_put. > > 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: xhci: Disable runtime PM suspend for quirky controllers.
On Fri, 24 May 2013, Sarah Sharp wrote: > On Fri, May 24, 2013 at 11:12:57AM -0700, Shawn Nematbakhsh wrote: > > If a USB controller with XHCI_RESET_ON_RESUME goes to runtime suspend, > > a reset will be performed upon runtime resume. Any previously suspended > > devices attached to the controller will be re-enumerated at this time. > > This will cause problems, for example, if an open system call on the > > device triggered the resume (the open call will fail). > > That's ugly, but disabling runtime PM is going to have a big impact on > the power consumption of those systems. > > Alan, do you think this is really the right thing to be doing here? It > feels like userspace should just be able to deal with devices > disconnecting on resume. After all, there are lots of USB devices that > can't handle USB device suspend at all. This is a complicated issue. It depends on the runtime PM settings for both the device and the host controller. As just one aspect, consider the fact that if it wants to, userspace can already prevent the controller from going into runtime suspend. Always preventing this at the kernel level, even when no devices are plugged in, does seem too heavy-handed. > Shouldn't userspace just disable runtime PM for the USB device classes > that don't have a reset resume callback? That's not so easy, because the kernel changes over time. Userspace has no general way to tell which drivers have reset-resume support. > > Note that this change is only relevant when persist_enabled is not set > > for USB devices. > > Could we at least wrap the call in an ifdef CONFIG_USB_DEFAULT_PERSIST? > That way if people have USB persist turned off in their configuration, > their host will still be able to suspend. Not just that; the patch is incorrect on the face of it... > > @@ -4687,6 +4687,12 @@ int xhci_gen_setup(struct usb_hcd *hcd, > > xhci_get_quirks_t get_quirks) > > > > get_quirks(dev, xhci); > > > > + /* If we are resetting upon resume, we must disable runtime PM. > > +* Otherwise, an open() syscall to a device on our runtime suspended > > +* controller will trigger controller reset and device re-enumeration */ > > + if (xhci->quirks & XHCI_RESET_ON_RESUME) > > + pm_runtime_get_noresume(dev); > > + It adds a pm_runtime_get call with no corresponding pm_runtime_put. 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: xhci: Disable runtime PM suspend for quirky controllers.
On Fri, 24 May 2013, Sarah Sharp wrote: On Fri, May 24, 2013 at 11:12:57AM -0700, Shawn Nematbakhsh wrote: If a USB controller with XHCI_RESET_ON_RESUME goes to runtime suspend, a reset will be performed upon runtime resume. Any previously suspended devices attached to the controller will be re-enumerated at this time. This will cause problems, for example, if an open system call on the device triggered the resume (the open call will fail). That's ugly, but disabling runtime PM is going to have a big impact on the power consumption of those systems. Alan, do you think this is really the right thing to be doing here? It feels like userspace should just be able to deal with devices disconnecting on resume. After all, there are lots of USB devices that can't handle USB device suspend at all. This is a complicated issue. It depends on the runtime PM settings for both the device and the host controller. As just one aspect, consider the fact that if it wants to, userspace can already prevent the controller from going into runtime suspend. Always preventing this at the kernel level, even when no devices are plugged in, does seem too heavy-handed. Shouldn't userspace just disable runtime PM for the USB device classes that don't have a reset resume callback? That's not so easy, because the kernel changes over time. Userspace has no general way to tell which drivers have reset-resume support. Note that this change is only relevant when persist_enabled is not set for USB devices. Could we at least wrap the call in an ifdef CONFIG_USB_DEFAULT_PERSIST? That way if people have USB persist turned off in their configuration, their host will still be able to suspend. Not just that; the patch is incorrect on the face of it... @@ -4687,6 +4687,12 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks) get_quirks(dev, xhci); + /* If we are resetting upon resume, we must disable runtime PM. +* Otherwise, an open() syscall to a device on our runtime suspended +* controller will trigger controller reset and device re-enumeration */ + if (xhci-quirks XHCI_RESET_ON_RESUME) + pm_runtime_get_noresume(dev); + It adds a pm_runtime_get call with no corresponding pm_runtime_put. 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: xhci: Disable runtime PM suspend for quirky controllers.
Hi Sarah and Alan, Thanks for the comments. I will make the following revisions: 1. Call pm_runtime_get_noresume only when the first device is connected, and call pm_runtime_put when the last device is disconnected. 2. Wrap the calls in an ifdef CONFIG_USB_DEFAULT_PERSIST. 3. Make sure that all pm_runtime_get_noresume calls have a corresponding pm_runtime_put somewhere (originally the intent was to disable runtime suspend forever, but no longer). In principle, would the patch be acceptable with these revisions? On Sat, May 25, 2013 at 7:11 AM, Alan Stern st...@rowland.harvard.edu wrote: On Fri, 24 May 2013, Sarah Sharp wrote: On Fri, May 24, 2013 at 11:12:57AM -0700, Shawn Nematbakhsh wrote: If a USB controller with XHCI_RESET_ON_RESUME goes to runtime suspend, a reset will be performed upon runtime resume. Any previously suspended devices attached to the controller will be re-enumerated at this time. This will cause problems, for example, if an open system call on the device triggered the resume (the open call will fail). That's ugly, but disabling runtime PM is going to have a big impact on the power consumption of those systems. Alan, do you think this is really the right thing to be doing here? It feels like userspace should just be able to deal with devices disconnecting on resume. After all, there are lots of USB devices that can't handle USB device suspend at all. This is a complicated issue. It depends on the runtime PM settings for both the device and the host controller. As just one aspect, consider the fact that if it wants to, userspace can already prevent the controller from going into runtime suspend. Always preventing this at the kernel level, even when no devices are plugged in, does seem too heavy-handed. Shouldn't userspace just disable runtime PM for the USB device classes that don't have a reset resume callback? That's not so easy, because the kernel changes over time. Userspace has no general way to tell which drivers have reset-resume support. Note that this change is only relevant when persist_enabled is not set for USB devices. Could we at least wrap the call in an ifdef CONFIG_USB_DEFAULT_PERSIST? That way if people have USB persist turned off in their configuration, their host will still be able to suspend. Not just that; the patch is incorrect on the face of it... @@ -4687,6 +4687,12 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks) get_quirks(dev, xhci); + /* If we are resetting upon resume, we must disable runtime PM. +* Otherwise, an open() syscall to a device on our runtime suspended +* controller will trigger controller reset and device re-enumeration */ + if (xhci-quirks XHCI_RESET_ON_RESUME) + pm_runtime_get_noresume(dev); + It adds a pm_runtime_get call with no corresponding pm_runtime_put. 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: xhci: Disable runtime PM suspend for quirky controllers.
On Fri, May 24, 2013 at 11:12:57AM -0700, Shawn Nematbakhsh wrote: > If a USB controller with XHCI_RESET_ON_RESUME goes to runtime suspend, > a reset will be performed upon runtime resume. Any previously suspended > devices attached to the controller will be re-enumerated at this time. > This will cause problems, for example, if an open system call on the > device triggered the resume (the open call will fail). That's ugly, but disabling runtime PM is going to have a big impact on the power consumption of those systems. Alan, do you think this is really the right thing to be doing here? It feels like userspace should just be able to deal with devices disconnecting on resume. After all, there are lots of USB devices that can't handle USB device suspend at all. Shouldn't userspace just disable runtime PM for the USB device classes that don't have a reset resume callback? > Note that this change is only relevant when persist_enabled is not set > for USB devices. Could we at least wrap the call in an ifdef CONFIG_USB_DEFAULT_PERSIST? That way if people have USB persist turned off in their configuration, their host will still be able to suspend. Sarah Sharp > > Signed-off-by: Shawn Nematbakhsh > --- > drivers/usb/host/xhci.c | 6 ++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c > index b4aa79d..7455156 100644 > --- a/drivers/usb/host/xhci.c > +++ b/drivers/usb/host/xhci.c > @@ -4687,6 +4687,12 @@ int xhci_gen_setup(struct usb_hcd *hcd, > xhci_get_quirks_t get_quirks) > > get_quirks(dev, xhci); > > + /* If we are resetting upon resume, we must disable runtime PM. > + * Otherwise, an open() syscall to a device on our runtime suspended > + * controller will trigger controller reset and device re-enumeration */ > + if (xhci->quirks & XHCI_RESET_ON_RESUME) > + pm_runtime_get_noresume(dev); > + > /* Make sure the HC is halted. */ > retval = xhci_halt(xhci); > if (retval) > -- > 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/
[PATCH] usb: xhci: Disable runtime PM suspend for quirky controllers.
If a USB controller with XHCI_RESET_ON_RESUME goes to runtime suspend, a reset will be performed upon runtime resume. Any previously suspended devices attached to the controller will be re-enumerated at this time. This will cause problems, for example, if an open system call on the device triggered the resume (the open call will fail). Note that this change is only relevant when persist_enabled is not set for USB devices. Signed-off-by: Shawn Nematbakhsh --- drivers/usb/host/xhci.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index b4aa79d..7455156 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -4687,6 +4687,12 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks) get_quirks(dev, xhci); + /* If we are resetting upon resume, we must disable runtime PM. +* Otherwise, an open() syscall to a device on our runtime suspended +* controller will trigger controller reset and device re-enumeration */ + if (xhci->quirks & XHCI_RESET_ON_RESUME) + pm_runtime_get_noresume(dev); + /* Make sure the HC is halted. */ retval = xhci_halt(xhci); if (retval) -- 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/
[PATCH] usb: xhci: Disable runtime PM suspend for quirky controllers.
If a USB controller with XHCI_RESET_ON_RESUME goes to runtime suspend, a reset will be performed upon runtime resume. Any previously suspended devices attached to the controller will be re-enumerated at this time. This will cause problems, for example, if an open system call on the device triggered the resume (the open call will fail). Note that this change is only relevant when persist_enabled is not set for USB devices. Signed-off-by: Shawn Nematbakhsh sha...@chromium.org --- drivers/usb/host/xhci.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index b4aa79d..7455156 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -4687,6 +4687,12 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks) get_quirks(dev, xhci); + /* If we are resetting upon resume, we must disable runtime PM. +* Otherwise, an open() syscall to a device on our runtime suspended +* controller will trigger controller reset and device re-enumeration */ + if (xhci-quirks XHCI_RESET_ON_RESUME) + pm_runtime_get_noresume(dev); + /* Make sure the HC is halted. */ retval = xhci_halt(xhci); if (retval) -- 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/
Re: [PATCH] usb: xhci: Disable runtime PM suspend for quirky controllers.
On Fri, May 24, 2013 at 11:12:57AM -0700, Shawn Nematbakhsh wrote: If a USB controller with XHCI_RESET_ON_RESUME goes to runtime suspend, a reset will be performed upon runtime resume. Any previously suspended devices attached to the controller will be re-enumerated at this time. This will cause problems, for example, if an open system call on the device triggered the resume (the open call will fail). That's ugly, but disabling runtime PM is going to have a big impact on the power consumption of those systems. Alan, do you think this is really the right thing to be doing here? It feels like userspace should just be able to deal with devices disconnecting on resume. After all, there are lots of USB devices that can't handle USB device suspend at all. Shouldn't userspace just disable runtime PM for the USB device classes that don't have a reset resume callback? Note that this change is only relevant when persist_enabled is not set for USB devices. Could we at least wrap the call in an ifdef CONFIG_USB_DEFAULT_PERSIST? That way if people have USB persist turned off in their configuration, their host will still be able to suspend. Sarah Sharp Signed-off-by: Shawn Nematbakhsh sha...@chromium.org --- drivers/usb/host/xhci.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index b4aa79d..7455156 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -4687,6 +4687,12 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks) get_quirks(dev, xhci); + /* If we are resetting upon resume, we must disable runtime PM. + * Otherwise, an open() syscall to a device on our runtime suspended + * controller will trigger controller reset and device re-enumeration */ + if (xhci-quirks XHCI_RESET_ON_RESUME) + pm_runtime_get_noresume(dev); + /* Make sure the HC is halted. */ retval = xhci_halt(xhci); if (retval) -- 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/
