Re: [PATCH] input: Fix autosuspend on bcm5974

Re: [PATCH] input: Fix autosuspend on bcm5974

[Oliver Neukum]

Am Dienstag, 13. März 2012, 00:51:18 schrieb Matthew Garrett:
> @@ -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;

Hi,

optimally you'd drop the requirement for remote wakeup before
you do the put operation. You might save an unneeded wakeup.

	Regards
		Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] input: Fix autosuspend on bcm5974

[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. 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 <mjg@redhat.com>
Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
---
 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

Re: [PATCH] input: Fix autosuspend on bcm5974

[Henrik Rydberg]

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.

> 
> [rydberg@euromail.se: wakeup before mode switch]
> Signed-off-by: Matthew Garrett <mjg@redhat.com>
> Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
> ---
>  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
> 

Thanks,
Henrik

Re: [PATCH] input: Fix autosuspend on bcm5974

[Matthew Garrett]

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

Re: [PATCH] input: Fix autosuspend on bcm5974

[Oliver Neukum]

Am Dienstag, 13. März 2012, 15:49:41 schrieb Matthew Garrett:
> > 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

Re: [PATCH] input: Fix autosuspend on bcm5974

[Henrik Rydberg]

> 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

Re: [PATCH] input: Fix autosuspend on bcm5974

[Matthew Garrett]

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

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