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