usb hid: reset NumLock

usb hid: reset NumLock

[Pete Zaitcev]

This is a patch for comments only, please do not apply (at least not as-is).
I haven't got the test results yet.

Dell people (Stuart and Charles) complained that on some USB keyboards,
if BIOS enables NumLock, it stays on even after Linux has started. Since
we always start with NumLock off, this confuses users. Quick double dab
at NumLock fixes it, but it's not nice.

The keyboard driver tries to reset LEDs at start, but this never works.
Since the bitmap dev->led is zero, the input_event() filters out any
attempts to switch LEDs off. So, the code in kbd_start() does nothing.

The Dell's solution looks like this (for 2.6.9 code base):

linux-2.6.9-42.0.3/drivers/char/keyboard.c:
@@ -931,6 +931,19 @@ void kbd_refresh_leds(struct input_handl
 
 	tasklet_disable(&keyboard_tasklet);
 	if (leds != 0xff) {
+		/*
+		 * We can't be sure of the state of the LEDs on keyboards
+		 * (e.g., BIOS might have left LED_NUML on), so set dev->led
+		 * opposite of ledstate, to make sure input_event() actually
+		 * sends the command to the keyboard to set each LED.
+		 */
+		if (test_bit(LED_SCROLLL, handle->dev->led) == !!(leds & 0x01))
+			change_bit(LED_SCROLLL, handle->dev->led);
+		if (test_bit(LED_NUML, handle->dev->led) == !!(leds & 0x02))
+			change_bit(LED_NUML, handle->dev->led);
+		if (test_bit(LED_CAPSL, handle->dev->led) == !!(leds & 0x04))
+			change_bit(LED_CAPSL, handle->dev->led);
+
 		input_event(handle->dev, EV_LED, LED_SCROLLL, !!(leds & 0x01));
 		input_event(handle->dev, EV_LED, LED_NUML,    !!(leds & 0x02));
 		input_event(handle->dev, EV_LED, LED_CAPSL,   !!(leds & 0x04));

I didn't like a) layering violation, and b) that they defeat filtering
unconditionally. Why have any filtering then?

Instead, I propose for USB HID driver to reset NumLock on probe. Like this:

--- a/drivers/usb/input/hid-core.c
+++ b/drivers/usb/input/hid-core.c
@@ -458,6 +458,18 @@ static int usb_hidinput_input_event(struct input_dev *dev, unsigned int type, un
 	return 0;
 }
 
+static void usbhid_set_leds(struct hid_device *hid, unsigned int code, int val)
+{
+	struct hid_field *field;
+	int offset;
+
+	/* This is often called for the mouse half. */
+	if ((offset = hidinput_find_field(hid, EV_LED, code, &field)) != -1) {
+		hid_set_field(field, offset, val);
+		usbhid_submit_report(hid, field->report, USB_DIR_OUT);
+	}
+}
+
 int usbhid_wait_io(struct hid_device *hid)
 {
 	struct usbhid_device *usbhid = hid->driver_data;
@@ -1328,9 +1340,18 @@ static int hid_probe(struct usb_interface *intf, const struct usb_device_id *id)
 		return -ENODEV;
 	}
 
-	if ((hid->claimed & HID_CLAIMED_INPUT))
+	if ((hid->claimed & HID_CLAIMED_INPUT)) {
 		hid_ff_init(hid);
 
+		/*
+		 * We do this only if input has claimed the device because
+		 * we can only find fields after they were configured in
+		 * hidinput_connect.
+		 */
+		/* if (hid->quirks & HID_QUIRK_RESET_LEDS) */
+		usbhid_set_leds(hid, LED_NUML, 0);
+	}
+
 	if (hid->quirks & HID_QUIRK_SONY_PS3_CONTROLLER)
 		hid_fixup_sony_ps3_controller(interface_to_usbdev(intf),
 			intf->cur_altsetting->desc.bInterfaceNumber);
diff --git a/include/linux/hid.h b/include/linux/hid.h
index d26b08f..f592f01 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -267,6 +267,7 @@ struct hid_item {
 #define HID_QUIRK_SKIP_OUTPUT_REPORTS		0x00020000
 #define HID_QUIRK_IGNORE_MOUSE			0x00040000
 #define HID_QUIRK_SONY_PS3_CONTROLLER		0x00080000
+#define HID_QUIRK_RESET_LEDS			0x00100000
 
 /*
  * This is the global environment of the parser. This information is

URL with details, discussion, rejected patch to read BIOS byte at 0x417:
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=228674

Jiri, Dmitry, any opinions please?

-- Pete

Re: usb hid: reset NumLock

[Dmitry Torokhov]

Hi Pete,

On 3/30/07, Pete Zaitcev <zaitcev@redhat.com> wrote:
>
> I didn't like a) layering violation, and b) that they defeat filtering
> unconditionally. Why have any filtering then?
>
> Instead, I propose for USB HID driver to reset NumLock on probe. Like this:
>
> --- a/drivers/usb/input/hid-core.c
> +++ b/drivers/usb/input/hid-core.c
> @@ -458,6 +458,18 @@ static int usb_hidinput_input_event(struct input_dev *dev, unsigned int type, un
>        return 0;
>  }
>
> +static void usbhid_set_leds(struct hid_device *hid, unsigned int code, int val)
> +{
> +       struct hid_field *field;
> +       int offset;
> +
> +       /* This is often called for the mouse half. */
> +       if ((offset = hidinput_find_field(hid, EV_LED, code, &field)) != -1) {
> +               hid_set_field(field, offset, val);
> +               usbhid_submit_report(hid, field->report, USB_DIR_OUT);
> +       }
> +}
> +

This is fine and that's what we do in atkbd probe but maybe we should
move that in input core and reset leds as part of
input_register_device()?

-- 
Dmitry

Re: usb hid: reset NumLock

[Pete Zaitcev]

On Fri, 30 Mar 2007 14:14:20 -0400, "Dmitry Torokhov" <dmitry.torokhov@gmail.com> wrote:

> > I didn't like a) layering violation, and b) that they defeat filtering
> > unconditionally. Why have any filtering then?
> >
> > Instead, I propose for USB HID driver to reset NumLock on probe. Like this:
> >
> > --- a/drivers/usb/input/hid-core.c

> This is fine and that's what we do in atkbd probe but maybe we should
> move that in input core and reset leds as part of
> input_register_device()?

Sure, as long as it works. I think (as much as I understand), that we
already attempt to do this indirectly. input_register_device invokes
->start handlers, and the kbd_start attempts to reset LEDs, but fails
because of the state filtering.

-- Pete

Re: usb hid: reset NumLock

[Dmitry Torokhov]

On 3/30/07, Pete Zaitcev <zaitcev@redhat.com> wrote:
> On Fri, 30 Mar 2007 14:14:20 -0400, "Dmitry Torokhov" <dmitry.torokhov@gmail.com> wrote:
>
> > > I didn't like a) layering violation, and b) that they defeat filtering
> > > unconditionally. Why have any filtering then?
> > >
> > > Instead, I propose for USB HID driver to reset NumLock on probe. Like this:
> > >
> > > --- a/drivers/usb/input/hid-core.c
>
> > This is fine and that's what we do in atkbd probe but maybe we should
> > move that in input core and reset leds as part of
> > input_register_device()?
>
> Sure, as long as it works. I think (as much as I understand), that we
> already attempt to do this indirectly. input_register_device invokes
> ->start handlers, and the kbd_start attempts to reset LEDs, but fails
> because of the state filtering.
>

Not exactly... Keyboard handler's start method tries to bring state of
new keyboard in sync with the state of the rest of keyboards. The
purpose of start() is not to complete hardware initialization but to
adjust logical state of the device according to handler's
requirements...

But I am backpedaling on my statement about moving it into input core.
The driver (HID in this case) should properly reset the device before
registering it with input layer.

-- 
Dmitry

Re: usb hid: reset NumLock

[Jiri Kosina]

On Fri, 30 Mar 2007, Pete Zaitcev wrote:

> @@ -1328,9 +1340,18 @@ static int hid_probe(struct usb_interface *intf, const struct usb_device_id *id)
>  		return -ENODEV;
>  	}
>  
> -	if ((hid->claimed & HID_CLAIMED_INPUT))
> +	if ((hid->claimed & HID_CLAIMED_INPUT)) {
>  		hid_ff_init(hid);
>  
> +		/*
> +		 * We do this only if input has claimed the device because
> +		 * we can only find fields after they were configured in
> +		 * hidinput_connect.
> +		 */
> +		/* if (hid->quirks & HID_QUIRK_RESET_LEDS) */
> +		usbhid_set_leds(hid, LED_NUML, 0);
> +	}
> +
>  	if (hid->quirks & HID_QUIRK_SONY_PS3_CONTROLLER)
>  		hid_fixup_sony_ps3_controller(interface_to_usbdev(intf),
>  			intf->cur_altsetting->desc.bInterfaceNumber);

Hi Pete,

I think I see an issue here. Imagine that you boot a system initially with 
one keyboard connected (usb, ps/2, doesn't matter), and after some time 
you connect second USB keyboard (the NumLock is 'on' on the first keyboard 
when you connect the second one).

Without your patch, the NumLock led will be OK on the second keyboard 
immediately. With your patch, the NumLock will be forced to 'off' and it 
will be out of sync with the first keyboard. The leds will get in sync 
later when any change occurs. 

> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index d26b08f..f592f01 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -267,6 +267,7 @@ struct hid_item {
>  #define HID_QUIRK_SKIP_OUTPUT_REPORTS		0x00020000
>  #define HID_QUIRK_IGNORE_MOUSE			0x00040000
>  #define HID_QUIRK_SONY_PS3_CONTROLLER		0x00080000
> +#define HID_QUIRK_RESET_LEDS			0x00100000

I think this is not worth a quirk - when we get it working properly, why 
not do it unconditionally for all keyboards?

> URL with details, discussion, rejected patch to read BIOS byte at 0x417:
> https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=228674

"You are not authorized to access bug #228674. To see this bug, you must 
first log in to an account with the appropriate permissions." 

:)

Thanks,

-- 
Jiri Kosina

Re: usb hid: reset NumLock

[Pete Zaitcev]

On Sat, 31 Mar 2007 21:35:19 +0200 (CEST), Jiri Kosina <jkosina@suse.cz> wrote:
> On Fri, 30 Mar 2007, Pete Zaitcev wrote:

> I think I see an issue here. Imagine that you boot a system initially with 
> one keyboard connected (usb, ps/2, doesn't matter), and after some time 
> you connect second USB keyboard (the NumLock is 'on' on the first keyboard 
> when you connect the second one).
> 
> Without your patch, the NumLock led will be OK on the second keyboard 
> immediately. With your patch, the NumLock will be forced to 'off' and it 
> will be out of sync with the first keyboard. The leds will get in sync 
> later when any change occurs. 

Oh, right, I missed it. The difficulty is how I rely on usages and fields
being already mapped by hidinput_configure_usage(). Thanks for letting me
know, I'll think about it.

-- Pete

Re: usb hid: reset NumLock

[Pekka Enberg]

On 3/30/07, Pete Zaitcev <zaitcev@redhat.com> wrote:
> Dell people (Stuart and Charles) complained that on some USB keyboards,
> if BIOS enables NumLock, it stays on even after Linux has started. Since
> we always start with NumLock off, this confuses users. Quick double dab
> at NumLock fixes it, but it's not nice.

What I am seeing on my Thinkpad is that when I boot _without_ an USB
keyboard NumLock is enabled. Switching to virtual console and back to
X fixes it which is why I have never bothered to debug it further.
Perhaps this is related? Should I give your patch a spin to see if it
fixes the problem?

Re: usb hid: reset NumLock

[Pete Zaitcev]

On Sun, 1 Apr 2007 14:49:59 +0300, "Pekka Enberg" <penberg@cs.helsinki.fi> wrote:
> On 3/30/07, Pete Zaitcev <zaitcev@redhat.com> wrote:

> > Dell people (Stuart and Charles) complained that on some USB keyboards,
> > if BIOS enables NumLock, it stays on even after Linux has started. Since
> > we always start with NumLock off, this confuses users. Quick double dab
> > at NumLock fixes it, but it's not nice.
> 
> What I am seeing on my Thinkpad is that when I boot _without_ an USB
> keyboard NumLock is enabled. Switching to virtual console and back to
> X fixes it which is why I have never bothered to debug it further.
> Perhaps this is related? Should I give your patch a spin to see if it
> fixes the problem?

It's related, but not the same problem. Perhaps the initialization in atkbd
which Dmitry mentioned is not complete. Jiri found a fatal bug in my patch,
but even assuming that it worked, it still would do nothing to you.

-- Pete

Re: usb hid: reset NumLock

[Dmitry Torokhov]

Hi Pekka,

On Sunday 01 April 2007 07:49, Pekka Enberg wrote:
> On 3/30/07, Pete Zaitcev <zaitcev@redhat.com> wrote:
> > Dell people (Stuart and Charles) complained that on some USB keyboards,
> > if BIOS enables NumLock, it stays on even after Linux has started. Since
> > we always start with NumLock off, this confuses users. Quick double dab
> > at NumLock fixes it, but it's not nice.
> 
> What I am seeing on my Thinkpad is that when I boot _without_ an USB
> keyboard NumLock is enabled. Switching to virtual console and back to
> X fixes it which is why I have never bothered to debug it further.
> Perhaps this is related? Should I give your patch a spin to see if it
> fixes the problem?
> 

Are you saying that NumLock LED is lit or that keyboard is in NumLock
state? Does the same happen if you boot with init=/bin/bash?

-- 
Dmitry

Re: usb hid: reset NumLock

[Pete Zaitcev]

On Sat, 31 Mar 2007 21:35:19 +0200 (CEST), Jiri Kosina <jkosina@suse.cz> wrote:

> I think I see an issue here. Imagine that you boot a system initially with 
> one keyboard connected (usb, ps/2, doesn't matter), and after some time 
> you connect second USB keyboard (the NumLock is 'on' on the first keyboard 
> when you connect the second one).

OK. I toyed with a return to the Stuart's idea: defeat filtering
somehow, but it wasn't working well, too much duplication. So, I created
a clone of hidinput_find_field() instead. It's still an annoying
duplication, but a lesser one, I think.

diff --git a/drivers/usb/input/hid-core.c b/drivers/usb/input/hid-core.c
index ef09952..7338e81 100644
--- a/drivers/usb/input/hid-core.c
+++ b/drivers/usb/input/hid-core.c
@@ -548,6 +548,28 @@ void usbhid_init_reports(struct hid_device *hid)
 		warn("timeout initializing reports");
 }
 
+/*
+ * Reset LEDs which BIOS might have left on.
+ */
+static int hid_find_field_early(struct hid_device *hid, unsigned int page,
+    unsigned int hid_code, struct hid_field **field);
+
+static void usbhid_set_leds(struct hid_device *hid)
+{
+	struct hid_field *field;
+	int offset;
+
+	/*
+	 * Just reset Num Lock for now.
+	 * This is called for non-keyboard devices too, so no printk if field
+	 * is not found.
+	 */
+	if ((offset = hid_find_field_early(hid, HID_UP_LED, 0x01, &field)) != -1) {
+		hid_set_field(field, offset, 0);
+		usbhid_submit_report(hid, field->report, USB_DIR_OUT);
+	}
+}
+
 #define USB_VENDOR_ID_GTCO		0x078c
 #define USB_DEVICE_ID_GTCO_90		0x0090
 #define USB_DEVICE_ID_GTCO_100		0x0100
@@ -971,6 +993,30 @@ static void hid_find_max_report(struct hid_device *hid, unsigned int type, int *
 	}
 }
 
+static int hid_find_field_early(struct hid_device *hid, unsigned int page,
+    unsigned int hid_code, struct hid_field **pfield)
+{
+	struct hid_report *report;
+	struct hid_field *field;
+	struct hid_usage *usage;
+	int i, j;
+
+	list_for_each_entry(report, &hid->report_enum[HID_OUTPUT_REPORT].report_list, list) {
+		for (i = 0; i < report->maxfield; i++) {
+			field = report->field[i];
+			for (j = 0; j < field->maxusage; j++) {
+				usage = &field->usage[j];
+				if ((usage->hid & HID_USAGE_PAGE) == page &&
+				    (usage->hid & 0xFFFF) == hid_code) {
+					*pfield = field;
+					return j;
+				}
+			}
+		}
+	}
+	return -1;
+}
+
 static int hid_alloc_buffers(struct usb_device *dev, struct hid_device *hid)
 {
 	struct usbhid_device *usbhid = hid->driver_data;
@@ -1314,6 +1360,8 @@ static int hid_probe(struct usb_interface *intf, const struct usb_device_id *id)
 
 	usbhid_init_reports(hid);
 	hid_dump_device(hid);
+	/* if (hid->quirks & HID_QUIRK_RESET_LEDS) */
+	usbhid_set_leds(hid);
 
 	if (!hidinput_connect(hid))
 		hid->claimed |= HID_CLAIMED_INPUT;
diff --git a/include/linux/hid.h b/include/linux/hid.h
index d26b08f..f592f01 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -267,6 +267,7 @@ struct hid_item {
 #define HID_QUIRK_SKIP_OUTPUT_REPORTS		0x00020000
 #define HID_QUIRK_IGNORE_MOUSE			0x00040000
 #define HID_QUIRK_SONY_PS3_CONTROLLER		0x00080000
+#define HID_QUIRK_RESET_LEDS			0x00100000
 
 /*
  * This is the global environment of the parser. This information is

> > +++ b/include/linux/hid.h
> > @@ -267,6 +267,7 @@ struct hid_item {
> >  #define HID_QUIRK_SKIP_OUTPUT_REPORTS		0x00020000
> >  #define HID_QUIRK_IGNORE_MOUSE			0x00040000
> >  #define HID_QUIRK_SONY_PS3_CONTROLLER		0x00080000
> > +#define HID_QUIRK_RESET_LEDS			0x00100000
> 
> I think this is not worth a quirk - when we get it working properly, why 
> not do it unconditionally for all keyboards?

The main reason is, I have a USB-to-PS/2 adapter where early acces to
LEDs does not work. Apparently, it is still initializing the PS/2 part
when it reports to USB that it's ready, and needs a delay. So, I figured
that I may be breaking some odd devices. Some may crash or whatnot.
This is why I asked Stuart to get me VID/PID for involved keyboards.

> > URL with details, discussion, rejected patch to read BIOS byte at 0x417:
> > https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=228674
> 
> "You are not authorized to access bug #228674. To see this bug, you must 
> first log in to an account with the appropriate permissions." 

Sorry. I missed that the bug has access flags set...

-- Pete

Re: usb hid: reset NumLock

[Jiri Kosina]

On Sun, 1 Apr 2007, Pete Zaitcev wrote:

> diff --git a/drivers/usb/input/hid-core.c b/drivers/usb/input/hid-core.c
> index ef09952..7338e81 100644
> --- a/drivers/usb/input/hid-core.c
> +++ b/drivers/usb/input/hid-core.c
> @@ -548,6 +548,28 @@ void usbhid_init_reports(struct hid_device *hid)
>  		warn("timeout initializing reports");
>  }
>  
> +/*
> + * Reset LEDs which BIOS might have left on.
> + */
> +static int hid_find_field_early(struct hid_device *hid, unsigned int page,
> +    unsigned int hid_code, struct hid_field **field);

Hi Pete,

could you please change the order of the two functions, so that you 
don't have to put the forward declaration here?

> +	/*
> +	 * Just reset Num Lock for now.
> +	 * This is called for non-keyboard devices too, so no printk if field
> +	 * is not found.
> +	 */

I'd say this is a little bit overcommented.

> The main reason is, I have a USB-to-PS/2 adapter where early acces to 
> LEDs does not work. Apparently, it is still initializing the PS/2 part 
> when it reports to USB that it's ready, and needs a delay. So, I figured 
> that I may be breaking some odd devices. Some may crash or whatnot. This 
> is why I asked Stuart to get me VID/PID for involved keyboards.

Fair enough, thanks.

Besides the trivial nitpicks above, I think that this fixed version is 
fine. So as soon as you have the VIDs and PIDs of the hardware which 
requires this, could you please update the patch and send it to me again?

Thanks,

-- 
Jiri Kosina

Re: usb hid: reset NumLock

[Pete Zaitcev]

On Mon, 2 Apr 2007 16:48:24 +0200 (CEST), Jiri Kosina <jkosina@suse.cz> wrote:
> On Sun, 1 Apr 2007, Pete Zaitcev wrote:

> could you please change the order of the two functions, so that you 
> don't have to put the forward declaration here?
>[...]
> I'd say this is a little bit overcommented.
>[...]
> So as soon as you have the VIDs and PIDs of the hardware which 
> requires this, could you please update the patch and send it to me again?

How about this?

diff --git a/drivers/usb/input/hid-core.c b/drivers/usb/input/hid-core.c
index 827a75a..23b1e70 100644
--- a/drivers/usb/input/hid-core.c
+++ b/drivers/usb/input/hid-core.c
@@ -545,6 +545,45 @@ void usbhid_init_reports(struct hid_device *hid)
 		warn("timeout initializing reports");
 }
 
+/*
+ * Reset LEDs which BIOS might have left on. For now, just NumLock (0x01).
+ */
+
+static int hid_find_field_early(struct hid_device *hid, unsigned int page,
+    unsigned int hid_code, struct hid_field **pfield)
+{
+	struct hid_report *report;
+	struct hid_field *field;
+	struct hid_usage *usage;
+	int i, j;
+
+	list_for_each_entry(report, &hid->report_enum[HID_OUTPUT_REPORT].report_list, list) {
+		for (i = 0; i < report->maxfield; i++) {
+			field = report->field[i];
+			for (j = 0; j < field->maxusage; j++) {
+				usage = &field->usage[j];
+				if ((usage->hid & HID_USAGE_PAGE) == page &&
+				    (usage->hid & 0xFFFF) == hid_code) {
+					*pfield = field;
+					return j;
+				}
+			}
+		}
+	}
+	return -1;
+}
+
+static void usbhid_set_leds(struct hid_device *hid)
+{
+	struct hid_field *field;
+	int offset;
+
+	if ((offset = hid_find_field_early(hid, HID_UP_LED, 0x01, &field)) != -1) {
+		hid_set_field(field, offset, 0);
+		usbhid_submit_report(hid, field->report, USB_DIR_OUT);
+	}
+}
+
 #define USB_VENDOR_ID_GTCO		0x078c
 #define USB_DEVICE_ID_GTCO_90		0x0090
 #define USB_DEVICE_ID_GTCO_100		0x0100
@@ -765,6 +804,9 @@ void usbhid_init_reports(struct hid_device *hid)
 #define USB_VENDOR_ID_SONY			0x054c
 #define USB_DEVICE_ID_SONY_PS3_CONTROLLER	0x0268
 
+#define USB_VENDOR_ID_DELL		0x413c
+#define USB_DEVICE_ID_DELL_W7658	0x2005
+
 /*
  * Alphabetically sorted blacklist by quirk type.
  */
@@ -947,6 +989,8 @@ static const struct hid_blacklist {
 
 	{ USB_VENDOR_ID_CIDC, 0x0103, HID_QUIRK_IGNORE },
 
+	{ USB_VENDOR_ID_DELL, USB_DEVICE_ID_DELL_W7658, HID_QUIRK_RESET_LEDS },
+
 	{ 0, 0 }
 };
 
@@ -1334,6 +1378,8 @@ static int hid_probe(struct usb_interface *intf, const struct usb_device_id *id)
 
 	usbhid_init_reports(hid);
 	hid_dump_device(hid);
+	if (hid->quirks & HID_QUIRK_RESET_LEDS)
+		usbhid_set_leds(hid);
 
 	if (!hidinput_connect(hid))
 		hid->claimed |= HID_CLAIMED_INPUT;
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 8c97d4d..3e8dcb0 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -269,6 +269,7 @@ struct hid_item {
 #define HID_QUIRK_SONY_PS3_CONTROLLER		0x00080000
 #define HID_QUIRK_LOGITECH_S510_DESCRIPTOR	0x00100000
 #define HID_QUIRK_DUPLICATE_USAGES		0x00200000
+#define HID_QUIRK_RESET_LEDS			0x00400000
 
 /*
  * This is the global environment of the parser. This information is

I wasn't sure where to place the function, so I just put it above its
user, to signify that it's special-casing. Also, it's unclear where
to put the quirk entry and defines. There's a comment saying that they
are sorted alphabetically by quirk, but apparently the order was violated
with more recent additions.

-- Pete

Re: usb hid: reset NumLock

[Dmitry Torokhov]

On Monday 02 April 2007 19:12, Pete Zaitcev wrote:
> On Mon, 2 Apr 2007 16:48:24 +0200 (CEST), Jiri Kosina <jkosina@suse.cz> wrote:
> > On Sun, 1 Apr 2007, Pete Zaitcev wrote:
> 
> > could you please change the order of the two functions, so that you 
> > don't have to put the forward declaration here?
> >[...]
> > I'd say this is a little bit overcommented.
> >[...]
> > So as soon as you have the VIDs and PIDs of the hardware which 
> > requires this, could you please update the patch and send it to me again?
> 
> How about this?

Actually I think I will be adding the patch below, but it has to wait
till 2.6.22 as it requires input core to struct device conversion
patch.

What do you think?

-- 
Dmitry

Input: add generic suspend and resume for uinput devices

Automatically turn off leds and sound effects as part of suspend
process and restore led state, sounds and repeat rate at resume.

Also synchronize hardware state with logical state at device
registration.

Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---

 drivers/input/input.c |   80 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 80 insertions(+)

Index: work/drivers/input/input.c
===================================================================
--- work.orig/drivers/input/input.c
+++ work/drivers/input/input.c
@@ -997,10 +997,88 @@ static int input_dev_uevent(struct devic
 	return 0;
 }
 
+static void input_dev_toggle(struct input_dev *dev,
+			     unsigned int type, unsigned int code,
+			     unsigned long *cap_bits, unsigned long *bits,
+			     int force_off)
+{
+	if (test_bit(code, cap_bits)) {
+		if (!force_off)
+			dev->event(dev, type, code, test_bit(code, bits));
+		else if (test_bit(code, bits))
+			dev->event(dev, type, code, 0);
+	}
+}
+
+static void input_dev_reset(struct input_dev *dev, int force_off)
+{
+	int i;
+
+	if (!dev->event)
+		return;
+
+	/* synchronize led state */
+	if (test_bit(EV_LED, dev->evbit))
+		for (i = 0; i <= LED_MAX; i++)
+			input_dev_toggle(dev, EV_LED, i,
+					 dev->ledbit, dev->led, force_off);
+
+	/* restore sound */
+	if (test_bit(EV_SND, dev->evbit))
+		for (i = 0; i <= SND_MAX; i++)
+			input_dev_toggle(dev, EV_SND, i,
+					 dev->sndbit, dev->snd, force_off);
+
+	if (!force_off && test_bit(EV_REP, dev->evbit)) {
+		dev->event(dev, EV_REP, REP_PERIOD, dev->rep[REP_PERIOD]);
+		dev->event(dev, EV_REP, REP_DELAY, dev->rep[REP_DELAY]);
+	}
+}
+
+#ifdef CONFIG_PM
+static int input_dev_suspend(struct device *dev, pm_message_t state)
+{
+	struct input_dev *input_dev = to_input_dev(dev);
+
+	mutex_lock(&input_dev->mutex);
+
+	if (dev->power.power_state.event != state.event) {
+		if (state.event == PM_EVENT_SUSPEND)
+			input_dev_reset(input_dev, 1);
+
+		dev->power.power_state = state;
+	}
+
+	mutex_unlock(&input_dev->mutex);
+
+	return 0;
+}
+
+static int input_dev_resume(struct device *dev)
+{
+	struct input_dev *input_dev = to_input_dev(dev);
+
+	mutex_lock(&input_dev->mutex);
+
+	if (dev->power.power_state.event != PM_EVENT_ON)
+		input_dev_reset(to_input_dev(dev), 0);
+
+	dev->power.power_state = PMSG_ON;
+
+	mutex_unlock(&input_dev->mutex);
+
+	return 0;
+}
+#endif /* CONFIG_PM */
+
 static struct device_type input_dev_type = {
 	.groups		= input_dev_attr_groups,
 	.release	= input_dev_release,
 	.uevent		= input_dev_uevent,
+#ifdef CONFIG_PM
+	.suspend	= input_dev_suspend,
+	.resume		= input_dev_resume,
+#endif
 };
 
 struct class input_class = {
@@ -1080,6 +1158,8 @@ int input_register_device(struct input_d
 		dev->rep[REP_PERIOD] = 33;
 	}
 
+	input_dev_reset(dev, 0);
+
 	if (!dev->getkeycode)
 		dev->getkeycode = input_default_getkeycode;
 

Re: usb hid: reset NumLock

[Pete Zaitcev]

On Tue, 3 Apr 2007 01:04:05 -0400, Dmitry Torokhov <dtor@insightbb.com> wrote:

> What do you think?

The patch looks sane, although I haven't yet tested it. I'll live with
it until next quarterly update, then consider if I should take it or use
my patch for RHEL 5 and RHEL 4.

-- Pete

Re: usb hid: reset NumLock

[Jiri Kosina]

On Mon, 2 Apr 2007, Pete Zaitcev wrote:

> How about this?

Looks quite fine to me.

But in case that Dmitry's patch "Input: add generic suspend and resume for 
uinput devices" fixes your issue too, I wouldn't merge it as it won't be 
needed. Could you please let me know?

> @@ -947,6 +989,8 @@ static const struct hid_blacklist {
>  
>  	{ USB_VENDOR_ID_CIDC, 0x0103, HID_QUIRK_IGNORE },
>  
> +	{ USB_VENDOR_ID_DELL, USB_DEVICE_ID_DELL_W7658, HID_QUIRK_RESET_LEDS },
> +

As this quirk is only needed once in the initialization method, we could 
probably get rid of it and just put and explicit test for PID and VID 
there (with apropriate comment). We can't afford wasting quirk entries, as 
we are slowly running out of them.

Depends on whether this is going to be needed for > 1 PID.

> I wasn't sure where to place the function, so I just put it above its 
> user, to signify that it's special-casing. Also, it's unclear where to 
> put the quirk entry and defines. There's a comment saying that they are 
> sorted alphabetically by quirk, but apparently the order was violated 
> with more recent additions.

Yep, the blacklist is ugly. I have a patch moving it to the proper place 
and rearranging it to be sorted again, queued for 2.6.22.

Thanks,

-- 
Jiri Kosina

Re: [linux-usb-devel] usb hid: reset NumLock

[Robert Marquardt]

Jiri Kosina wrote:

> As this quirk is only needed once in the initialization method, we could 
> probably get rid of it and just put and explicit test for PID and VID 
> there (with apropriate comment). We can't afford wasting quirk entries, as 
> we are slowly running out of them.

Is it a problem of table size or number of table entries?
For the number of entries a simple change to PID ranges should help.
It would also allow to move Wacom to the blacklist.
I have sent a patch for that already, but it was rejected by Greg.

Re: usb hid: reset NumLock

[Jiri Kosina]

On Tue, 3 Apr 2007, Robert Marquardt wrote:

> > As this quirk is only needed once in the initialization method, we 
> > could probably get rid of it and just put and explicit test for PID 
> > and VID there (with apropriate comment). We can't afford wasting quirk 
> > entries, as we are slowly running out of them.
> Is it a problem of table size or number of table entries? For the number 
> of entries a simple change to PID ranges should help. It would also 
> allow to move Wacom to the blacklist. I have sent a patch for that 
> already, but it was rejected by Greg.

The issue is that 32 bits of the quirk bitmask are going to be taken by 
the quirk entries (so no, it's not related to the size of the table).

Thanks,

-- 
Jiri Kosina

Re: usb hid: reset NumLock

[Dmitry Torokhov]

On Tuesday 03 April 2007 04:52, Jiri Kosina wrote:
> On Mon, 2 Apr 2007, Pete Zaitcev wrote:
> 
> > How about this?
> 
> Looks quite fine to me.
> 
> But in case that Dmitry's patch "Input: add generic suspend and resume for 
> uinput devices" fixes your issue too, I wouldn't merge it as it won't be 
> needed. Could you please let me know?

Unfortunately my patch is crap. We should not be sending events down
dev->event() until dev->open() has been called because many drivers
start hardware from there and not ready until then.

So it is HID driver responsibility to properly reset leds after all.

-- 
Dmitry

Re: usb hid: reset NumLock

[Jiri Kosina]

On Wed, 4 Apr 2007, Dmitry Torokhov wrote:

> Unfortunately my patch is crap. We should not be sending events down 
> dev->event() until dev->open() has been called because many drivers 
> start hardware from there and not ready until then. So it is HID driver 
> responsibility to properly reset leds after all.

Pete, could you please resend your patch, with proper metadata, we need to 
merge your one then.

Thanks,

-- 
Jiri Kosina

Re: usb hid: reset NumLock

[Pete Zaitcev]

On a certain keyboard, when BIOS sets NumLock LED on, it survives the takeover
by Linux and thus confuses users.

Eating of an increasibly scarce quirk bit is unfortunate. We do it for safety,
given the history of nervous input devices which crash if anything unusual
happens.

Signed-off-by: Pete Zaitcev <zaitcev@redhat.com>

---

diff --git a/drivers/usb/input/hid-core.c b/drivers/usb/input/hid-core.c
index 827a75a..23b1e70 100644
--- a/drivers/usb/input/hid-core.c
+++ b/drivers/usb/input/hid-core.c
@@ -545,6 +545,45 @@ void usbhid_init_reports(struct hid_device *hid)
 		warn("timeout initializing reports");
 }
 
+/*
+ * Reset LEDs which BIOS might have left on. For now, just NumLock (0x01).
+ */
+
+static int hid_find_field_early(struct hid_device *hid, unsigned int page,
+    unsigned int hid_code, struct hid_field **pfield)
+{
+	struct hid_report *report;
+	struct hid_field *field;
+	struct hid_usage *usage;
+	int i, j;
+
+	list_for_each_entry(report, &hid->report_enum[HID_OUTPUT_REPORT].report_list, list) {
+		for (i = 0; i < report->maxfield; i++) {
+			field = report->field[i];
+			for (j = 0; j < field->maxusage; j++) {
+				usage = &field->usage[j];
+				if ((usage->hid & HID_USAGE_PAGE) == page &&
+				    (usage->hid & 0xFFFF) == hid_code) {
+					*pfield = field;
+					return j;
+				}
+			}
+		}
+	}
+	return -1;
+}
+
+static void usbhid_set_leds(struct hid_device *hid)
+{
+	struct hid_field *field;
+	int offset;
+
+	if ((offset = hid_find_field_early(hid, HID_UP_LED, 0x01, &field)) != -1) {
+		hid_set_field(field, offset, 0);
+		usbhid_submit_report(hid, field->report, USB_DIR_OUT);
+	}
+}
+
 #define USB_VENDOR_ID_GTCO		0x078c
 #define USB_DEVICE_ID_GTCO_90		0x0090
 #define USB_DEVICE_ID_GTCO_100		0x0100
@@ -765,6 +804,9 @@ void usbhid_init_reports(struct hid_device *hid)
 #define USB_VENDOR_ID_SONY			0x054c
 #define USB_DEVICE_ID_SONY_PS3_CONTROLLER	0x0268
 
+#define USB_VENDOR_ID_DELL		0x413c
+#define USB_DEVICE_ID_DELL_W7658	0x2005
+
 /*
  * Alphabetically sorted blacklist by quirk type.
  */
@@ -947,6 +989,8 @@ static const struct hid_blacklist {
 
 	{ USB_VENDOR_ID_CIDC, 0x0103, HID_QUIRK_IGNORE },
 
+	{ USB_VENDOR_ID_DELL, USB_DEVICE_ID_DELL_W7658, HID_QUIRK_RESET_LEDS },
+
 	{ 0, 0 }
 };
 
@@ -1334,6 +1378,8 @@ static int hid_probe(struct usb_interface *intf, const struct usb_device_id *id)
 
 	usbhid_init_reports(hid);
 	hid_dump_device(hid);
+	if (hid->quirks & HID_QUIRK_RESET_LEDS)
+		usbhid_set_leds(hid);
 
 	if (!hidinput_connect(hid))
 		hid->claimed |= HID_CLAIMED_INPUT;
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 8c97d4d..3e8dcb0 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -269,6 +269,7 @@ struct hid_item {
 #define HID_QUIRK_SONY_PS3_CONTROLLER		0x00080000
 #define HID_QUIRK_LOGITECH_S510_DESCRIPTOR	0x00100000
 #define HID_QUIRK_DUPLICATE_USAGES		0x00200000
+#define HID_QUIRK_RESET_LEDS			0x00400000
 
 /*
  * This is the global environment of the parser. This information is

Re: usb hid: reset NumLock

[Dmitry Torokhov]

On 4/5/07, Pete Zaitcev <zaitcev@redhat.com> wrote:
> On a certain keyboard, when BIOS sets NumLock LED on, it survives the takeover
> by Linux and thus confuses users.
>
> Eating of an increasibly scarce quirk bit is unfortunate. We do it for safety,
> given the history of nervous input devices which crash if anything unusual
> happens.
>

You know, I would not call turning leds on an off an unusual operation
for a keyboard.

-- 
Dmitry

Re: usb hid: reset NumLock

[Pete Zaitcev]

On Thu, 5 Apr 2007 16:54:17 -0400, "Dmitry Torokhov" <dmitry.torokhov@gmail.com> wrote:

> > On a certain keyboard, when BIOS sets NumLock LED on, it survives the takeover
> > by Linux and thus confuses users.
> >
> > Eating of an increasibly scarce quirk bit is unfortunate. We do it for safety,
> > given the history of nervous input devices which crash if anything unusual
> > happens.

> You know, I would not call turning leds on an off an unusual operation
> for a keyboard.

Right, but monkeying with them immediately upon initialization may be
"unusual", meaning "device was tested with one version of Windows and
shipped, everything else is unusual".

-- Pete