[PATCH V2] input: Fix USB 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.

Signed-off-by: Matthew Garrett <redacted>
---

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

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 <redacted>
---

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

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

Am Montag, 10. Oktober 2011, 16:16:35 schrieb Matthew Garrett:

On Mon, Oct 10, 2011 at 04:06:37PM +0200, Oliver Neukum wrote:

quoted

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

On Mon, Oct 10, 2011 at 04:34:38PM +0200, Oliver Neukum wrote:

Am Montag, 10. Oktober 2011, 16:16:35 schrieb Matthew Garrett:

quoted

On Mon, Oct 10, 2011 at 04:06:37PM +0200, Oliver Neukum wrote:

quoted

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.

quoted

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

Am Montag, 10. Oktober 2011, 16:38:02 schrieb Matthew Garrett:

On Mon, Oct 10, 2011 at 04:34:38PM +0200, Oliver Neukum wrote:

quoted

Am Montag, 10. Oktober 2011, 16:16:35 schrieb Matthew Garrett:

quoted

On Mon, Oct 10, 2011 at 04:06:37PM +0200, Oliver Neukum wrote:

quoted

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.

quoted

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