input: correctly handle keys without hardware release event

PATCH In review Giel de Nijs 2 participants 5 messages May 11, 2007 – May 14, 2007

Subsystems (3)

HID Char and misc drivers Input drivers

lore

Share & tools

lore

Giel de Nijs

author

2007-05-11 17:23 UTC #0 lore raw

cover input: correctly handle keys without hardware release event

Hi,

This patch adds a soft release key mask to input_dev, to enable keyboard drivers to determine which keys never generate a hardware release event and hence add a release event after every press event of such keys. The mask is controlled by ioctls.

The Fn+F? key combinations of Dell Latitude series laptops (and possibly other Dells or other brands) only generate a key press event and never a key release event, which is most probable a hardware flaw (or feature?). Due to this flaw, combinations like Fn+F1 for hibernate and Fn-F3 for showing battery status cannot be used. Ubuntu has probably fixed this by patching the X input layer and HAL, but other distributions (like Debian) cannot use these keys. This patch adds a generic method to signal if keys with certain scancodes never generate release events, so the keyboard driver can add those events right after a key press event.

The ioctls used to read and write to this bitmask might be used in a program like setkeycodes, which is normally used to map certain scancodes to keycodes. With a command line option, this program could also set the soft release bit for a certain scancode if desired. Patches for setkeycodes and getkeycodes against the Debian console-tools can be found at
http://giel.operation0.org/keyboard-soft-release

This patch also uses the infrastructure for generating release events for KEY_HANGEUL and KEY_HANJA, something which was already done in atkbd.c.

See also this thread: http://thread.gmane.org/gmane.linux.kernel/401378

Greetings,
Giel

Giel de Nijs

author

2007-05-11 17:23 UTC #1 lore raw

PATCH 1/2 input: add soft release key mask to keyboard driver

Add keysrlmask to input_dev and add soft release handling in atkbd.c

Signed-off-by Giel de Nijs <[email protected]>

---

drivers/input/keyboard/atkbd.c +11 -8

Index: linux-2.6.21.1/drivers/input/keyboard/atkbd.c
===================================================================
@@ -201,6 +201,7 @@
 
 	unsigned short id;
 	unsigned char keycode[512];
+	unsigned long keysrlmask[NBITS(512)];
 	unsigned char set;
 	unsigned char translated;
 	unsigned char extra;

@@ -412,14 +413,6 @@
 			       "Some program might be trying access hardware directly.\n",
 			       data == ATKBD_RET_ACK ? "ACK" : "NAK", serio->phys);
 			goto out;
-		case ATKBD_RET_HANGEUL:
-		case ATKBD_RET_HANJA:
-			/*
-			 * These keys do not report release and thus need to be
-			 * flagged properly
-			 */
-			add_release_event = 1;
-			break;
 		case ATKBD_RET_ERR:
 			atkbd->err_count++;
 #ifdef ATKBD_DEBUG

@@ -438,6 +431,9 @@
 	if (keycode != ATKBD_KEY_NULL)
 		input_event(dev, EV_MSC, MSC_SCAN, code);
 
+	if (test_bit(code, atkbd->keysrlmask))
+		add_release_event = 1;
+
 	switch (keycode) {
 		case ATKBD_KEY_NULL:
 			break;

@@ -832,6 +828,7 @@
 	int i, j;
 
 	memset(atkbd->keycode, 0, sizeof(atkbd->keycode));
+	bitmap_zero(atkbd->keysrlmask, 512);
 
 	if (atkbd->translated) {
 		for (i = 0; i < 128; i++) {

@@ -854,6 +851,11 @@
 
 	atkbd->keycode[atkbd_compat_scancode(atkbd, ATKBD_RET_HANGEUL)] = KEY_HANGUEL;
 	atkbd->keycode[atkbd_compat_scancode(atkbd, ATKBD_RET_HANJA)] = KEY_HANJA;
+
+	set_bit(atkbd_compat_scancode(atkbd, ATKBD_RET_HANGEUL),
+		atkbd->keysrlmask);
+	set_bit(atkbd_compat_scancode(atkbd, ATKBD_RET_HANJA),
+		atkbd->keysrlmask);
 }
 
 /*

@@ -911,6 +913,7 @@
 	}
 
 	input_dev->keycode = atkbd->keycode;
+	input_dev->keysrlmask = atkbd->keysrlmask;
 	input_dev->keycodesize = sizeof(unsigned char);
 	input_dev->keycodemax = ARRAY_SIZE(atkbd_set2_keycode);

include/linux/input.h +1 -0

Index: linux-2.6.21.1/include/linux/input.h
===================================================================
@@ -962,6 +962,7 @@
 	unsigned int keycodemax;
 	unsigned int keycodesize;
 	void *keycode;
+	unsigned long *keysrlmask; /* soft-release scancodes key mask */
 
 	struct ff_device *ff;

Giel de Nijs

author

2007-05-11 17:23 UTC #2 lore raw

PATCH 2/2 input: add ioctls to console for soft release mask read/write

In reply to Giel de Nijs

Add KDGETKEYSRL and KDSETKEYSRL to the console to control the soft release key mask in input_dev.

Signed-off-by Giel de Nijs <[email protected]>

---

drivers/char/vt_ioctl.c +29 -0

Index: linux-2.6.21.1/drivers/char/vt_ioctl.c
===================================================================
@@ -181,6 +181,29 @@
 	return kc;
 }
 
+static int
+do_kbkeysrl_ioctl(int cmd, struct kbkeysrl __user *user_kbks, int perm)
+{
+	struct kbkeysrl tmp;
+	int srl = 0;
+
+	if (copy_from_user(&tmp, user_kbks, sizeof(struct kbkeysrl)))
+		return -EFAULT;
+	switch (cmd) {
+	case KDGETKEYSRL:
+		srl = getkeysrl(tmp.scancode);
+		if (srl >= 0)
+			srl = put_user(srl, &user_kbks->srl);
+		break;
+	case KDSETKEYSRL:
+		if (!perm)
+			return -EPERM;
+		srl = setkeysrl(tmp.scancode, tmp.srl);
+		break;
+	}
+	return srl;
+}
+
 static inline int
 do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm)
 {

@@ -571,6 +594,12 @@
 			perm=0;
 		return do_kbkeycode_ioctl(cmd, up, perm);
 
+	case KDGETKEYSRL:
+	case KDSETKEYSRL:
+		if(!capable(CAP_SYS_TTY_CONFIG))
+			perm=0;
+		return do_kbkeysrl_ioctl(cmd, up, perm);
+
 	case KDGKBENT:
 	case KDSKBENT:
 		return do_kdsk_ioctl(cmd, up, perm, kbd);

include/linux/kbd_kern.h +2 -0

Index: linux-2.6.21.1/include/linux/kbd_kern.h
===================================================================
@@ -143,6 +143,8 @@
 
 int getkeycode(unsigned int scancode);
 int setkeycode(unsigned int scancode, unsigned int keycode);
+int getkeysrl(unsigned int scancode);
+int setkeysrl(unsigned int scancode, int srl);
 void compute_shiftstate(void);
 
 /* defkeymap.c */

include/linux/kd.h +7 -0

Index: linux-2.6.21.1/include/linux/kd.h
===================================================================
@@ -144,6 +144,13 @@
 
 #define KDFONTOP	0x4B72	/* font operations */
 
+struct kbkeysrl {
+	unsigned int scancode;
+	int srl;
+};
+#define KDGETKEYSRL	0x4B82  /* read kernel soft release key table entry */
+#define KDSETKEYSRL	0x4B83  /* write kernel soft release key table entry */
+
 struct console_font_op {
 	unsigned int op;	/* operation code KD_FONT_OP_* */
 	unsigned int flags;	/* KD_FONT_FLAG_* */

drivers/char/keyboard.c +51 ~~-11~~

88 lines not on this page · raw message

include/linux/compat_ioctl.h +2 -0

Index: linux-2.6.21.1/include/linux/compat_ioctl.h
===================================================================
@@ -172,6 +172,8 @@
 ULONG_IOCTL(KDSIGACCEPT)
 COMPATIBLE_IOCTL(KDGETKEYCODE)
 COMPATIBLE_IOCTL(KDSETKEYCODE)
+COMPATIBLE_IOCTL(KDGETKEYSRL)
+COMPATIBLE_IOCTL(KDSETKEYSRL)
 ULONG_IOCTL(KIOCSOUND)
 ULONG_IOCTL(KDMKTONE)
 COMPATIBLE_IOCTL(KDGKBTYPE)

Dmitry Torokhov

2007-05-14 03:44 UTC #3 lore raw

In reply to Giel de Nijs

Hi Giel,

Quoted 6 lines · Giel de Nijs

Hi,
This patch adds a soft release key mask to input_dev, to enable keyboard drivers to determine which keys never generate a hardware release event and hence add a release event after every press event of such keys. The mask is controlled by ioctls.

I don't think we want to add all the infrastructure for the benefit of single driver. Can we add a quirk to atkbd and activate it based on DMI?

Signature · 1 line

Dmitry

Giel de Nijs

author

2007-05-14 06:16 UTC #4 lore raw

Quoted 7 lines · Dmitry Torokhov

Quoted 4 lines
This patch adds a soft release key mask to input_dev, to enable keyboard drivers to determine which keys never generate a hardware release event and hence add a release event after every press event of such keys. The mask is controlled by ioctls.
I don't think we want to add all the infrastructure for the benefit of single driver. Can we add a quirk to atkbd and activate it based on DMI?

As the atkbd.c driver already had a work-around for exactly the same problem with some (I think) Korean keyboards (KEY_HANGEUL and KEY_HANJA), adding this infrastructure (well, one pointer and two ioctls) seemed a logical step. If you prefer a hack just in atkbd.c and activate it on Dell laptops, I see what I can do. The problem with that is that I don't know the scancodes of all keys on all models (and future models) that exhibit this behaviour. I'll implement what I know, though (it'll take some time because I'll be travelling quite a lot very soon).

Greetings,
Giel