login
Search
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;
}
...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 -
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 -
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 -
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 -
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 -
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 ...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 -
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 ...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 ...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 -
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 -
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 -
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 -
Pete, could you please resend your patch, with proper metadata, we need to merge your one then. Thanks, -- Jiri Kosina -
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 ...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 -
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 -
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 -
