[PATCH] input: Fix USB autosuspend on bcm5974

[PATCH] input: Fix USB autosuspend on bcm5974

[Matthew Garrett]

The bcm5974 code takes a USB autosuspend reference on device open but
only releases it if there's an error. Repeated opens will bump the count,
which is clearly wrong. It's also unnecessary - the only part of the
driver operation that is incompatible with autosuspend is the initial
setup, and after that the device generates appropriate remote wakeups.
Making the unreference unconditional fixes this.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 drivers/input/mouse/bcm5974.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/input/mouse/bcm5974.c b/drivers/input/mouse/bcm5974.c
index 5ec617e..38d0286 100644
--- a/drivers/input/mouse/bcm5974.c
+++ b/drivers/input/mouse/bcm5974.c
@@ -764,8 +764,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;
 }
-- 
1.7.6.4

Re: [PATCH] input: Fix USB autosuspend on bcm5974

[Dmitry Torokhov]

Hi Matthew,

On Thu, Oct 06, 2011 at 05:36:37PM -0400, Matthew Garrett wrote:
> The bcm5974 code takes a USB autosuspend reference on device open but
> only releases it if there's an error. Repeated opens will bump the count,

No, there won't be repeated opens as input core will not issue another
open unless everyone closes the device first (and then bcm5974_close
will drop the reference that is being held).

> which is clearly wrong. It's also unnecessary - the only part of the
> driver operation that is incompatible with autosuspend is the initial
> setup, and after that the device generates appropriate remote wakeups.
> Making the unreference unconditional fixes this.

... and makes counter unbalanced on close. If we change bcm5974_open()
as you are suggesting we need to adjust bcm5974_close() accordingly.

> 
> Signed-off-by: Matthew Garrett <mjg@redhat.com>
> ---
>  drivers/input/mouse/bcm5974.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/input/mouse/bcm5974.c b/drivers/input/mouse/bcm5974.c
> index 5ec617e..38d0286 100644
> --- a/drivers/input/mouse/bcm5974.c
> +++ b/drivers/input/mouse/bcm5974.c
> @@ -764,8 +764,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;
>  }

Thanks.

-- 
Dmitry

Re: [PATCH] input: Fix USB autosuspend on bcm5974

[Matthew Garrett]

You're completely right. I have absolutely no idea how I missed that - I 
checked and didn't find any other puts. I'll fix and resend.

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

Re: [PATCH] input: Fix USB autosuspend on bcm5974

[Oliver Neukum]

Am Freitag, 7. Oktober 2011, 01:19:54 schrieb Matthew Garrett:
> You're completely right. I have absolutely no idea how I missed that - I 
> checked and didn't find any other puts. I'll fix and resend.

Please also note that for a device to generate wakeup events the kernel has to
enable them which drivers have to request, like the usbhid driver does.
If you do this you better have a test device, because I found that most
generic mice generate wakeup events only if you press a button, not move
the mouse. We are simply not equipped in our API to use that capability.

	Regards
		Oliver

Re: [PATCH] input: Fix USB autosuspend on bcm5974

[Matthew Garrett]

On Fri, Oct 07, 2011 at 08:37:46AM +0200, Oliver Neukum wrote:

> Please also note that for a device to generate wakeup events the kernel has to
> enable them which drivers have to request, like the usbhid driver does.
> If you do this you better have a test device, because I found that most
> generic mice generate wakeup events only if you press a button, not move
> the mouse. We are simply not equipped in our API to use that capability.

This hardware generates wakeups events on touch.

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

Re: [PATCH] input: Fix USB autosuspend on bcm5974

[Dmitry Torokhov]

On Fri, Oct 07, 2011 at 01:36:53PM +0100, Matthew Garrett wrote:
> On Fri, Oct 07, 2011 at 08:37:46AM +0200, Oliver Neukum wrote:
> 
> > Please also note that for a device to generate wakeup events the kernel has to
> > enable them which drivers have to request, like the usbhid driver does.
> > If you do this you better have a test device, because I found that most
> > generic mice generate wakeup events only if you press a button, not move
> > the mouse. We are simply not equipped in our API to use that capability.
> 
> This hardware generates wakeups events on touch.

Is it true for all generations that the driver supports?

-- 
Dmitry

Re: [PATCH] input: Fix USB autosuspend on bcm5974

[Matthew Garrett]

On Fri, Oct 07, 2011 at 09:20:27AM -0700, Dmitry Torokhov wrote:
> On Fri, Oct 07, 2011 at 01:36:53PM +0100, Matthew Garrett wrote:
> > On Fri, Oct 07, 2011 at 08:37:46AM +0200, Oliver Neukum wrote:
> > 
> > > Please also note that for a device to generate wakeup events the kernel has to
> > > enable them which drivers have to request, like the usbhid driver does.
> > > If you do this you better have a test device, because I found that most
> > > generic mice generate wakeup events only if you press a button, not move
> > > the mouse. We are simply not equipped in our API to use that capability.
> > 
> > This hardware generates wakeups events on touch.
> 
> Is it true for all generations that the driver supports?

As far as I can tell.

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

Re: [PATCH] input: Fix USB autosuspend on bcm5974

[Henrik Rydberg]

On Fri, Oct 07, 2011 at 05:27:56PM +0100, Matthew Garrett wrote:
> On Fri, Oct 07, 2011 at 09:20:27AM -0700, Dmitry Torokhov wrote:
> > On Fri, Oct 07, 2011 at 01:36:53PM +0100, Matthew Garrett wrote:
> > > On Fri, Oct 07, 2011 at 08:37:46AM +0200, Oliver Neukum wrote:
> > > 
> > > > Please also note that for a device to generate wakeup events the kernel has to
> > > > enable them which drivers have to request, like the usbhid driver does.
> > > > If you do this you better have a test device, because I found that most
> > > > generic mice generate wakeup events only if you press a button, not move
> > > > the mouse. We are simply not equipped in our API to use that capability.
> > > 
> > > This hardware generates wakeups events on touch.
> > 
> > Is it true for all generations that the driver supports?
> 
> As far as I can tell.

And yet, there is a complication, since the keyboard and mouse
interfaces are both present on the same device. I just ran a small
test, on an MBA3,1. While it is true that the trackpad generates
wakeup events on touch, it enters a loop of suspend/resume, presumably
due to the keyboard. Moreover, the trackpad emits some strange packets
which leave a (large) message trail.

Henrik

Re: [PATCH] input: Fix USB autosuspend on bcm5974

[Matthew Garrett]

On Fri, Oct 07, 2011 at 07:06:12PM +0200, Henrik Rydberg wrote:

> And yet, there is a complication, since the keyboard and mouse
> interfaces are both present on the same device. I just ran a small
> test, on an MBA3,1. While it is true that the trackpad generates
> wakeup events on touch, it enters a loop of suspend/resume, presumably
> due to the keyboard. Moreover, the trackpad emits some strange packets
> which leave a (large) message trail.

I've seen the occasional error message on my 3,1, but I haven't seen any 
looping suspend/resume.

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

Re: [PATCH] input: Fix USB autosuspend on bcm5974

[Henrik Rydberg]

On Fri, Oct 07, 2011 at 06:13:25PM +0100, Matthew Garrett wrote:
> On Fri, Oct 07, 2011 at 07:06:12PM +0200, Henrik Rydberg wrote:
> 
> > And yet, there is a complication, since the keyboard and mouse
> > interfaces are both present on the same device. I just ran a small
> > test, on an MBA3,1. While it is true that the trackpad generates
> > wakeup events on touch, it enters a loop of suspend/resume, presumably
> > due to the keyboard. Moreover, the trackpad emits some strange packets
> > which leave a (large) message trail.
> 
> I've seen the occasional error message on my 3,1, but I haven't seen any 
> looping suspend/resume.

You need to sprinkle some outputs in the actual autosuspend machinery to see it.

Henrik

Re: [PATCH] input: Fix USB autosuspend on bcm5974

[Matthew Garrett]

On Fri, Oct 07, 2011 at 07:42:14PM +0200, Henrik Rydberg wrote:
> On Fri, Oct 07, 2011 at 06:13:25PM +0100, Matthew Garrett wrote:
> > I've seen the occasional error message on my 3,1, but I haven't seen any 
> > looping suspend/resume.
> 
> You need to sprinkle some outputs in the actual autosuspend machinery to see it.

Can you describe exactly what you're seeing?

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

Re: [PATCH] input: Fix USB autosuspend on bcm5974

[Henrik Rydberg]

On Fri, Oct 07, 2011 at 06:39:37PM +0100, Matthew Garrett wrote:
> On Fri, Oct 07, 2011 at 07:42:14PM +0200, Henrik Rydberg wrote:
> > On Fri, Oct 07, 2011 at 06:13:25PM +0100, Matthew Garrett wrote:
> > > I've seen the occasional error message on my 3,1, but I haven't seen any 
> > > looping suspend/resume.
> > 
> > You need to sprinkle some outputs in the actual autosuspend machinery to see it.
> 
> Can you describe exactly what you're seeing?

With a corrected version of your patch, which enables autosuspend
while the device file is open, I see the power layer repeatedly
suspending and resuming the usb device in question, even while
constantly moving a finger around on the pad. In conjunction with this
behavior, I see error messages indicating unknown/faulty packets
appearing in the bcm5974 interrupt handler.

For the next version of the patch, please add a) why you want to
change the current behavior, b) what the patch is supposed to do, and
c) what exact devices are known to work with the patch.

If there are any strange error messages appearing as a result of the
patch, mentioning them would be minimum. Better yet, an explanation of
why they appear, or how an improved patch makes them disappear would
be great.

Thanks,
Henrik

Re: [PATCH] input: Fix USB autosuspend on bcm5974

[Matthew Garrett]

Ah, I see the issue. This is handled by the core hid code in most cases, 
but it looks like the bcm code needs to do it itself. I'll fix up and 
repost next week.

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