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;
 }
 ...
[ message continues ]

Hi Pete,


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
-

[ message continues ]

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
-

[ message continues ]

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
-

[ message continues ]

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 

I think this is not worth a quirk - when we get it working properly, why 

"You are not authorized to access bug #228674. To see this bug, you must 

Thanks,

-- 
Jiri Kosina
-

[ message continues ]

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
-

[ message continues ]

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 ...
[ message continues ]

Hi Pete,

could you please change the order of the two functions, so that you 


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
-

[ message continues ]

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 ...
[ message continues ]

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 ...
[ message continues ]

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
-

[ message continues ]

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 

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.


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
-

[ message continues ]

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

[ message continues ]

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
-

[ message continues ]

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
-

[ message continues ]

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

Thanks,

-- 
Jiri Kosina
-

[ message continues ]

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 ...
[ message continues ]

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

-- 
Dmitry
-

[ message continues ]

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
-

[ message continues ]

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

[ message continues ]

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
-

[ message continues ]

Hi Pekka,


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
-

[ message continues ]