lkml.org 
[lkml]   [2013]   [Aug]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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 <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
> >
> >


\
 
 \ /
  Last update: 2013-08-09 19:41    [W:0.109 / U:0.580 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site