[PATCH] input: Fix autosuspend on bcm5974

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. The downside is a small
amount of additional latency when the device wakes up, and as a result
this functionality isn't enabled by default.

[rydberg@euromail.se: wakeup before mode switch]
Signed-off-by: Matthew Garrett <redacted>
Signed-off-by: Henrik Rydberg <redacted>
---
 drivers/input/mouse/bcm5974.c |   30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/drivers/input/mouse/bcm5974.c b/drivers/input/mouse/bcm5974.c
index 927e479..aaf158c 100644
--- a/drivers/input/mouse/bcm5974.c
+++ b/drivers/input/mouse/bcm5974.c

@@ -634,6 +634,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:

@@ -663,6 +664,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:

@@ -735,10 +737,15 @@ err_out:
 	return error;
 }
 
-static void bcm5974_pause_traffic(struct bcm5974 *dev)
+static void bcm5974_kill_urbs(struct bcm5974 *dev)
 {
 	usb_kill_urb(dev->tp_urb);
 	usb_kill_urb(dev->bt_urb);
+}
+
+static void bcm5974_pause_traffic(struct bcm5974 *dev)
+{
+	bcm5974_kill_urbs(dev);
 	bcm5974_wellspring_mode(dev, false);
 }
 

@@ -759,6 +766,9 @@ static int bcm5974_open(struct input_dev *input)
 	if (error)
 		return error;
 
+	/* Required for autosuspend */
+	dev->intf->needs_remote_wakeup = 1;
+
 	mutex_lock(&dev->pm_mutex);
 
 	error = bcm5974_start_traffic(dev);

@@ -767,8 +777,10 @@ static int bcm5974_open(struct input_dev *input)
 
 	mutex_unlock(&dev->pm_mutex);
 
+	usb_autopm_put_interface(dev->intf);
+
 	if (error)
-		usb_autopm_put_interface(dev->intf);
+		dev->intf->needs_remote_wakeup = 0;
 
 	return error;
 }

@@ -776,15 +788,25 @@ static int bcm5974_open(struct input_dev *input)
 static void bcm5974_close(struct input_dev *input)
 {
 	struct bcm5974 *dev = input_get_drvdata(input);
+	int error;
+
+	dev->intf->needs_remote_wakeup = 0;
+
+	error = usb_autopm_get_interface(dev->intf);
 
 	mutex_lock(&dev->pm_mutex);
 
-	bcm5974_pause_traffic(dev);
+	bcm5974_kill_urbs(dev);
 	dev->opened = 0;
+	if (error)
+		err("bcm5974: wakeup failed during close: %d\n", error);
+	else
+		bcm5974_wellspring_mode(dev, false);
 
 	mutex_unlock(&dev->pm_mutex);
 
-	usb_autopm_put_interface(dev->intf);
+	if (!error)
+		usb_autopm_put_interface(dev->intf);
 }
 
 static int bcm5974_suspend(struct usb_interface *iface, pm_message_t message)

-- 
1.7.9.3

Hi Matthew,

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. The downside is a small
amount of additional latency when the device wakes up, and as a result
this functionality isn't enabled by default.

With the previous version of this patch, the machine would not wake up
on a normal tap. Is that what you refer to with the "small amount of
additional latency"? By not enabled by default, you mean in the usb
subsystem?

Last time I checked, it seemed the first keystroke would sometimes
disappear at boot. As far as I can see, this version modifies the
needs_remote_wakeup variable to only be true while open, which might
remove that particular problem.

Still, I am concerned with the user experience. On this machine
(MBA31), turning on autopm with this patch included feels like a
regression.

Thanks,
Henrik

On Tue, Mar 13, 2012 at 03:39:58PM +0100, Henrik Rydberg wrote:

With the previous version of this patch, the machine would not wake up
on a normal tap. Is that what you refer to with the "small amount of
additional latency"? By not enabled by default, you mean in the usb
subsystem?

What do you mean by "normal tap"? By "not enabled by default" I mean the 
kernel doesn't enable autosuspend on input devices. Userspace would have 
to do that.

Last time I checked, it seemed the first keystroke would sometimes
disappear at boot. As far as I can see, this version modifies the
needs_remote_wakeup variable to only be true while open, which might
remove that particular problem.

I haven't seen any lost keystrokes.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Am Dienstag, 13. März 2012, 15:49:41 schrieb Matthew Garrett:

quoted

Last time I checked, it seemed the first keystroke would sometimes
disappear at boot. As far as I can see, this version modifies the
needs_remote_wakeup variable to only be true while open, which might
remove that particular problem.

I haven't seen any lost keystrokes.

Hi,

I have seen keyboards that lost keystrokes on UHCI but not on OHCI
and other strange things. Unfortunately such patches are very hard
to test.

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

What do you mean by "normal tap"? By "not enabled by default" I mean the 
kernel doesn't enable autosuspend on input devices. Userspace would have 
to do that.

A normal tap is a short distinct touch, normally in the 100
millisecond range or so. I am not sure what is unclear about it. :-)

Have you not experienced this problem? Simply wait for some seconds
for the device to suspend, then perform a tap while monitoring the
device output.

Henrik

Ah, I see what you mean now. I'll check whether the hardware reports 
state at resume.

-- 
Matthew Garrett | mjg59@srcf.ucam.org