* [PATCH] usb: xhci: Disable runtime PM suspend for quirky controllers. @ 2013-05-24 18:12 Shawn Nematbakhsh 2013-05-24 21:05 ` Sarah Sharp 0 siblings, 1 reply; 13+ messages in thread From: Shawn Nematbakhsh @ 2013-05-24 18:12 UTC (permalink / raw) To: linux-usb Cc: Sarah Sharp, Greg Kroah-Hartman, linux-kernel, Julius Werner, Shawn Nematbakhsh 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 <shawnn@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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] usb: xhci: Disable runtime PM suspend for quirky controllers. 2013-05-24 18:12 [PATCH] usb: xhci: Disable runtime PM suspend for quirky controllers Shawn Nematbakhsh @ 2013-05-24 21:05 ` Sarah Sharp 2013-05-25 14:11 ` Alan Stern 0 siblings, 1 reply; 13+ messages in thread From: Sarah Sharp @ 2013-05-24 21:05 UTC (permalink / raw) To: Shawn Nematbakhsh, Alan Stern Cc: linux-usb, Greg Kroah-Hartman, linux-kernel, Julius Werner 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 <shawnn@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 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] usb: xhci: Disable runtime PM suspend for quirky controllers. 2013-05-24 21:05 ` Sarah Sharp @ 2013-05-25 14:11 ` Alan Stern 2013-05-25 16:59 ` Shawn Nematbakhsh [not found] ` <CALaWCOOGEDtF1z29df2ST9kV-VpMa9VbvFK0Hh71WJG5f_pngA@mail.gmail.com> 0 siblings, 2 replies; 13+ messages in thread From: Alan Stern @ 2013-05-25 14:11 UTC (permalink / raw) To: Sarah Sharp Cc: Shawn Nematbakhsh, linux-usb, Greg Kroah-Hartman, linux-kernel, Julius Werner 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] usb: xhci: Disable runtime PM suspend for quirky controllers. 2013-05-25 14:11 ` Alan Stern @ 2013-05-25 16:59 ` Shawn Nematbakhsh [not found] ` <CALaWCOOGEDtF1z29df2ST9kV-VpMa9VbvFK0Hh71WJG5f_pngA@mail.gmail.com> 1 sibling, 0 replies; 13+ messages in thread From: Shawn Nematbakhsh @ 2013-05-25 16:59 UTC (permalink / raw) To: Alan Stern, Sarah Sharp Cc: linux-usb, Greg Kroah-Hartman, linux-kernel, Julius Werner 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 <stern@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 > ^ permalink raw reply [flat|nested] 13+ messages in thread
[parent not found: <CALaWCOOGEDtF1z29df2ST9kV-VpMa9VbvFK0Hh71WJG5f_pngA@mail.gmail.com>]
* Re: [PATCH] usb: xhci: Disable runtime PM suspend for quirky controllers. [not found] ` <CALaWCOOGEDtF1z29df2ST9kV-VpMa9VbvFK0Hh71WJG5f_pngA@mail.gmail.com> @ 2013-05-28 20:58 ` Sarah Sharp 2013-05-28 21:41 ` Julius Werner 2013-08-09 17:22 ` Sarah Sharp 1 sibling, 1 reply; 13+ messages in thread From: Sarah Sharp @ 2013-05-28 20:58 UTC (permalink / raw) To: Shawn Nematbakhsh Cc: Alan Stern, linux-usb, Greg Kroah-Hartman, linux-kernel, Julius Werner 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] usb: xhci: Disable runtime PM suspend for quirky controllers. 2013-05-28 20:58 ` Sarah Sharp @ 2013-05-28 21:41 ` Julius Werner 2013-05-29 14:23 ` Alan Stern 2013-05-29 19:38 ` Sarah Sharp 0 siblings, 2 replies; 13+ messages in thread From: Julius Werner @ 2013-05-28 21:41 UTC (permalink / raw) To: Sarah Sharp Cc: Shawn Nematbakhsh, Alan Stern, linux-usb, Greg Kroah-Hartman, LKML, Julius Werner 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). ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] usb: xhci: Disable runtime PM suspend for quirky controllers. 2013-05-28 21:41 ` Julius Werner @ 2013-05-29 14:23 ` Alan Stern 2013-05-29 19:38 ` Sarah Sharp 1 sibling, 0 replies; 13+ messages in thread From: Alan Stern @ 2013-05-29 14:23 UTC (permalink / raw) To: Julius Werner Cc: Sarah Sharp, Shawn Nematbakhsh, linux-usb, Greg Kroah-Hartman, LKML 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] usb: xhci: Disable runtime PM suspend for quirky controllers. 2013-05-28 21:41 ` Julius Werner 2013-05-29 14:23 ` Alan Stern @ 2013-05-29 19:38 ` Sarah Sharp 2013-05-29 20:11 ` Alan Stern 2013-05-29 20:32 ` Don Zickus 1 sibling, 2 replies; 13+ messages in thread From: Sarah Sharp @ 2013-05-29 19:38 UTC (permalink / raw) To: Julius Werner Cc: Shawn Nematbakhsh, Alan Stern, linux-usb, Greg Kroah-Hartman, LKML, Don Zickus, Oliver Neukum 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&m=136941922715772&w=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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] usb: xhci: Disable runtime PM suspend for quirky controllers. 2013-05-29 19:38 ` Sarah Sharp @ 2013-05-29 20:11 ` Alan Stern 2013-05-29 20:32 ` Don Zickus 1 sibling, 0 replies; 13+ messages in thread From: Alan Stern @ 2013-05-29 20:11 UTC (permalink / raw) To: Sarah Sharp Cc: Julius Werner, Shawn Nematbakhsh, linux-usb, Greg Kroah-Hartman, LKML, Don Zickus, Oliver Neukum 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&m=136941922715772&w=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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] usb: xhci: Disable runtime PM suspend for quirky controllers. 2013-05-29 19:38 ` Sarah Sharp 2013-05-29 20:11 ` Alan Stern @ 2013-05-29 20:32 ` Don Zickus 2013-06-03 11:33 ` Oliver Neukum 1 sibling, 1 reply; 13+ messages in thread From: Don Zickus @ 2013-05-29 20:32 UTC (permalink / raw) To: Sarah Sharp Cc: Julius Werner, Shawn Nematbakhsh, Alan Stern, linux-usb, Greg Kroah-Hartman, LKML, Oliver Neukum 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&m=136941922715772&w=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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] usb: xhci: Disable runtime PM suspend for quirky controllers. 2013-05-29 20:32 ` Don Zickus @ 2013-06-03 11:33 ` Oliver Neukum 0 siblings, 0 replies; 13+ messages in thread From: Oliver Neukum @ 2013-06-03 11:33 UTC (permalink / raw) To: Don Zickus Cc: Sarah Sharp, Julius Werner, Shawn Nematbakhsh, Alan Stern, linux-usb, Greg Kroah-Hartman, LKML 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&m=136941922715772&w=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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] usb: xhci: Disable runtime PM suspend for quirky controllers. [not found] ` <CALaWCOOGEDtF1z29df2ST9kV-VpMa9VbvFK0Hh71WJG5f_pngA@mail.gmail.com> 2013-05-28 20:58 ` Sarah Sharp @ 2013-08-09 17:22 ` Sarah Sharp 2013-08-12 15:49 ` Shawn Nematbakhsh 1 sibling, 1 reply; 13+ messages in thread From: Sarah Sharp @ 2013-08-09 17:22 UTC (permalink / raw) To: Shawn Nematbakhsh Cc: Alan Stern, linux-usb, Greg Kroah-Hartman, linux-kernel, Julius Werner 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 <stern@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 > > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] usb: xhci: Disable runtime PM suspend for quirky controllers. 2013-08-09 17:22 ` Sarah Sharp @ 2013-08-12 15:49 ` Shawn Nematbakhsh 0 siblings, 0 replies; 13+ messages in thread From: Shawn Nematbakhsh @ 2013-08-12 15:49 UTC (permalink / raw) To: Sarah Sharp Cc: Alan Stern, linux-usb, Greg Kroah-Hartman, linux-kernel, Julius Werner Hi Sarah, I will resubmit the patch with these changes shortly. On Fri, Aug 9, 2013 at 10:22 AM, Sarah Sharp <sarah.a.sharp@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 <stern@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 >> > >> > ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2013-08-12 15:49 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-05-24 18:12 [PATCH] usb: xhci: Disable runtime PM suspend for quirky controllers Shawn Nematbakhsh 2013-05-24 21:05 ` Sarah Sharp 2013-05-25 14:11 ` Alan Stern 2013-05-25 16:59 ` Shawn Nematbakhsh [not found] ` <CALaWCOOGEDtF1z29df2ST9kV-VpMa9VbvFK0Hh71WJG5f_pngA@mail.gmail.com> 2013-05-28 20:58 ` Sarah Sharp 2013-05-28 21:41 ` Julius Werner 2013-05-29 14:23 ` Alan Stern 2013-05-29 19:38 ` Sarah Sharp 2013-05-29 20:11 ` Alan Stern 2013-05-29 20:32 ` Don Zickus 2013-06-03 11:33 ` Oliver Neukum 2013-08-09 17:22 ` Sarah Sharp 2013-08-12 15:49 ` Shawn Nematbakhsh
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.