* [PATCH V2] input: Fix USB autosuspend on bcm5974 @ 2011-10-10 13:41 Matthew Garrett 2011-10-10 14:06 ` Oliver Neukum 0 siblings, 1 reply; 6+ messages in thread From: Matthew Garrett @ 2011-10-10 13:41 UTC (permalink / raw) To: linux-input; +Cc: rydberg, dtor, Matthew Garrett The bcm5974 code takes a USB autosuspend reference on device open and releases it on device close. This means that the hardware won't sleep when anything holds it open. This is sensible for input devices that don't support remote wakeups on normal use (like most mice), but this hardware trigger wakeups on touch and so can suspend transparently to the user. Doing so allows the USB host controller to sleep when the machine is idle, giving measurable power savings. Signed-off-by: Matthew Garrett <mjg@redhat.com> --- Got rid of the unbalanced put on the close path, and added usb_mark_last_busy() calls to make sure the device doesn't go to sleep while in use. This works fine on my hardware now, and I don't see any warning messages being generated. drivers/input/mouse/bcm5974.c | 7 +++---- 1 files changed, 3 insertions(+), 4 deletions(-) diff --git a/drivers/input/mouse/bcm5974.c b/drivers/input/mouse/bcm5974.c index 5ec617e..453e9da 100644 --- a/drivers/input/mouse/bcm5974.c +++ b/drivers/input/mouse/bcm5974.c @@ -631,6 +631,7 @@ static void bcm5974_irq_button(struct urb *urb) switch (urb->status) { case 0: + usb_mark_last_busy(dev->udev); break; case -EOVERFLOW: case -ECONNRESET: @@ -660,6 +661,7 @@ static void bcm5974_irq_trackpad(struct urb *urb) switch (urb->status) { case 0: + usb_mark_last_busy(dev->udev); break; case -EOVERFLOW: case -ECONNRESET: @@ -764,8 +766,7 @@ static int bcm5974_open(struct input_dev *input) mutex_unlock(&dev->pm_mutex); - if (error) - usb_autopm_put_interface(dev->intf); + usb_autopm_put_interface(dev->intf); return error; } @@ -780,8 +781,6 @@ static void bcm5974_close(struct input_dev *input) dev->opened = 0; mutex_unlock(&dev->pm_mutex); - - usb_autopm_put_interface(dev->intf); } static int bcm5974_suspend(struct usb_interface *iface, pm_message_t message) -- 1.7.6.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH V2] input: Fix USB autosuspend on bcm5974 2011-10-10 13:41 [PATCH V2] input: Fix USB autosuspend on bcm5974 Matthew Garrett @ 2011-10-10 14:06 ` Oliver Neukum 2011-10-10 14:16 ` Matthew Garrett 0 siblings, 1 reply; 6+ messages in thread From: Oliver Neukum @ 2011-10-10 14:06 UTC (permalink / raw) To: Matthew Garrett; +Cc: linux-input, rydberg, dtor Am Montag, 10. Oktober 2011, 15:41:26 schrieb Matthew Garrett: > The bcm5974 code takes a USB autosuspend reference on device open and > releases it on device close. This means that the hardware won't sleep when > anything holds it open. This is sensible for input devices that don't > support remote wakeups on normal use (like most mice), but this hardware > trigger wakeups on touch and so can suspend transparently to the user. Doing > so allows the USB host controller to sleep when the machine is idle, giving > measurable power savings. > > Signed-off-by: Matthew Garrett <mjg@redhat.com> > --- > > Got rid of the unbalanced put on the close path, and added usb_mark_last_busy() > calls to make sure the device doesn't go to sleep while in use. This works fine > on my hardware now, and I don't see any warning messages being generated. This is odd, because the hardware shouldn't generate remote wakeups unless you request them, like this (usbhid) int usbhid_open(struct hid_device *hid) { struct usbhid_device *usbhid = hid->driver_data; int res; mutex_lock(&hid_open_mut); if (!hid->open++) { res = usb_autopm_get_interface(usbhid->intf); /* the device must be awake to reliably request remote wakeup */ if (res < 0) { hid->open--; mutex_unlock(&hid_open_mut); return -EIO; } usbhid->intf->needs_remote_wakeup = 1; Regards Oliver ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V2] input: Fix USB autosuspend on bcm5974 2011-10-10 14:06 ` Oliver Neukum @ 2011-10-10 14:16 ` Matthew Garrett 2011-10-10 14:34 ` Oliver Neukum 0 siblings, 1 reply; 6+ messages in thread From: Matthew Garrett @ 2011-10-10 14:16 UTC (permalink / raw) To: Oliver Neukum; +Cc: linux-input, rydberg, dtor On Mon, Oct 10, 2011 at 04:06:37PM +0200, Oliver Neukum wrote: > This is odd, because the hardware shouldn't generate remote wakeups unless you > request them, like this (usbhid) I thought needs_remote_wakeup was a hint to the kernel that remote wakeup ability was required for the hardware to autosuspend? In theory I guess it should be set, but in practice all the hardware supported by this driver generates them so it'd be a noop. No objection to adding it in the name of correctness (or if some future version is broken, I guess...), though. -- Matthew Garrett | mjg59@srcf.ucam.org ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V2] input: Fix USB autosuspend on bcm5974 2011-10-10 14:16 ` Matthew Garrett @ 2011-10-10 14:34 ` Oliver Neukum 2011-10-10 14:38 ` Matthew Garrett 0 siblings, 1 reply; 6+ messages in thread From: Oliver Neukum @ 2011-10-10 14:34 UTC (permalink / raw) To: Matthew Garrett; +Cc: linux-input, rydberg, dtor Am Montag, 10. Oktober 2011, 16:16:35 schrieb Matthew Garrett: > On Mon, Oct 10, 2011 at 04:06:37PM +0200, Oliver Neukum wrote: > > > This is odd, because the hardware shouldn't generate remote wakeups unless you > > request them, like this (usbhid) > > I thought needs_remote_wakeup was a hint to the kernel that remote > wakeup ability was required for the hardware to autosuspend? In theory I It fulfills that role, but it is not its sole function. > guess it should be set, but in practice all the hardware supported by > this driver generates them so it'd be a noop. No objection to adding it > in the name of correctness (or if some future version is broken, I > guess...), though. It is used in usb_port_suspend in form of do_remote_wakeup which is computed from it. And we send a real control message. Regards Oliver int usb_port_suspend(struct usb_device *udev, pm_message_t msg) { struct usb_hub *hub = hdev_to_hub(udev->parent); int port1 = udev->portnum; int status; // dev_dbg(hub->intfdev, "suspend port %d\n", port1); /* enable remote wakeup when appropriate; this lets the device * wake up the upstream hub (including maybe the root hub). * * NOTE: OTG devices may issue remote wakeup (or SRP) even when * we don't explicitly enable it here. */ if (udev->do_remote_wakeup) { status = usb_control_msg(udev, usb_sndctrlpipe(udev, 0), USB_REQ_SET_FEATURE, USB_RECIP_DEVICE, USB_DEVICE_REMOTE_WAKEUP, 0, NULL, 0, USB_CTRL_SET_TIMEOUT); if (status) { dev_dbg(&udev->dev, "won't remote wakeup, status %d\n", status); /* bail if autosuspend is requested */ if (msg.event & PM_EVENT_AUTO) return status; } } > -- > Matthew Garrett | mjg59@srcf.ucam.org > -- > To unsubscribe from this list: send the line "unsubscribe linux-input" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- - - - SUSE LINUX Products GmbH, GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer, HRB 16746 (AG Nürnberg) Maxfeldstraße 5 90409 Nürnberg Germany - - - -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V2] input: Fix USB autosuspend on bcm5974 2011-10-10 14:34 ` Oliver Neukum @ 2011-10-10 14:38 ` Matthew Garrett 2011-10-10 14:42 ` Oliver Neukum 0 siblings, 1 reply; 6+ messages in thread From: Matthew Garrett @ 2011-10-10 14:38 UTC (permalink / raw) To: Oliver Neukum; +Cc: linux-input, rydberg, dtor On Mon, Oct 10, 2011 at 04:34:38PM +0200, Oliver Neukum wrote: > Am Montag, 10. Oktober 2011, 16:16:35 schrieb Matthew Garrett: > > On Mon, Oct 10, 2011 at 04:06:37PM +0200, Oliver Neukum wrote: > > > > > This is odd, because the hardware shouldn't generate remote wakeups unless you > > > request them, like this (usbhid) > > > > I thought needs_remote_wakeup was a hint to the kernel that remote > > wakeup ability was required for the hardware to autosuspend? In theory I > > It fulfills that role, but it is not its sole function. > > > guess it should be set, but in practice all the hardware supported by > > this driver generates them so it'd be a noop. No objection to adding it > > in the name of correctness (or if some future version is broken, I > > guess...), though. > > It is used in usb_port_suspend in form of do_remote_wakeup which > is computed from it. And we send a real control message. do_remote_wakeup will be set if device_may_wakeup is true, regardless of whether the driver asks for it. Or am I misreading choose_wakeup()? -- Matthew Garrett | mjg59@srcf.ucam.org ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V2] input: Fix USB autosuspend on bcm5974 2011-10-10 14:38 ` Matthew Garrett @ 2011-10-10 14:42 ` Oliver Neukum 0 siblings, 0 replies; 6+ messages in thread From: Oliver Neukum @ 2011-10-10 14:42 UTC (permalink / raw) To: Matthew Garrett; +Cc: linux-input, rydberg, dtor Am Montag, 10. Oktober 2011, 16:38:02 schrieb Matthew Garrett: > On Mon, Oct 10, 2011 at 04:34:38PM +0200, Oliver Neukum wrote: > > Am Montag, 10. Oktober 2011, 16:16:35 schrieb Matthew Garrett: > > > On Mon, Oct 10, 2011 at 04:06:37PM +0200, Oliver Neukum wrote: > > > > > > > This is odd, because the hardware shouldn't generate remote wakeups unless you > > > > request them, like this (usbhid) > > > > > > I thought needs_remote_wakeup was a hint to the kernel that remote > > > wakeup ability was required for the hardware to autosuspend? In theory I > > > > It fulfills that role, but it is not its sole function. > > > > > guess it should be set, but in practice all the hardware supported by > > > this driver generates them so it'd be a noop. No objection to adding it > > > in the name of correctness (or if some future version is broken, I > > > guess...), though. > > > > It is used in usb_port_suspend in form of do_remote_wakeup which > > is computed from it. And we send a real control message. > > do_remote_wakeup will be set if device_may_wakeup is true, regardless of > whether the driver asks for it. Or am I misreading choose_wakeup()? Indeed things have changed. Arguably this raises power consumption. Anyway, you ought to set it so usbcore will refuse should remote wakeup fail to be enabled. Regards Oliver ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-10-10 14:42 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-10-10 13:41 [PATCH V2] input: Fix USB autosuspend on bcm5974 Matthew Garrett 2011-10-10 14:06 ` Oliver Neukum 2011-10-10 14:16 ` Matthew Garrett 2011-10-10 14:34 ` Oliver Neukum 2011-10-10 14:38 ` Matthew Garrett 2011-10-10 14:42 ` Oliver Neukum
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).