[PATCH] usb: Keep track of actual LED state

[PATCH] usb: Keep track of actual LED state

[Matthew Garrett]

The usbhid autosuspend code refuses to suspend if there are any LEDs lit,
in order to avoid situations where this would draw more power than
available when suspended. The current implementation is a counter that's
incremented on LED on and decremented on LED off. However, if a system
suspend takes place, the input core will resynchronise state on resume by
sending LED on and off requests. This may cause the counter to underflow,
resulting in autosuspend being disabled for the rest of system uptime.
This patch simply replaces the counter with a bitmask of the actual LED
state in order to avoid this.

Signed-off-by: Matthew Garrett <mjg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/hid/usbhid/hid-core.c |    6 +++---
 drivers/hid/usbhid/usbhid.h   |    2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index ad978f5..fd96d38 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -598,11 +598,11 @@ static int usb_hidinput_input_event(struct input_dev *dev, unsigned int type, un
 	hid_set_field(field, offset, value);
 	if (value) {
 		spin_lock_irqsave(&usbhid->lock, flags);
-		usbhid->ledcount++;
+		usbhid->ledstate |= (1 << code);
 		spin_unlock_irqrestore(&usbhid->lock, flags);
 	} else {
 		spin_lock_irqsave(&usbhid->lock, flags);
-		usbhid->ledcount--;
+		usbhid->ledstate &= ~(1 << code);
 		spin_unlock_irqrestore(&usbhid->lock, flags);
 	}
 	usbhid_submit_report(hid, field->report, USB_DIR_OUT);
@@ -1339,7 +1339,7 @@ static int hid_suspend(struct usb_interface *intf, pm_message_t message)
 		    && !test_bit(HID_OUT_RUNNING, &usbhid->iofl)
 		    && !test_bit(HID_CTRL_RUNNING, &usbhid->iofl)
 		    && !test_bit(HID_KEYS_PRESSED, &usbhid->iofl)
-		    && (!usbhid->ledcount || ignoreled))
+		    && (!usbhid->ledstate || ignoreled))
 		{
 			set_bit(HID_REPORTED_IDLE, &usbhid->iofl);
 			spin_unlock_irq(&usbhid->lock);
diff --git a/drivers/hid/usbhid/usbhid.h b/drivers/hid/usbhid/usbhid.h
index 1673cac..1d0e2bd 100644
--- a/drivers/hid/usbhid/usbhid.h
+++ b/drivers/hid/usbhid/usbhid.h
@@ -96,7 +96,7 @@ struct usbhid_device {
 	unsigned int retry_delay;                                       /* Delay length in ms */
 	struct work_struct reset_work;                                  /* Task context for resets */
 	wait_queue_head_t wait;						/* For sleeping */
-	int ledcount;							/* counting the number of active leds */
+	int ledstate;							/* Bitmask of active LEDs */
 };
 
 #define	hid_to_usb_dev(hid_dev) \
-- 
1.7.6.4

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

Re: [PATCH] usb: Keep track of actual LED state

[Henrik Rydberg]

Hi Matthew,

> The usbhid autosuspend code refuses to suspend if there are any LEDs lit,
> in order to avoid situations where this would draw more power than
> available when suspended. The current implementation is a counter that's
> incremented on LED on and decremented on LED off. However, if a system
> suspend takes place, the input core will resynchronise state on resume by
> sending LED on and off requests. This may cause the counter to underflow,
> resulting in autosuspend being disabled for the rest of system uptime.
> This patch simply replaces the counter with a bitmask of the actual LED
> state in order to avoid this.
> 
> Signed-off-by: Matthew Garrett <mjg@redhat.com>
> ---
>  drivers/hid/usbhid/hid-core.c |    6 +++---
>  drivers/hid/usbhid/usbhid.h   |    2 +-
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index ad978f5..fd96d38 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -598,11 +598,11 @@ static int usb_hidinput_input_event(struct input_dev *dev, unsigned int type, un
>  	hid_set_field(field, offset, value);
>  	if (value) {
>  		spin_lock_irqsave(&usbhid->lock, flags);
> -		usbhid->ledcount++;
> +		usbhid->ledstate |= (1 << code);

To be consistent, ledstate should be a bitmask the same size as in
input_dev, but it aint pretty...

>  		spin_unlock_irqrestore(&usbhid->lock, flags);
>  	} else {
>  		spin_lock_irqsave(&usbhid->lock, flags);
> -		usbhid->ledcount--;
> +		usbhid->ledstate &= ~(1 << code);

Could simply check for ledcount > 0 here instead, but it is even uglier.

>  		spin_unlock_irqrestore(&usbhid->lock, flags);
>  	}
>  	usbhid_submit_report(hid, field->report, USB_DIR_OUT);
> @@ -1339,7 +1339,7 @@ static int hid_suspend(struct usb_interface *intf, pm_message_t message)
>  		    && !test_bit(HID_OUT_RUNNING, &usbhid->iofl)
>  		    && !test_bit(HID_CTRL_RUNNING, &usbhid->iofl)
>  		    && !test_bit(HID_KEYS_PRESSED, &usbhid->iofl)
> -		    && (!usbhid->ledcount || ignoreled))
> +		    && (!usbhid->ledstate || ignoreled))

Why not test the actual input_dev state here instead? Adding a set/get
led state function to input core could resolve this cleanly.

>  		{
>  			set_bit(HID_REPORTED_IDLE, &usbhid->iofl);
>  			spin_unlock_irq(&usbhid->lock);
> diff --git a/drivers/hid/usbhid/usbhid.h b/drivers/hid/usbhid/usbhid.h
> index 1673cac..1d0e2bd 100644
> --- a/drivers/hid/usbhid/usbhid.h
> +++ b/drivers/hid/usbhid/usbhid.h
> @@ -96,7 +96,7 @@ struct usbhid_device {
>  	unsigned int retry_delay;                                       /* Delay length in ms */
>  	struct work_struct reset_work;                                  /* Task context for resets */
>  	wait_queue_head_t wait;						/* For sleeping */
> -	int ledcount;							/* counting the number of active leds */
> +	int ledstate;							/* Bitmask of active LEDs */
>  };
>  
>  #define	hid_to_usb_dev(hid_dev) \
> -- 
> 1.7.6.4

Cheers,
Henrik

Re: [PATCH] usb: Keep track of actual LED state

[Matthew Garrett]

On Mon, Oct 10, 2011 at 07:13:14PM +0200, Henrik Rydberg wrote:

> > -		usbhid->ledcount--;
> > +		usbhid->ledstate &= ~(1 << code);
> 
> Could simply check for ledcount > 0 here instead, but it is even uglier.

That'd still fail if we had one led turned on and two turned off - 
depending on the order we'd potentially think that suspend was safe.

> Why not test the actual input_dev state here instead? Adding a set/get
> led state function to input core could resolve this cleanly.

That might well be a better approach.

-- 
Matthew Garrett | mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org
--
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