[PATCH 23/28] keyboard, input: Add hook to input to allow low level event clear

[PATCH 23/28] keyboard, input: Add hook to input to allow low level event clear

[Jason Wessel]

When using a keyboard with kdb, on resuming the system there needs to
be a hook to allow for the keyboard state to get reset.

This is mainly because there is no way to force the end user to hold
down the original keys that were pressed prior to entering kdb.

CC: Dmitry Torokhov <dmitry.torokhov@gmail.com>
CC: Henrik Rydberg <rydberg@euromail.se>
CC: Greg Kroah-Hartman <gregkh@suse.de>
CC: Alexey Dobriyan <adobriyan@gmail.com>
CC: Kay Sievers <kay.sievers@vrfy.org>
CC: linux-input@vger.kernel.org
Signed-off-by: Jason Wessel <jason.wessel@windriver.com>
---
 drivers/char/keyboard.c |   29 ++++++++++++++++++++++++-----
 drivers/input/input.c   |   15 +++++++++++++++
 drivers/serial/kgdboc.c |   13 +++++++++++++
 include/linux/input.h   |   10 ++++++++++
 4 files changed, 62 insertions(+), 5 deletions(-)

diff --git a/drivers/char/keyboard.c b/drivers/char/keyboard.c
index f706b1d..d61145a 100644
--- a/drivers/char/keyboard.c
+++ b/drivers/char/keyboard.c
@@ -1195,6 +1195,11 @@ static void kbd_keycode(unsigned int keycode, int down, int hw_raw)
 			if (keycode < BTN_MISC && printk_ratelimit())
 				printk(KERN_WARNING "keyboard.c: can't emulate rawmode for keycode %d\n", keycode);
 
+	if (down)
+		set_bit(keycode, key_down);
+	else
+		clear_bit(keycode, key_down);
+
 #ifdef CONFIG_MAGIC_SYSRQ	       /* Handle the SysRq Hack */
 	if (keycode == KEY_SYSRQ && (sysrq_down || (down == 1 && sysrq_alt))) {
 		if (!sysrq_down) {
@@ -1237,11 +1242,6 @@ static void kbd_keycode(unsigned int keycode, int down, int hw_raw)
 		raw_mode = 1;
 	}
 
-	if (down)
-		set_bit(keycode, key_down);
-	else
-		clear_bit(keycode, key_down);
-
 	if (rep &&
 	    (!vc_kbd_mode(kbd, VC_REPEAT) ||
 	     (tty && !L_ECHO(tty) && tty_chars_in_buffer(tty)))) {
@@ -1410,6 +1410,22 @@ static const struct input_device_id kbd_ids[] = {
 
 MODULE_DEVICE_TABLE(input, kbd_ids);
 
+#ifdef CONFIG_KDB_KEYBOARD
+void kbd_clear_keys(void)
+{
+	int i, j, k;
+
+	for (i = 0; i < ARRAY_SIZE(key_down); i++) {
+		k = i * BITS_PER_LONG;
+		for (j = 0; j < BITS_PER_LONG; j++, k++) {
+			if (test_bit(k, key_down)) {
+				kbd_keycode(k, 0, 0);
+			}
+		}
+	}
+}
+#endif
+
 static struct input_handler kbd_handler = {
 	.event		= kbd_event,
 	.connect	= kbd_connect,
@@ -1417,6 +1433,9 @@ static struct input_handler kbd_handler = {
 	.start		= kbd_start,
 	.name		= "kbd",
 	.id_table	= kbd_ids,
+#ifdef CONFIG_KDB_KEYBOARD
+	.dbg_clear_keys = kbd_clear_keys,
+#endif
 };
 
 int __init kbd_init(void)
diff --git a/drivers/input/input.c b/drivers/input/input.c
index 86cb2d2..7473300 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -1692,6 +1692,21 @@ int input_register_handler(struct input_handler *handler)
 }
 EXPORT_SYMBOL(input_register_handler);
 
+#ifdef CONFIG_KDB_KEYBOARD
+/* input_db_clear_keys - Clear any keyboards if they have a call back,
+ * after returning from the kernel debugger
+ */
+void input_dbg_clear_keys(void)
+{
+	struct input_handler *handler;
+
+	list_for_each_entry(handler, &input_handler_list, node)
+		if (handler->dbg_clear_keys)
+			handler->dbg_clear_keys();
+}
+EXPORT_SYMBOL_GPL(input_dbg_clear_keys);
+#endif
+
 /**
  * input_unregister_handler - unregisters an input handler
  * @handler: handler to be unregistered
diff --git a/drivers/serial/kgdboc.c b/drivers/serial/kgdboc.c
index b765ab4..201cdf5 100644
--- a/drivers/serial/kgdboc.c
+++ b/drivers/serial/kgdboc.c
@@ -17,6 +17,7 @@
 #include <linux/kdb.h>
 #include <linux/tty.h>
 #include <linux/console.h>
+#include <linux/input.h>
 
 #define MAX_CONFIG_LEN		40
 
@@ -35,12 +36,16 @@ static struct tty_driver	*kgdb_tty_driver;
 static int			kgdb_tty_line;
 
 #ifdef CONFIG_KDB_KEYBOARD
+static int kgdboc_use_kbd;  /* 1 if we use a keyboard */
+
 static int kgdboc_register_kbd(char **cptr)
 {
+	kgdboc_use_kbd = 0;
 	if (strncmp(*cptr, "kbd", 3) == 0) {
 		if (kdb_poll_idx < KDB_POLL_FUNC_MAX) {
 			kdb_poll_funcs[kdb_poll_idx] = kdb_get_kbd_char;
 			kdb_poll_idx++;
+			kgdboc_use_kbd = 1;
 			if (cptr[0][3] == ',')
 				*cptr += 4;
 			else
@@ -63,9 +68,16 @@ static void kgdboc_unregister_kbd(void)
 		}
 	}
 }
+
+static inline void kgdboc_clear_kbd(void)
+{
+	if (kgdboc_use_kbd)
+		input_dbg_clear_keys(); /* Release all pressed keys */
+}
 #else /* ! CONFIG_KDB_KEYBOARD */
 #define kgdboc_register_kbd(x) 0
 #define kgdboc_unregister_kbd()
+#define kgdboc_clear_kbd()
 #endif /* ! CONFIG_KDB_KEYBOARD */
 
 static int kgdboc_option_setup(char *opt)
@@ -213,6 +225,7 @@ static void kgdboc_post_exp_handler(void)
 	/* decrement the module count when the debugger detaches */
 	if (!kgdb_connected)
 		module_put(THIS_MODULE);
+	kgdboc_clear_kbd();
 }
 
 static struct kgdb_io kgdboc_io_ops = {
diff --git a/include/linux/input.h b/include/linux/input.h
index 663208a..9529b63 100644
--- a/include/linux/input.h
+++ b/include/linux/input.h
@@ -1232,6 +1232,9 @@ struct input_handler {
 	int (*connect)(struct input_handler *handler, struct input_dev *dev, const struct input_device_id *id);
 	void (*disconnect)(struct input_handle *handle);
 	void (*start)(struct input_handle *handle);
+#ifdef CONFIG_KDB_KEYBOARD
+	void (*dbg_clear_keys)(void);
+#endif
 
 	const struct file_operations *fops;
 	int minor;
@@ -1317,6 +1320,13 @@ int input_flush_device(struct input_handle* handle, struct file* file);
 void input_event(struct input_dev *dev, unsigned int type, unsigned int code, int value);
 void input_inject_event(struct input_handle *handle, unsigned int type, unsigned int code, int value);
 
+#ifdef CONFIG_KDB_KEYBOARD
+void input_dbg_clear_keys(void);
+#else
+static inline void input_dbg_clear_keys(void)
+{}
+#endif
+
 static inline void input_report_key(struct input_dev *dev, unsigned int code, int value)
 {
 	input_event(dev, EV_KEY, code, !!value);
-- 
1.6.4.rc1

Re: [PATCH 23/28] keyboard, input: Add hook to input to allow low level event clear

[Dmitry Torokhov]

On Thu, Feb 25, 2010 at 03:21:28PM -0600, Jason Wessel wrote:
> When using a keyboard with kdb, on resuming the system there needs to
> be a hook to allow for the keyboard state to get reset.
> 
> This is mainly because there is no way to force the end user to hold
> down the original keys that were pressed prior to entering kdb.
> 

Instead of adding all the new hook can't you copy the bitmap of
currently pressed keys when you invoke kdb and theni, on exit, use
input_inject_event() to clear bitmasks in the devices?

-- 
Dmitry

Re: [PATCH 23/28] keyboard, input: Add hook to input to allow low level event clear

[Jason Wessel]

Dmitry Torokhov wrote:
> On Thu, Feb 25, 2010 at 03:21:28PM -0600, Jason Wessel wrote:
>> When using a keyboard with kdb, on resuming the system there needs to
>> be a hook to allow for the keyboard state to get reset.
>>
>> This is mainly because there is no way to force the end user to hold
>> down the original keys that were pressed prior to entering kdb.
>>
> 
> Instead of adding all the new hook can't you copy the bitmap of
> currently pressed keys when you invoke kdb and theni, on exit, use
> input_inject_event() to clear bitmasks in the devices?
> 

I know just a little more about the input system then I did 6 months
ago.  I am not sure that input_inject_event() is exactly what should
be used, but perhaps you had a different idea in mind.

I created a new patch which uses the same sort of concept.  I moved
the key release code from input_disconnect_device() into a common
function, so that it could be called by the debugger key free hook.

Would you consider acking this version (for which I can apply a delta
patch to the kdb code base)?  Or let me know what further comments you
have?

Thanks,
Jason.

---

From: Jason Wessel <jason.wessel@windriver.com>
Subject: [PATCH] input,keyboard,kgdboc: Allow key release on kernel resume

When using a keyboard with kdb, a hook point to free all the
keystrokes is required for resuming kernel operations.

This is mainly because there is no way to force the end user to hold
down the original keys that were pressed prior to entering kdb.

The change to keyboard.c fixes a defect where the keyboard bitmap map
state is not correct while in a sysrq handler.  The bitmap needs to be
updated before calling the sysrq handler.

When resuming the kernel kgdboc's callback will attempt to release all
the keys if the locks are available, else tasklet will be schedule to
release all the keys when the kernel is full resumed.

CC: Dmitry Torokhov <dmitry.torokhov@gmail.com>
CC: Henrik Rydberg <rydberg@euromail.se>
CC: Greg Kroah-Hartman <gregkh@suse.de>
CC: Alexey Dobriyan <adobriyan@gmail.com>
CC: Kay Sievers <kay.sievers@vrfy.org>
CC: linux-input@vger.kernel.org
Signed-off-by: Jason Wessel <jason.wessel@windriver.com>

---
 drivers/char/keyboard.c |   10 +++---
 drivers/input/input.c   |   79 +++++++++++++++++++++++++++++++++++++++---------
 drivers/serial/kgdboc.c |   13 +++++++
 include/linux/input.h   |    7 ++++
 4 files changed, 90 insertions(+), 19 deletions(-)

--- a/drivers/char/keyboard.c
+++ b/drivers/char/keyboard.c
@@ -1195,6 +1195,11 @@ static void kbd_keycode(unsigned int key
 			if (keycode < BTN_MISC && printk_ratelimit())
 				printk(KERN_WARNING "keyboard.c: can't emulate rawmode for keycode %d\n", keycode);
 
+	if (down)
+		set_bit(keycode, key_down);
+	else
+		clear_bit(keycode, key_down);
+
 #ifdef CONFIG_MAGIC_SYSRQ	       /* Handle the SysRq Hack */
 	if (keycode == KEY_SYSRQ && (sysrq_down || (down == 1 && sysrq_alt))) {
 		if (!sysrq_down) {
@@ -1237,11 +1242,6 @@ static void kbd_keycode(unsigned int key
 		raw_mode = 1;
 	}
 
-	if (down)
-		set_bit(keycode, key_down);
-	else
-		clear_bit(keycode, key_down);
-
 	if (rep &&
 	    (!vc_kbd_mode(kbd, VC_REPEAT) ||
 	     (tty && !L_ECHO(tty) && tty_chars_in_buffer(tty)))) {
--- a/drivers/serial/kgdboc.c
+++ b/drivers/serial/kgdboc.c
@@ -17,6 +17,7 @@
 #include <linux/kdb.h>
 #include <linux/tty.h>
 #include <linux/console.h>
+#include <linux/input.h>
 
 #define MAX_CONFIG_LEN		40
 
@@ -35,12 +36,16 @@ static struct tty_driver	*kgdb_tty_drive
 static int			kgdb_tty_line;
 
 #ifdef CONFIG_KDB_KEYBOARD
+static int kgdboc_use_kbd;  /* 1 if we use a keyboard */
+
 static int kgdboc_register_kbd(char **cptr)
 {
+	kgdboc_use_kbd = 0;
 	if (strncmp(*cptr, "kbd", 3) == 0) {
 		if (kdb_poll_idx < KDB_POLL_FUNC_MAX) {
 			kdb_poll_funcs[kdb_poll_idx] = kdb_get_kbd_char;
 			kdb_poll_idx++;
+			kgdboc_use_kbd = 1;
 			if (cptr[0][3] == ',')
 				*cptr += 4;
 			else
@@ -63,9 +68,16 @@ static void kgdboc_unregister_kbd(void)
 		}
 	}
 }
+
+static inline void kgdboc_clear_kbd(void)
+{
+	if (kgdboc_use_kbd)
+		input_dbg_clear_keys(); /* Release all pressed keys */
+}
 #else /* ! CONFIG_KDB_KEYBOARD */
 #define kgdboc_register_kbd(x) 0
 #define kgdboc_unregister_kbd()
+#define kgdboc_clear_kbd()
 #endif /* ! CONFIG_KDB_KEYBOARD */
 
 static int kgdboc_option_setup(char *opt)
@@ -213,6 +225,7 @@ static void kgdboc_post_exp_handler(void
 	/* decrement the module count when the debugger detaches */
 	if (!kgdb_connected)
 		module_put(THIS_MODULE);
+	kgdboc_clear_kbd();
 }
 
 static struct kgdb_io kgdboc_io_ops = {
--- a/include/linux/input.h
+++ b/include/linux/input.h
@@ -1359,6 +1359,13 @@ int input_flush_device(struct input_hand
 void input_event(struct input_dev *dev, unsigned int type, unsigned int code, int value);
 void input_inject_event(struct input_handle *handle, unsigned int type, unsigned int code, int value);
 
+#ifdef CONFIG_KDB_KEYBOARD
+extern void input_dbg_clear_keys(void);
+#else
+static inline void input_dbg_clear_keys(void)
+{}
+#endif
+
 static inline void input_report_key(struct input_dev *dev, unsigned int code, int value)
 {
 	input_event(dev, EV_KEY, code, !!value);
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -24,6 +24,7 @@
 #include <linux/mutex.h>
 #include <linux/rcupdate.h>
 #include <linux/smp_lock.h>
+#include <linux/interrupt.h>
 #include "input-compat.h"
 
 MODULE_AUTHOR("Vojtech Pavlik <vojtech@suse.cz>");
@@ -511,12 +512,30 @@ void input_close_device(struct input_han
 EXPORT_SYMBOL(input_close_device);
 
 /*
+ * input_clear_keys - Simulate keyup events for all pressed keys so
+ * that handlers are not left with "stuck" keys.
+ */
+static void input_clear_keys(struct input_dev *dev)
+{
+	int code;
+
+	if (is_event_supported(EV_KEY, dev->evbit, EV_MAX)) {
+		for (code = 0; code <= KEY_MAX; code++) {
+			if (is_event_supported(code, dev->keybit, KEY_MAX) &&
+			    __test_and_clear_bit(code, dev->key)) {
+				input_pass_event(dev, EV_KEY, code, 0);
+			}
+		}
+		input_pass_event(dev, EV_SYN, SYN_REPORT, 1);
+	}
+}
+
+/*
  * Prepare device for unregistering
  */
 static void input_disconnect_device(struct input_dev *dev)
 {
 	struct input_handle *handle;
-	int code;
 
 	/*
 	 * Mark device as going away. Note that we take dev->mutex here
@@ -530,20 +549,10 @@ static void input_disconnect_device(stru
 	spin_lock_irq(&dev->event_lock);
 
 	/*
-	 * Simulate keyup events for all pressed keys so that handlers
-	 * are not left with "stuck" keys. The driver may continue
-	 * generate events even after we done here but they will not
-	 * reach any handlers.
+	 * The driver may continue generate events even after we done here
+	 * but they will not reach any handlers.
 	 */
-	if (is_event_supported(EV_KEY, dev->evbit, EV_MAX)) {
-		for (code = 0; code <= KEY_MAX; code++) {
-			if (is_event_supported(code, dev->keybit, KEY_MAX) &&
-			    __test_and_clear_bit(code, dev->key)) {
-				input_pass_event(dev, EV_KEY, code, 0);
-			}
-		}
-		input_pass_event(dev, EV_SYN, SYN_REPORT, 1);
-	}
+	input_clear_keys(dev);
 
 	list_for_each_entry(handle, &dev->h_list, d_node)
 		handle->open = 0;
@@ -1692,6 +1701,48 @@ int input_register_handler(struct input_
 }
 EXPORT_SYMBOL(input_register_handler);
 
+#ifdef CONFIG_KDB_KEYBOARD
+static bool input_dbg_keys_cleared;
+
+/*
+ * input_dbg_clear_keys - Clear any keyboards if they have a call back,
+ * after returning from the kernel debugger
+ */
+static void input_clear_keys_task(unsigned long not_used)
+{
+	struct input_dev *dev;
+	unsigned long flags;
+
+	if (!input_dbg_keys_cleared)
+		return;
+	list_for_each_entry(dev, &input_dev_list, node) {
+		spin_lock_irqsave(&dev->event_lock, flags);
+		input_clear_keys(dev);
+		spin_unlock_irqrestore(&dev->event_lock, flags);
+	}
+	input_dbg_keys_cleared = false;
+}
+
+static DECLARE_TASKLET(input_clear_keys_tasklet, input_clear_keys_task, 0);
+
+void input_dbg_clear_keys(void)
+{
+	struct input_dev *dev;
+
+	if (input_dbg_keys_cleared)
+		return;
+	input_dbg_keys_cleared = true;
+	/* Schedule a tasklet unless all locks are avaialble */
+	list_for_each_entry(dev, &input_dev_list, node)
+		if (spin_is_locked(&dev->event_lock)) {
+			tasklet_schedule(&input_clear_keys_tasklet);
+			return;
+		}
+	input_clear_keys_task(0);
+}
+EXPORT_SYMBOL_GPL(input_dbg_clear_keys);
+#endif /* CONFIG_KDB_KEYBOARD */
+
 /**
  * input_unregister_handler - unregisters an input handler
  * @handler: handler to be unregistered

------------------------------------------------------------------------------
Download Intel&#174; Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev

Re: [PATCH 23/28] keyboard, input: Add hook to input to allow low level event clear

[Dmitry Torokhov]

On Fri, Feb 26, 2010 at 10:06:19AM -0600, Jason Wessel wrote:
> Dmitry Torokhov wrote:
> > On Thu, Feb 25, 2010 at 03:21:28PM -0600, Jason Wessel wrote:
> >> When using a keyboard with kdb, on resuming the system there needs to
> >> be a hook to allow for the keyboard state to get reset.
> >>
> >> This is mainly because there is no way to force the end user to hold
> >> down the original keys that were pressed prior to entering kdb.
> >>
> > 
> > Instead of adding all the new hook can't you copy the bitmap of
> > currently pressed keys when you invoke kdb and theni, on exit, use
> > input_inject_event() to clear bitmasks in the devices?
> > 
> 
> I know just a little more about the input system then I did 6 months
> ago.  I am not sure that input_inject_event() is exactly what should
> be used, but perhaps you had a different idea in mind.
> 
> I created a new patch which uses the same sort of concept.  I moved
> the key release code from input_disconnect_device() into a common
> function, so that it could be called by the debugger key free hook.
> 

The problem with your patch is that you end up using input_pass_event()
which only passes events to handler, but it does not reset device state.
This will cause loss of the first press of the same button after
returning from kdb. input_inject_event() should do what you need. You
just need to do it from a tasklet or, better yet (there is no
performance issue) schedule a work on keventd so you don't deadlock
on the event lock. It will also do all necessary locking, which is
something you seem to be ignoring.

...

> 
> --- a/drivers/char/keyboard.c
> +++ b/drivers/char/keyboard.c
> @@ -1195,6 +1195,11 @@ static void kbd_keycode(unsigned int key
>  			if (keycode < BTN_MISC && printk_ratelimit())
>  				printk(KERN_WARNING "keyboard.c: can't emulate rawmode for keycode %d\n", keycode);
>  
> +	if (down)
> +		set_bit(keycode, key_down);
> +	else
> +		clear_bit(keycode, key_down);
> +

You sure it is not too early? Right now any key while in SysRq mode is
ignored, with your change it will affect the shift state without
actually passing the key press to userspace.

>  #ifdef CONFIG_MAGIC_SYSRQ	       /* Handle the SysRq Hack */
>  	if (keycode == KEY_SYSRQ && (sysrq_down || (down == 1 && sysrq_alt))) {
>  		if (!sysrq_down) {
> @@ -1237,11 +1242,6 @@ static void kbd_keycode(unsigned int key
>  		raw_mode = 1;
>  	}
>  
> -	if (down)
> -		set_bit(keycode, key_down);
> -	else
> -		clear_bit(keycode, key_down);
> -
>  	if (rep &&
>  	    (!vc_kbd_mode(kbd, VC_REPEAT) ||
>  	     (tty && !L_ECHO(tty) && tty_chars_in_buffer(tty)))) {
> --- a/drivers/serial/kgdboc.c
> +++ b/drivers/serial/kgdboc.c
> @@ -17,6 +17,7 @@
>  #include <linux/kdb.h>
>  #include <linux/tty.h>
>  #include <linux/console.h>
> +#include <linux/input.h>
>  
>  #define MAX_CONFIG_LEN		40
>  
> @@ -35,12 +36,16 @@ static struct tty_driver	*kgdb_tty_drive
>  static int			kgdb_tty_line;
>  
>  #ifdef CONFIG_KDB_KEYBOARD
> +static int kgdboc_use_kbd;  /* 1 if we use a keyboard */

bool?

> +
>  static int kgdboc_register_kbd(char **cptr)
>  {
> +	kgdboc_use_kbd = 0;
>  	if (strncmp(*cptr, "kbd", 3) == 0) {
>  		if (kdb_poll_idx < KDB_POLL_FUNC_MAX) {
>  			kdb_poll_funcs[kdb_poll_idx] = kdb_get_kbd_char;
>  			kdb_poll_idx++;

Hm, no locking here whatsoever?

> +			kgdboc_use_kbd = 1;
>  			if (cptr[0][3] == ',')
>  				*cptr += 4;
>  			else
> @@ -63,9 +68,16 @@ static void kgdboc_unregister_kbd(void)
>  		}
>  	}
>  }
> +
> +static inline void kgdboc_clear_kbd(void)
> +{
> +	if (kgdboc_use_kbd)
> +		input_dbg_clear_keys(); /* Release all pressed keys */

I'd rather have the input_dbg_clear_keys() being implemented right here,
along with the tasklet/work handling, instead of puttin it in the input
core.

-- 
Dmitry

Re: [PATCH 23/28] keyboard, input: Add hook to input to allow low level event clear

[Jason Wessel]

Dmitry Torokhov wrote:
> The problem with your patch is that you end up using input_pass_event()
> which only passes events to handler, but it does not reset device state.
> This will cause loss of the first press of the same button after
> returning from kdb. input_inject_event() should do what you need. You
> just need to do it from a tasklet or, better yet (there is no
> performance issue) schedule a work on keventd so you don't deadlock
> on the event lock. It will also do all necessary locking, which is
> something you seem to be ignoring.
>
> ...
>
>   

For now I am just going to drop this patch from the series, and I think
we should pick up the continuation of the discussion on the
linux-input.  The lack of this patch simply means keys can stuck down
sometimes if you go in and out of the kernel debug shell when running
X.   People were able to live with this a long time in the original kdb
v4.4, so they can live with it a while longer while a solution is hashed
out to "unstick the keys".

This will allow me to re-issue a pull request which has no patches which
are in the discussion state.

It is not obvious to me how walk through the input device list with the
right locks and then obtain the handle required to call the inject
input.  It does not appear that it is possible to hold the locks while
walking through the keycode device bitmap and issue calls to inject
input. Is there any kind of input API function to obtain the device bitmaps?

>> --- a/drivers/char/keyboard.c
>> +++ b/drivers/char/keyboard.c
>> @@ -1195,6 +1195,11 @@ static void kbd_keycode(unsigned int key
>>  			if (keycode < BTN_MISC && printk_ratelimit())
>>  				printk(KERN_WARNING "keyboard.c: can't emulate rawmode for keycode %d\n", keycode);
>>  
>> +	if (down)
>> +		set_bit(keycode, key_down);
>> +	else
>> +		clear_bit(keycode, key_down);
>> +
>>     
>
> You sure it is not too early? Right now any key while in SysRq mode is
> ignored, with your change it will affect the shift state without
> actually passing the key press to userspace.
>
>   

I think it is possible to drop this chunk and leave it as it was because
the work is definitely going to move to a tasklet which occurs after the
sysrq and the key events will be synced at that point.

Previously the problem was trying to process the key bitmaps inside the
sysrq.

>> --- a/drivers/serial/kgdboc.c
>> +++ b/drivers/serial/kgdboc.c
>> @@ -17,6 +17,7 @@
>>  #include <linux/kdb.h>
>>  #include <linux/tty.h>
>>  #include <linux/console.h>
>> +#include <linux/input.h>
>>  
>>  #define MAX_CONFIG_LEN		40
>>  
>> @@ -35,12 +36,16 @@ static struct tty_driver	*kgdb_tty_drive
>>  static int			kgdb_tty_line;
>>  
>>  #ifdef CONFIG_KDB_KEYBOARD
>> +static int kgdboc_use_kbd;  /* 1 if we use a keyboard */
>>     
>
> bool?
>
>   

Agreed, I'll fix that in a future iteration.
>   
>> +
>>  static int kgdboc_register_kbd(char **cptr)
>>  {
>> +	kgdboc_use_kbd = 0;
>>  	if (strncmp(*cptr, "kbd", 3) == 0) {
>>  		if (kdb_poll_idx < KDB_POLL_FUNC_MAX) {
>>  			kdb_poll_funcs[kdb_poll_idx] = kdb_get_kbd_char;
>>  			kdb_poll_idx++;
>>     
>
> Hm, no locking here whatsoever?
>
>   

There is no locking needed here.  This only takes place at the kernel
module configuration.

>   
>> @@ -63,9 +68,16 @@ static void kgdboc_unregister_kbd(void)
>>  		}
>>  	}
>>  }
>> +
>> +static inline void kgdboc_clear_kbd(void)
>> +{
>> +	if (kgdboc_use_kbd)
>> +		input_dbg_clear_keys(); /* Release all pressed keys */
>>     
>
> I'd rather have the input_dbg_clear_keys() being implemented right here,
> along with the tasklet/work handling, instead of puttin it in the input
> core.
>
>   


That is fine with me, it is just not obvious to me which API calls you
need in order to be able iterate through the key bit maps and get
handles to call the inject input event.

Jason

------------------------------------------------------------------------------
Download Intel&#174; Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev

Re: [PATCH 23/28] keyboard, input: Add hook to input to allow low level event clear

[Dmitry Torokhov]

On Sun, Feb 28, 2010 at 09:56:56PM -0600, Jason Wessel wrote:
> Dmitry Torokhov wrote:
> > The problem with your patch is that you end up using input_pass_event()
> > which only passes events to handler, but it does not reset device state.
> > This will cause loss of the first press of the same button after
> > returning from kdb. input_inject_event() should do what you need. You
> > just need to do it from a tasklet or, better yet (there is no
> > performance issue) schedule a work on keventd so you don't deadlock
> > on the event lock. It will also do all necessary locking, which is
> > something you seem to be ignoring.
> >
> > ...
> >
> >   
> 
> For now I am just going to drop this patch from the series, and I think
> we should pick up the continuation of the discussion on the
> linux-input.  The lack of this patch simply means keys can stuck down
> sometimes if you go in and out of the kernel debug shell when running
> X.   People were able to live with this a long time in the original kdb
> v4.4, so they can live with it a while longer while a solution is hashed
> out to "unstick the keys".
> 
> This will allow me to re-issue a pull request which has no patches which
> are in the discussion state.
> 

OK.

> It is not obvious to me how walk through the input device list with the
> right locks and then obtain the handle required to call the inject
> input.  It does not appear that it is possible to hold the locks while
> walking through the keycode device bitmap and issue calls to inject
> input. Is there any kind of input API function to obtain the device bitmaps?
> 

There is key_down bitmap right there in keyboard.c that reflects all
keys that are considered "down" as far as keyboard driver is concerned.
You can use input_handler_for_each_handle() for kbd_handler to iterate
through all input handles and send "up" events for all keys that
keyboard.c is considering as being "down". There is no harm in sending
extra "up" events for keys that are not pressed - the event will simply
be ignored by input core.

Hope this helps.

-- 
Dmitry

------------------------------------------------------------------------------
Download Intel® Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev

Re: [PATCH 23/28] keyboard, input: Add hook to input to allow low level event clear

[Jason Wessel]

Dmitry Torokhov wrote:
> On Sun, Feb 28, 2010 at 09:56:56PM -0600, Jason Wessel wrote:
> 
>> It is not obvious to me how walk through the input device list with the
>> right locks and then obtain the handle required to call the inject
>> input.  It does not appear that it is possible to hold the locks while
>> walking through the keycode device bitmap and issue calls to inject
>> input. Is there any kind of input API function to obtain the device bitmaps?
>>
> 
> There is key_down bitmap right there in keyboard.c that reflects all
> keys that are considered "down" as far as keyboard driver is concerned.
> You can use input_handler_for_each_handle() for kbd_handler to iterate
> through all input handles and send "up" events for all keys that
> keyboard.c is considering as being "down". There is no harm in sending
> extra "up" events for keys that are not pressed - the event will simply
> be ignored by input core.
> 

Thanks for the explanation.

Below is the new version.  You just have to decide if you want to
expose the keyboard bitmap as a global, or use an accessors function
inside keyboard.c.

In X, it seems there is an unbalenced key up / down because
emulate_raw() puts the keys in, but the keyboard bit map is not
updated.  I worked around it by adding in a keyboard bitmap update
prior to invoking the sysrq, so the kdb key free can free all the keys
later.

I am not sure if there is a better way to fix the bitmap update.  I
did try moving the emulate_raw() around in a few combinations, but
nothing would yield the correct results for still allowing the
alt-print-screen work with the window capture in the window manager.

Thanks,
Jason.

---
From: Jason Wessel <jason.wessel@windriver.com>
Subject: [PATCH] keyboard,kgdboc: Allow key release on kernel resume

When using a keyboard with kdb, a hook point to free all the
keystrokes is required for resuming kernel operations.

This is mainly because there is no way to force the end user to hold
down the original keys that were pressed prior to entering kdb when
resuming the kernel.

The kgdboc driver will call kdb_clear_keys() just prior to resuming
the kernel execution which will create a tasklet to run
kbd_dbg_clear_keys() inside the keyboard.c.

CC: Dmitry Torokhov <dmitry.torokhov@gmail.com>
CC: Greg Kroah-Hartman <gregkh@suse.de>
CC: linux-input@vger.kernel.org
Signed-off-by: Jason Wessel <jason.wessel@windriver.com>

---
 drivers/char/keyboard.c         |   27 +++++++++++++++++++++++++++
 drivers/serial/kgdboc.c         |   13 +++++++++++++
 include/linux/kbd_kern.h        |    1 +
 include/linux/kdb.h             |    3 +++
 kernel/debug/kdb/kdb_keyboard.c |   25 +++++++++++++++++++++++++
 5 files changed, 69 insertions(+)

--- a/drivers/char/keyboard.c
+++ b/drivers/char/keyboard.c
@@ -379,6 +379,32 @@ static void to_utf8(struct vc_data *vc, 
 	}
 }
 
+static int kbd_dbg_clear_keys_helper(struct input_handle *handle, void *data)
+{
+	unsigned int *keycode = data;
+	input_inject_event(handle, EV_KEY, *keycode, 0);
+	return 0;
+}
+
+/* Called to clear the any key presses after resuming the kernel. */
+void kbd_dbg_clear_keys(void) {
+	unsigned int i, j, k;
+
+	for (i = 0; i < ARRAY_SIZE(key_down); i++) {
+		if (!key_down[i])
+			continue;
+
+		k = i * BITS_PER_LONG;
+
+		for (j = 0; j < BITS_PER_LONG; j++, k++) {
+			if (!test_bit(k, key_down))
+				continue;
+			input_handler_for_each_handle(&kbd_handler, &k,
+				      kbd_dbg_clear_keys_helper);
+		}
+	}
+}
+
 /*
  * Called after returning from RAW mode or when changing consoles - recompute
  * shift_down[] and shift_state from key_down[] maybe called when keymap is
@@ -1206,6 +1232,7 @@ static void kbd_keycode(unsigned int key
 	if (sysrq_down && !down && keycode == sysrq_alt_use)
 		sysrq_down = 0;
 	if (sysrq_down && down && !rep) {
+		set_bit(keycode, key_down);
 		handle_sysrq(kbd_sysrq_xlate[keycode], tty);
 		return;
 	}
--- a/drivers/serial/kgdboc.c
+++ b/drivers/serial/kgdboc.c
@@ -17,6 +17,7 @@
 #include <linux/kdb.h>
 #include <linux/tty.h>
 #include <linux/console.h>
+#include <linux/kbd_kern.h>
 
 #define MAX_CONFIG_LEN		40
 
@@ -35,12 +36,16 @@ static struct tty_driver	*kgdb_tty_drive
 static int			kgdb_tty_line;
 
 #ifdef CONFIG_KDB_KEYBOARD
+static bool			kgdboc_use_kbd;
+
 static int kgdboc_register_kbd(char **cptr)
 {
+	kgdboc_use_kbd = false;
 	if (strncmp(*cptr, "kbd", 3) == 0) {
 		if (kdb_poll_idx < KDB_POLL_FUNC_MAX) {
 			kdb_poll_funcs[kdb_poll_idx] = kdb_get_kbd_char;
 			kdb_poll_idx++;
+			kgdboc_use_kbd = true;
 			if (cptr[0][3] == ',')
 				*cptr += 4;
 			else
@@ -63,9 +68,16 @@ static void kgdboc_unregister_kbd(void)
 		}
 	}
 }
+
+static inline void kgdboc_clear_kbd(void)
+{
+	if (kgdboc_use_kbd)
+		kdb_clear_keys(); /* Release all pressed keys */
+}
 #else /* ! CONFIG_KDB_KEYBOARD */
 #define kgdboc_register_kbd(x) 0
 #define kgdboc_unregister_kbd()
+#define kgdboc_clear_kbd()
 #endif /* ! CONFIG_KDB_KEYBOARD */
 
 static int kgdboc_option_setup(char *opt)
@@ -213,6 +225,7 @@ static void kgdboc_post_exp_handler(void
 	/* decrement the module count when the debugger detaches */
 	if (!kgdb_connected)
 		module_put(THIS_MODULE);
+	kgdboc_clear_kbd();
 }
 
 static struct kgdb_io kgdboc_io_ops = {
--- a/include/linux/kbd_kern.h
+++ b/include/linux/kbd_kern.h
@@ -144,6 +144,7 @@ struct console;
 int getkeycode(unsigned int scancode);
 int setkeycode(unsigned int scancode, unsigned int keycode);
 void compute_shiftstate(void);
+void kbd_dbg_clear_keys(void);
 
 /* defkeymap.c */
 
--- a/include/linux/kdb.h
+++ b/include/linux/kdb.h
@@ -93,6 +93,9 @@ typedef int (*get_char_func)(void);
 extern get_char_func kdb_poll_funcs[];
 extern int kdb_get_kbd_char(void);
 
+/* KDB keyboard hooks */
+extern void kdb_clear_keys(void);
+
 static inline
 int kdb_process_cpu(const struct task_struct *p)
 {
--- a/kernel/debug/kdb/kdb_keyboard.c
+++ b/kernel/debug/kdb/kdb_keyboard.c
@@ -13,6 +13,7 @@
 #include <linux/ctype.h>
 #include <linux/module.h>
 #include <linux/io.h>
+#include <linux/kbd_kern.h>
 
 /* Keyboard Controller Registers on normal PCs. */
 
@@ -210,3 +211,27 @@ int kdb_get_kbd_char(void)
 	return keychar & 0xff;
 }
 EXPORT_SYMBOL_GPL(kdb_get_kbd_char);
+
+static bool dbg_keys_cleared;
+/*
+ * input_dbg_clear_keys - Clear any keyboards if they have a call back,
+ * after returning from the kernel debugger
+ */
+static void kdb_keys_task(unsigned long not_used)
+{
+	if (!dbg_keys_cleared)
+		return;
+	kbd_dbg_clear_keys();
+	dbg_keys_cleared = false;
+}
+
+static DECLARE_TASKLET(kdb_keys_tasklet, kdb_keys_task, 0);
+
+void kdb_clear_keys(void)
+{
+	if (dbg_keys_cleared)
+		return;
+	dbg_keys_cleared = true;
+	tasklet_schedule(&kdb_keys_tasklet);
+}
+EXPORT_SYMBOL_GPL(kdb_clear_keys);

------------------------------------------------------------------------------
Download Intel&#174; Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev

Re: [PATCH 23/28] keyboard, input: Add hook to input to allow low level event clear

[Dmitry Torokhov]

On Mon, Mar 01, 2010 at 10:49:33AM -0600, Jason Wessel wrote:
> Dmitry Torokhov wrote:
> > On Sun, Feb 28, 2010 at 09:56:56PM -0600, Jason Wessel wrote:
> > 
> >> It is not obvious to me how walk through the input device list with the
> >> right locks and then obtain the handle required to call the inject
> >> input.  It does not appear that it is possible to hold the locks while
> >> walking through the keycode device bitmap and issue calls to inject
> >> input. Is there any kind of input API function to obtain the device bitmaps?
> >>
> > 
> > There is key_down bitmap right there in keyboard.c that reflects all
> > keys that are considered "down" as far as keyboard driver is concerned.
> > You can use input_handler_for_each_handle() for kbd_handler to iterate
> > through all input handles and send "up" events for all keys that
> > keyboard.c is considering as being "down". There is no harm in sending
> > extra "up" events for keys that are not pressed - the event will simply
> > be ignored by input core.
> > 
> 
> Thanks for the explanation.
> 
> Below is the new version.  You just have to decide if you want to
> expose the keyboard bitmap as a global, or use an accessors function
> inside keyboard.c.

Looks much better, thanks. I do prefer accessor functions to exporintg
variables.

> 
> In X, it seems there is an unbalenced key up / down because
> emulate_raw() puts the keys in, but the keyboard bit map is not
> updated.

Yes, if keyboard in raw mode (and it is while you are in X), then most
of the keyboard.c code is not active.

>  I worked around it by adding in a keyboard bitmap update
> prior to invoking the sysrq, so the kdb key free can free all the keys
> later.
> 
> I am not sure if there is a better way to fix the bitmap update.  I
> did try moving the emulate_raw() around in a few combinations, but
> nothing would yield the correct results for still allowing the
> alt-print-screen work with the window capture in the window manager.
> 

Maybe we should simply add key_raw_down bitmap? It is not expensive and
would untangle the shiftstate computation in case of cooked keyboard
from what you need for kdb.

Another option would be writing your own tiny input handler that would
attach to all keyboards like keyboard.c does and just maintain it's own
bitmap of currently active keys. It would work regardless of what mode
keyboard.c is (raw, mediumraw, etc). Actually, I like this idea a lot,
it nicely decouples kdb from keyboard driver. For an example of really
simple handler take a look at drivers/input/apm-power.c

> Thanks,
> Jason.
> 
> ---
> From: Jason Wessel <jason.wessel@windriver.com>
> Subject: [PATCH] keyboard,kgdboc: Allow key release on kernel resume
> 
> When using a keyboard with kdb, a hook point to free all the
> keystrokes is required for resuming kernel operations.
> 
> This is mainly because there is no way to force the end user to hold
> down the original keys that were pressed prior to entering kdb when
> resuming the kernel.
> 
> The kgdboc driver will call kdb_clear_keys() just prior to resuming
> the kernel execution which will create a tasklet to run
> kbd_dbg_clear_keys() inside the keyboard.c.
> 
> CC: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> CC: Greg Kroah-Hartman <gregkh@suse.de>
> CC: linux-input@vger.kernel.org
> Signed-off-by: Jason Wessel <jason.wessel@windriver.com>
> 
> ---
>  drivers/char/keyboard.c         |   27 +++++++++++++++++++++++++++
>  drivers/serial/kgdboc.c         |   13 +++++++++++++
>  include/linux/kbd_kern.h        |    1 +
>  include/linux/kdb.h             |    3 +++
>  kernel/debug/kdb/kdb_keyboard.c |   25 +++++++++++++++++++++++++
>  5 files changed, 69 insertions(+)
> 
> --- a/drivers/char/keyboard.c
> +++ b/drivers/char/keyboard.c
> @@ -379,6 +379,32 @@ static void to_utf8(struct vc_data *vc, 
>  	}
>  }
>  
> +static int kbd_dbg_clear_keys_helper(struct input_handle *handle, void *data)
> +{
> +	unsigned int *keycode = data;
> +	input_inject_event(handle, EV_KEY, *keycode, 0);
> +	return 0;
> +}
> +
> +/* Called to clear the any key presses after resuming the kernel. */
> +void kbd_dbg_clear_keys(void) {

Style - function opening brace on a separate line please.

> +	unsigned int i, j, k;
> +
> +	for (i = 0; i < ARRAY_SIZE(key_down); i++) {
> +		if (!key_down[i])
> +			continue;
> +
> +		k = i * BITS_PER_LONG;
> +
> +		for (j = 0; j < BITS_PER_LONG; j++, k++) {
> +			if (!test_bit(k, key_down))
> +				continue;
> +			input_handler_for_each_handle(&kbd_handler, &k,
> +				      kbd_dbg_clear_keys_helper);
> +		}
> +	}
> +}
> +
>  /*
>   * Called after returning from RAW mode or when changing consoles - recompute
>   * shift_down[] and shift_state from key_down[] maybe called when keymap is
> @@ -1206,6 +1232,7 @@ static void kbd_keycode(unsigned int key
>  	if (sysrq_down && !down && keycode == sysrq_alt_use)
>  		sysrq_down = 0;
>  	if (sysrq_down && down && !rep) {
> +		set_bit(keycode, key_down);
>  		handle_sysrq(kbd_sysrq_xlate[keycode], tty);
>  		return;
>  	}
> --- a/drivers/serial/kgdboc.c
> +++ b/drivers/serial/kgdboc.c
> @@ -17,6 +17,7 @@
>  #include <linux/kdb.h>
>  #include <linux/tty.h>
>  #include <linux/console.h>
> +#include <linux/kbd_kern.h>
>  
>  #define MAX_CONFIG_LEN		40
>  
> @@ -35,12 +36,16 @@ static struct tty_driver	*kgdb_tty_drive
>  static int			kgdb_tty_line;
>  
>  #ifdef CONFIG_KDB_KEYBOARD
> +static bool			kgdboc_use_kbd;
> +
>  static int kgdboc_register_kbd(char **cptr)
>  {
> +	kgdboc_use_kbd = false;
>  	if (strncmp(*cptr, "kbd", 3) == 0) {
>  		if (kdb_poll_idx < KDB_POLL_FUNC_MAX) {
>  			kdb_poll_funcs[kdb_poll_idx] = kdb_get_kbd_char;
>  			kdb_poll_idx++;
> +			kgdboc_use_kbd = true;
>  			if (cptr[0][3] == ',')
>  				*cptr += 4;
>  			else
> @@ -63,9 +68,16 @@ static void kgdboc_unregister_kbd(void)
>  		}
>  	}
>  }
> +
> +static inline void kgdboc_clear_kbd(void)
> +{
> +	if (kgdboc_use_kbd)
> +		kdb_clear_keys(); /* Release all pressed keys */
> +}
>  #else /* ! CONFIG_KDB_KEYBOARD */
>  #define kgdboc_register_kbd(x) 0
>  #define kgdboc_unregister_kbd()
> +#define kgdboc_clear_kbd()
>  #endif /* ! CONFIG_KDB_KEYBOARD */
>  
>  static int kgdboc_option_setup(char *opt)
> @@ -213,6 +225,7 @@ static void kgdboc_post_exp_handler(void
>  	/* decrement the module count when the debugger detaches */
>  	if (!kgdb_connected)
>  		module_put(THIS_MODULE);
> +	kgdboc_clear_kbd();
>  }
>  
>  static struct kgdb_io kgdboc_io_ops = {
> --- a/include/linux/kbd_kern.h
> +++ b/include/linux/kbd_kern.h
> @@ -144,6 +144,7 @@ struct console;
>  int getkeycode(unsigned int scancode);
>  int setkeycode(unsigned int scancode, unsigned int keycode);
>  void compute_shiftstate(void);
> +void kbd_dbg_clear_keys(void);
>  
>  /* defkeymap.c */
>  
> --- a/include/linux/kdb.h
> +++ b/include/linux/kdb.h
> @@ -93,6 +93,9 @@ typedef int (*get_char_func)(void);
>  extern get_char_func kdb_poll_funcs[];
>  extern int kdb_get_kbd_char(void);
>  
> +/* KDB keyboard hooks */
> +extern void kdb_clear_keys(void);
> +
>  static inline
>  int kdb_process_cpu(const struct task_struct *p)
>  {
> --- a/kernel/debug/kdb/kdb_keyboard.c
> +++ b/kernel/debug/kdb/kdb_keyboard.c
> @@ -13,6 +13,7 @@
>  #include <linux/ctype.h>
>  #include <linux/module.h>
>  #include <linux/io.h>
> +#include <linux/kbd_kern.h>
>  
>  /* Keyboard Controller Registers on normal PCs. */
>  
> @@ -210,3 +211,27 @@ int kdb_get_kbd_char(void)
>  	return keychar & 0xff;
>  }
>  EXPORT_SYMBOL_GPL(kdb_get_kbd_char);
> +
> +static bool dbg_keys_cleared;
> +/*
> + * input_dbg_clear_keys - Clear any keyboards if they have a call back,
> + * after returning from the kernel debugger
> + */
> +static void kdb_keys_task(unsigned long not_used)
> +{
> +	if (!dbg_keys_cleared)
> +		return;
> +	kbd_dbg_clear_keys();
> +	dbg_keys_cleared = false;
> +}
> +
> +static DECLARE_TASKLET(kdb_keys_tasklet, kdb_keys_task, 0);

Do you really need tasklet? Would not regular work_struct do? Also, the
logic of dbg_keys_cleared seems to be inverted, not metioned the fact
that it does not seem to be needed anymore. If you decide against a new
input handler approach may I ask you to move the scheduling to
keyboard.c as well - I know I originally said I wanted it in kdb but now
that I thought about it some more I think keyboard.c is better.

> +
> +void kdb_clear_keys(void)
> +{
> +	if (dbg_keys_cleared)
> +		return;
> +	dbg_keys_cleared = true;
> +	tasklet_schedule(&kdb_keys_tasklet);
> +}
> +EXPORT_SYMBOL_GPL(kdb_clear_keys);

Thanks.

-- 
Dmitry

Re: [PATCH 23/28] keyboard, input: Add hook to input to allow low level event clear

[Jason Wessel]

Dmitry Torokhov wrote:
> On Mon, Mar 01, 2010 at 10:49:33AM -0600, Jason Wessel wrote:
>   
>> Dmitry Torokhov wrote:
>>     
>>> On Sun, Feb 28, 2010 at 09:56:56PM -0600, Jason Wessel wrote:
>>>
>>>       
>>>> It is not obvious to me how walk through the input device list with the
>>>> right locks and then obtain the handle required to call the inject
>>>> input.  It does not appear that it is possible to hold the locks while
>>>> walking through the keycode device bitmap and issue calls to inject
>>>> input. Is there any kind of input API function to obtain the device bitmaps?
>>>>
>>>>         
>>> There is key_down bitmap right there in keyboard.c that reflects all
>>> keys that are considered "down" as far as keyboard driver is concerned.
>>> You can use input_handler_for_each_handle() for kbd_handler to iterate
>>> through all input handles and send "up" events for all keys that
>>> keyboard.c is considering as being "down". There is no harm in sending
>>> extra "up" events for keys that are not pressed - the event will simply
>>> be ignored by input core.
>>>
>>>       
>> Thanks for the explanation.
>>
>> Below is the new version.  You just have to decide if you want to
>> expose the keyboard bitmap as a global, or use an accessors function
>> inside keyboard.c.
>>     
>
> Looks much better, thanks. I do prefer accessor functions to exporintg
> variables.
>
>   
>> In X, it seems there is an unbalenced key up / down because
>> emulate_raw() puts the keys in, but the keyboard bit map is not
>> updated.
>>     
>
> Yes, if keyboard in raw mode (and it is while you are in X), then most
> of the keyboard.c code is not active.
>
>   
>>  I worked around it by adding in a keyboard bitmap update
>> prior to invoking the sysrq, so the kdb key free can free all the keys
>> later.
>>
>> I am not sure if there is a better way to fix the bitmap update.  I
>> did try moving the emulate_raw() around in a few combinations, but
>> nothing would yield the correct results for still allowing the
>> alt-print-screen work with the window capture in the window manager.
>>
>>     
>
> Maybe we should simply add key_raw_down bitmap? It is not expensive and
> would untangle the shiftstate computation in case of cooked keyboard
> from what you need for kdb.
>
> Another option would be writing your own tiny input handler that would
> attach to all keyboards like keyboard.c does and just maintain it's own
> bitmap of currently active keys. It would work regardless of what mode
> keyboard.c is (raw, mediumraw, etc). Actually, I like this idea a lot,
> it nicely decouples kdb from keyboard driver. 

Unfortunately the keyboard driver is what has the problem of its state
getting out of sync when the kernel makes use of the kdb_keyboard.c,
which is why I was trying to hook it in the first place.  If the sysrq
code could be moved to its own input driver does this problem go away
for all parties?

Even without kdb, when running X, the sysrq definitely does not behave
so well and a sysrq-KEY_PRESS is passed on to the input queue instead of
getting eaten.  For example when I do alt-sysrq-h, I see the help menu
flash in the terminal because it thinks I pressed alt-h and kept it held
down.

>> +static void kdb_keys_task(unsigned long not_used)
>> +{
>> +	if (!dbg_keys_cleared)
>> +		return;
>> +	kbd_dbg_clear_keys();
>> +	dbg_keys_cleared = false;
>> +}
>> +
>> +static DECLARE_TASKLET(kdb_keys_tasklet, kdb_keys_task, 0);
>>     
>
> Do you really need tasklet? Would not regular work_struct do? 

It was safe to schedule a tasklet from the caller context.  I assume it
is probably also safe to use a work_struct, but I had not checked.

> Also, the
> logic of dbg_keys_cleared seems to be inverted, not metioned the fact
> that it does not seem to be needed anymore. 

That is a result of iteration churn.  Easy enough to change.

> If you decide against a new
> input handler approach may I ask you to move the scheduling to
> keyboard.c as well - I know I originally said I wanted it in kdb but now
> that I thought about it some more I think keyboard.c is better.
>
>   

I can move this back into the keyboard.c.

As I mentioned before, I am not sure an input handler will solve the
higher level problem, that even beyond kdb, keys are getting stuck with
X and sysrq.  Perhaps we need a generic solution that is not even kdb
specific for clearing the state after a sysrq?

For kdb it does still need the same sort of hook because if you type in:

  echo g > /proc/sysrq-trigger

And hit enter, the enter key will get repeated over and over because it
happened to be depressed when the kernel debugger was entered, while in X.

This means we have two cases:

1) Cleanup key strokes or "consume keys" for sysrq
2) Clear keyboard state on a kernel resume from kdb

I am open to any reasonable solution.  The most recent patch I sent
works well for case 2, but never really addressed case 1, which has been
a problem with or without kdb.

Jason.

Jason.

------------------------------------------------------------------------------
Download Intel® Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev

Re: [PATCH 23/28] keyboard, input: Add hook to input to allow low level event clear

[Jason Wessel]

Jason Wessel wrote:
> This means we have two cases:
> 
> 1) Cleanup key strokes or "consume keys" for sysrq
> 2) Clear keyboard state on a kernel resume from kdb
> 


Because we have two separate problems, I created two separate patches
which I inlined below.

The first patch really has nothing to do with kdb.  It is a generic
problem that has existed for sometime.  I find it annoying that you
cannot execute a sysrq without trigging the window capture in gnome
because it treats alt-sysrq like alt-PrintScreen.  The testing
I completed shows that this is no long a problem with this patch.

The second patch is a follow on to the original discussion we
were having with respect to releasing all the depressed keys
when resuming the kernel and modified per your recent comments.

Thanks,
Jason


----------Patch 1--------------
From: Jason Wessel <jason.wessel@windriver.com> Subject: [PATCH]
keyboard: Change alt-sysrq to always discard key events

When issuing an alt-sysrq-KEY sequence with the keyboard driver in the
raw mode some unbalanced key down and up events can get propagated to
the registered input handlers.  This leads to strange behavior
particularly when running an X server.

If handle_sysrq() is called while executing an alt-sysrq-KEY_PRESS,
all the key events should get discarded related to the sysrq
activation sequence.

When handle_sysrq() is not called and the keyboard driver is in the
raw mode, a down and up sysrq keycode will get passed to the
registered input handlers.

CC: Dmitry Torokhov <dmitry.torokhov@gmail.com>
CC: Greg Kroah-Hartman <gregkh@suse.de>
CC: linux-input@vger.kernel.org
Signed-off-by: Jason Wessel <jason.wessel@windriver.com>

---
 drivers/char/keyboard.c |   31 ++++++++++++++++++++++++-------
 1 file changed, 24 insertions(+), 7 deletions(-)

--- a/drivers/char/keyboard.c
+++ b/drivers/char/keyboard.c
@@ -160,6 +160,7 @@ unsigned char kbd_sysrq_xlate[KEY_MAX + 
         "\r\000/";                                      /* 0x60 - 0x6f */
 static int sysrq_down;
 static int sysrq_alt_use;
+static int sysrq_sent;
 #endif
 static int sysrq_alt;
 
@@ -1158,6 +1159,13 @@ static void kbd_rawcode(unsigned char da
 		put_queue(vc, data);
 }
 
+#define emulate_raw_keycode(kc, dwn) \
+	if (raw_mode && !hw_raw) \
+		if (emulate_raw(vc, kc, !dwn << 7)) \
+			if (keycode < BTN_MISC && printk_ratelimit()) \
+				printk(KERN_WARNING "keyboard.c: can't emulate rawmode for keycode %d\n", kc)
+
+
 static void kbd_keycode(unsigned int keycode, int down, int hw_raw)
 {
 	struct vc_data *vc = vc_cons[fg_console].d;
@@ -1185,26 +1193,35 @@ static void kbd_keycode(unsigned int key
 
 	rep = (down == 2);
 
-	if ((raw_mode = (kbd->kbdmode == VC_RAW)) && !hw_raw)
-		if (emulate_raw(vc, keycode, !down << 7))
-			if (keycode < BTN_MISC && printk_ratelimit())
-				printk(KERN_WARNING "keyboard.c: can't emulate rawmode for keycode %d\n", keycode);
+	raw_mode = kbd->kbdmode == VC_RAW;
 
 #ifdef CONFIG_MAGIC_SYSRQ	       /* Handle the SysRq Hack */
 	if (keycode == KEY_SYSRQ && (sysrq_down || (down == 1 && sysrq_alt))) {
 		if (!sysrq_down) {
 			sysrq_down = down;
 			sysrq_alt_use = sysrq_alt;
+			sysrq_sent = 0;
 		}
 		return;
 	}
-	if (sysrq_down && !down && keycode == sysrq_alt_use)
+	if (sysrq_down && !down && keycode == sysrq_alt_use) {
 		sysrq_down = 0;
-	if (sysrq_down && down && !rep) {
-		handle_sysrq(kbd_sysrq_xlate[keycode], tty);
+		if (!sysrq_sent) {
+			emulate_raw_keycode(KEY_SYSRQ, 1);
+			emulate_raw_keycode(KEY_SYSRQ, 0);
+		}
+	}
+	if (sysrq_down && !rep) {
+		if (down) {
+			handle_sysrq(kbd_sysrq_xlate[keycode], tty);
+			sysrq_sent = keycode;
+		}
 		return;
 	}
 #endif
+
+	emulate_raw_keycode(keycode, down);
+
 #ifdef CONFIG_SPARC
 	if (keycode == KEY_A && sparc_l1_a_state) {
 		sparc_l1_a_state = 0;







----------Patch 2--------------
From: Jason Wessel <jason.wessel@windriver.com>
Subject: [PATCH] keyboard,kgdboc: Allow key release on kernel resume

When using a keyboard with kdb, a hook point to free all the
keystrokes is required for resuming kernel operations.

This is mainly because there is no way to force the end user to hold
down the original keys that were pressed prior to entering kdb when
resuming the kernel.

The kgdboc driver will call kbd_dbg_clear_keys() just prior to
resuming the kernel execution which will schedule a callback to clear
any keys which were depressed prior to the entering the kernel
debugger.

CC: Dmitry Torokhov <dmitry.torokhov@gmail.com>
CC: Greg Kroah-Hartman <gregkh@suse.de>
CC: linux-input@vger.kernel.org
Signed-off-by: Jason Wessel <jason.wessel@windriver.com>

---
 drivers/char/keyboard.c  |   37 +++++++++++++++++++++++++++++++++++++
 drivers/serial/kgdboc.c  |   13 +++++++++++++
 include/linux/kbd_kern.h |    1 +
 3 files changed, 51 insertions(+)

--- a/drivers/char/keyboard.c
+++ b/drivers/char/keyboard.c
@@ -380,6 +380,43 @@ static void to_utf8(struct vc_data *vc, 
 	}
 }
 
+#ifdef CONFIG_KDB_KEYBOARD
+static int kbd_clear_keys_helper(struct input_handle *handle, void *data)
+{
+	unsigned int *keycode = data;
+	input_inject_event(handle, EV_KEY, *keycode, 0);
+	return 0;
+}
+
+static void kbd_clear_keys_callback(struct work_struct *dummy)
+{
+	unsigned int i, j, k;
+
+	for (i = 0; i < ARRAY_SIZE(key_down); i++) {
+		if (!key_down[i])
+			continue;
+
+		k = i * BITS_PER_LONG;
+
+		for (j = 0; j < BITS_PER_LONG; j++, k++) {
+			if (!test_bit(k, key_down))
+				continue;
+			input_handler_for_each_handle(&kbd_handler, &k,
+				      kbd_clear_keys_helper);
+		}
+	}
+}
+
+static DECLARE_WORK(kbd_clear_keys_work, kbd_clear_keys_callback);
+
+/* Called to clear any key presses after resuming the kernel. */
+void kbd_dbg_clear_keys(void)
+{
+	schedule_work(&kbd_clear_keys_work);
+}
+EXPORT_SYMBOL_GPL(kbd_dbg_clear_keys);
+#endif /* CONFIG_KDB_KEYBOARD */
+
 /*
  * Called after returning from RAW mode or when changing consoles - recompute
  * shift_down[] and shift_state from key_down[] maybe called when keymap is
--- a/drivers/serial/kgdboc.c
+++ b/drivers/serial/kgdboc.c
@@ -17,6 +17,7 @@
 #include <linux/kdb.h>
 #include <linux/tty.h>
 #include <linux/console.h>
+#include <linux/kbd_kern.h>
 
 #define MAX_CONFIG_LEN		40
 
@@ -35,12 +36,16 @@ static struct tty_driver	*kgdb_tty_drive
 static int			kgdb_tty_line;
 
 #ifdef CONFIG_KDB_KEYBOARD
+static bool			kgdboc_use_kbd;
+
 static int kgdboc_register_kbd(char **cptr)
 {
+	kgdboc_use_kbd = false;
 	if (strncmp(*cptr, "kbd", 3) == 0) {
 		if (kdb_poll_idx < KDB_POLL_FUNC_MAX) {
 			kdb_poll_funcs[kdb_poll_idx] = kdb_get_kbd_char;
 			kdb_poll_idx++;
+			kgdboc_use_kbd = true;
 			if (cptr[0][3] == ',')
 				*cptr += 4;
 			else
@@ -63,9 +68,16 @@ static void kgdboc_unregister_kbd(void)
 		}
 	}
 }
+
+static inline void kgdboc_clear_kbd(void)
+{
+	if (kgdboc_use_kbd)
+		kbd_dbg_clear_keys(); /* Release all pressed keys */
+}
 #else /* ! CONFIG_KDB_KEYBOARD */
 #define kgdboc_register_kbd(x) 0
 #define kgdboc_unregister_kbd()
+#define kgdboc_clear_kbd()
 #endif /* ! CONFIG_KDB_KEYBOARD */
 
 static int kgdboc_option_setup(char *opt)
@@ -213,6 +225,7 @@ static void kgdboc_post_exp_handler(void
 	/* decrement the module count when the debugger detaches */
 	if (!kgdb_connected)
 		module_put(THIS_MODULE);
+	kgdboc_clear_kbd();
 }
 
 static struct kgdb_io kgdboc_io_ops = {
--- a/include/linux/kbd_kern.h
+++ b/include/linux/kbd_kern.h
@@ -144,6 +144,7 @@ struct console;
 int getkeycode(unsigned int scancode);
 int setkeycode(unsigned int scancode, unsigned int keycode);
 void compute_shiftstate(void);
+void kbd_dbg_clear_keys(void);
 
 /* defkeymap.c */
 

------------------------------------------------------------------------------
Download Intel&#174; Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev