[PATCH] usb: xhci: Disable runtime PM suspend for quirky controllers.

[PATCH] usb: xhci: Disable runtime PM suspend for quirky controllers.

[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

Re: [PATCH] usb: xhci: Disable runtime PM suspend for quirky controllers.

[Sarah Sharp]

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
> 

Re: [PATCH] usb: xhci: Disable runtime PM suspend for quirky controllers.

[Alan Stern]

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

Re: [PATCH] usb: xhci: Disable runtime PM suspend for quirky controllers.

[Shawn Nematbakhsh]

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
>

Re: [PATCH] usb: xhci: Disable runtime PM suspend for quirky controllers.

[Sarah Sharp]

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

Re: [PATCH] usb: xhci: Disable runtime PM suspend for quirky controllers.

[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).

Re: [PATCH] usb: xhci: Disable runtime PM suspend for quirky controllers.

[Alan Stern]

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

Re: [PATCH] usb: xhci: Disable runtime PM suspend for quirky controllers.

[Sarah Sharp]

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

Re: [PATCH] usb: xhci: Disable runtime PM suspend for quirky controllers.

[Alan Stern]

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

Re: [PATCH] usb: xhci: Disable runtime PM suspend for quirky controllers.

[Don Zickus]

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

Re: [PATCH] usb: xhci: Disable runtime PM suspend for quirky controllers.

[Oliver Neukum]

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

Re: [PATCH] usb: xhci: Disable runtime PM suspend for quirky controllers.

[Sarah Sharp]

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
> >
> >

Re: [PATCH] usb: xhci: Disable runtime PM suspend for quirky controllers.

[Shawn Nematbakhsh]

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
>> >
>> >