Regression?: keyboard state/LED inconsistency in 2.6.32-rc1/2,3,4

Regression?: keyboard state/LED inconsistency in 2.6.32-rc1/2,3,4

[Marin Mitov]

Hello,

I'm testing 2.6.32-rc1/2,3,4 and I'm using 2.6.31.3.
There is a difference in keyboard/LED behavior.
If in BIOS NumLock is set on, when the kernel starts
it puts NumLock off and the LED state is off in 2.6.31.3
(which is consistent), but in 2.6.32-rc1/2,3,4 the LED
stays on (and this is not consistent with keyboard's state).

Comparing drivers/input/keyboard/atkbd.c (I'm using it)
in 2.6.31.3 and 2.6.32-rc1/2,3,4 among others changes I
found the following lines (see the patch bellow) deleted,
so I propose either to add them again (this will restore 
keyboard/LED consistency) or revert the patch that has 
deleted them.

The better solution will be if the kernel preserves the state
of the keyboard as set by BIOS.

Best regards.

Marin Mitov

P.S. The lines in the patch concerning "repeat rate" are
not related with the "keyboard state/LED inconsistency",
but have been also deleted (by mistake/intentionally) too.
Could be dropped if necessary.

Marin Mitov

Signed-off-by: Marin Mitov <mitov@issp.bas.bg>

======================================================
--- a/drivers/input/keyboard/atkbd.c	2009-10-11 20:15:27.000000000 +0300
+++ b/drivers/input/keyboard/atkbd.c	2009-10-11 20:17:34.000000000 +0300
@@ -773,6 +773,23 @@
 static int atkbd_activate(struct atkbd *atkbd)
 {
 	struct ps2dev *ps2dev = &atkbd->ps2dev;
+	unsigned char param[1];
+
+/*
+ * Set the LEDs to a defined state.
+ */
+
+	param[0] = 0;
+	if (ps2_command(ps2dev, param, ATKBD_CMD_SETLEDS))
+		return -1;
+
+/*
+ * Set autorepeat to fastest possible.
+ */
+
+	param[0] = 0;
+	if (ps2_command(ps2dev, param, ATKBD_CMD_SETREP))
+		return -1;
 
 /*
  * Enable the keyboard to receive keystrokes.
@@ -1141,6 +1158,14 @@
 			return -1;
 
 		atkbd_activate(atkbd);
+
+/*
+ * Restore repeat rate and LEDs (that were reset by atkbd_activate)
+ * to pre-resume state
+ */
+		if (!atkbd->softrepeat)
+			atkbd_set_repeat_rate(atkbd);
+		atkbd_set_leds(atkbd);
 	}
 
 	atkbd_enable(atkbd);

Re: Regression?: keyboard state/LED inconsistency in 2.6.32-rc1/2,3,4

[Vojtech Pavlik]

On Mon, Oct 12, 2009 at 11:18:28AM +0300, Marin Mitov wrote:
> Hello,
> 
> I'm testing 2.6.32-rc1/2,3,4 and I'm using 2.6.31.3.
> There is a difference in keyboard/LED behavior.
> If in BIOS NumLock is set on, when the kernel starts
> it puts NumLock off and the LED state is off in 2.6.31.3
> (which is consistent), but in 2.6.32-rc1/2,3,4 the LED
> stays on (and this is not consistent with keyboard's state).
> 
> Comparing drivers/input/keyboard/atkbd.c (I'm using it)
> in 2.6.31.3 and 2.6.32-rc1/2,3,4 among others changes I
> found the following lines (see the patch bellow) deleted,
> so I propose either to add them again (this will restore 
> keyboard/LED consistency) or revert the patch that has 
> deleted them.

Oh, wow, those lines are quite clearly needed, I wonder why they got
deleted, and I'm too lazy to find the commit comment ...

> The better solution will be if the kernel preserves the state
> of the keyboard as set by BIOS.

That is certainly possible - by reading a BIOS variable from memory it's
possible to figure out what the BIOS setting was. But it has to be done
outside atkbd, because it affects all system keyboards, including eg.
USB.

Vojtech

> 
> Best regards.
> 
> Marin Mitov
> 
> P.S. The lines in the patch concerning "repeat rate" are
> not related with the "keyboard state/LED inconsistency",
> but have been also deleted (by mistake/intentionally) too.
> Could be dropped if necessary.
> 
> Marin Mitov
> 
> Signed-off-by: Marin Mitov <mitov@issp.bas.bg>
> 
> ======================================================
> --- a/drivers/input/keyboard/atkbd.c	2009-10-11 20:15:27.000000000 +0300
> +++ b/drivers/input/keyboard/atkbd.c	2009-10-11 20:17:34.000000000 +0300
> @@ -773,6 +773,23 @@
>  static int atkbd_activate(struct atkbd *atkbd)
>  {
>  	struct ps2dev *ps2dev = &atkbd->ps2dev;
> +	unsigned char param[1];
> +
> +/*
> + * Set the LEDs to a defined state.
> + */
> +
> +	param[0] = 0;
> +	if (ps2_command(ps2dev, param, ATKBD_CMD_SETLEDS))
> +		return -1;
> +
> +/*
> + * Set autorepeat to fastest possible.
> + */
> +
> +	param[0] = 0;
> +	if (ps2_command(ps2dev, param, ATKBD_CMD_SETREP))
> +		return -1;
>  
>  /*
>   * Enable the keyboard to receive keystrokes.
> @@ -1141,6 +1158,14 @@
>  			return -1;
>  
>  		atkbd_activate(atkbd);
> +
> +/*
> + * Restore repeat rate and LEDs (that were reset by atkbd_activate)
> + * to pre-resume state
> + */
> +		if (!atkbd->softrepeat)
> +			atkbd_set_repeat_rate(atkbd);
> +		atkbd_set_leds(atkbd);
>  	}
>  
>  	atkbd_enable(atkbd);
> 

-- 
Vojtech Pavlik
Director SuSE Labs

Re: Regression?: keyboard state/LED inconsistency in 2.6.32-rc1/2,3,4

[Dmitry Torokhov]

On Mon, Oct 12, 2009 at 10:30:15AM +0200, Vojtech Pavlik wrote:
> On Mon, Oct 12, 2009 at 11:18:28AM +0300, Marin Mitov wrote:
> > Hello,
> > 
> > I'm testing 2.6.32-rc1/2,3,4 and I'm using 2.6.31.3.
> > There is a difference in keyboard/LED behavior.
> > If in BIOS NumLock is set on, when the kernel starts
> > it puts NumLock off and the LED state is off in 2.6.31.3
> > (which is consistent), but in 2.6.32-rc1/2,3,4 the LED
> > stays on (and this is not consistent with keyboard's state).
> > 
> > Comparing drivers/input/keyboard/atkbd.c (I'm using it)
> > in 2.6.31.3 and 2.6.32-rc1/2,3,4 among others changes I
> > found the following lines (see the patch bellow) deleted,
> > so I propose either to add them again (this will restore 
> > keyboard/LED consistency) or revert the patch that has 
> > deleted them.
> 
> Oh, wow, those lines are quite clearly needed, I wonder why they got
> deleted, and I'm too lazy to find the commit comment ...
> 

Hmm, it appears I deleted a bit too much.. At some point I was thinking
the input core would reset LEDs and repeat rate on newly created device
but it was wrong idea. Unfortunately I was careless in applying atkbd
patch removing resume support (which should be handled by the input
core)...

-- 
Dmitry

Re: Regression?: keyboard state/LED inconsistency in 2.6.32-rc1/2,3,4

[Dmitry Torokhov]

On Mon, Oct 12, 2009 at 01:57:08AM -0700, Dmitry Torokhov wrote:
> On Mon, Oct 12, 2009 at 10:30:15AM +0200, Vojtech Pavlik wrote:
> > On Mon, Oct 12, 2009 at 11:18:28AM +0300, Marin Mitov wrote:
> > > Hello,
> > > 
> > > I'm testing 2.6.32-rc1/2,3,4 and I'm using 2.6.31.3.
> > > There is a difference in keyboard/LED behavior.
> > > If in BIOS NumLock is set on, when the kernel starts
> > > it puts NumLock off and the LED state is off in 2.6.31.3
> > > (which is consistent), but in 2.6.32-rc1/2,3,4 the LED
> > > stays on (and this is not consistent with keyboard's state).
> > > 
> > > Comparing drivers/input/keyboard/atkbd.c (I'm using it)
> > > in 2.6.31.3 and 2.6.32-rc1/2,3,4 among others changes I
> > > found the following lines (see the patch bellow) deleted,
> > > so I propose either to add them again (this will restore 
> > > keyboard/LED consistency) or revert the patch that has 
> > > deleted them.
> > 
> > Oh, wow, those lines are quite clearly needed, I wonder why they got
> > deleted, and I'm too lazy to find the commit comment ...
> > 
> 
> Hmm, it appears I deleted a bit too much.. At some point I was thinking
> the input core would reset LEDs and repeat rate on newly created device
> but it was wrong idea. Unfortunately I was careless in applying atkbd
> patch removing resume support (which should be handled by the input
> core)...
> 

Could you please try the patch below and let me know if it fixes
things?

Thanks!

-- 
Dmitry


Input: atkbd - restore resetting LED state at startup

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Fix breakage caused by commit 9605fb48e1998935a5ee70c965f90ad1ac023add
While the input core indeed takes care of restoring led state and
typematic settings upon resume the driver still need to initialize
them properly when registering a new device

Reported-by: Marin Mitov <mitov@issp.bas.bg>
Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---

 drivers/input/keyboard/atkbd.c |   26 ++++++++++++++++++++++++++
 1 files changed, 26 insertions(+), 0 deletions(-)


diff --git a/drivers/input/keyboard/atkbd.c b/drivers/input/keyboard/atkbd.c
index 4709e15..03d961f 100644
--- a/drivers/input/keyboard/atkbd.c
+++ b/drivers/input/keyboard/atkbd.c
@@ -770,6 +770,30 @@ static int atkbd_select_set(struct atkbd *atkbd, int target_set, int allow_extra
 	return 3;
 }
 
+static int atkbd_reset_state(struct atkbd *atkbd)
+{
+        struct ps2dev *ps2dev = &atkbd->ps2dev;
+	unsigned char param[1];
+
+	/*
+	 * Set the LEDs to a predefined state (all off).
+	 */
+
+	param[0] = 0;
+	if (ps2_command(ps2dev, param, ATKBD_CMD_SETLEDS))
+		return -1;
+
+	/*
+	 * Set autorepeat to fastest possible.
+	 */
+
+	param[0] = 0;
+	if (ps2_command(ps2dev, param, ATKBD_CMD_SETREP))
+		return -1;
+
+	return 0;
+}
+
 static int atkbd_activate(struct atkbd *atkbd)
 {
 	struct ps2dev *ps2dev = &atkbd->ps2dev;
@@ -1087,6 +1111,7 @@ static int atkbd_connect(struct serio *serio, struct serio_driver *drv)
 		}
 
 		atkbd->set = atkbd_select_set(atkbd, atkbd_set, atkbd_extra);
+		atkbd_reset_state(atkbd);
 		atkbd_activate(atkbd);
 
 	} else {
@@ -1267,6 +1292,7 @@ static ssize_t atkbd_set_extra(struct atkbd *atkbd, const char *buf, size_t coun
 
 		atkbd->dev = new_dev;
 		atkbd->set = atkbd_select_set(atkbd, atkbd->set, value);
+		atkbd_reset_state(atkbd);
 		atkbd_activate(atkbd);
 		atkbd_set_keycode_table(atkbd);
 		atkbd_set_device_attrs(atkbd);

Re: Regression?: keyboard state/LED inconsistency in 2.6.32-rc1/2,3,4

[Marin Mitov]

On Tuesday 13 October 2009 06:03:14 am Dmitry Torokhov wrote:
> On Mon, Oct 12, 2009 at 01:57:08AM -0700, Dmitry Torokhov wrote:
> > On Mon, Oct 12, 2009 at 10:30:15AM +0200, Vojtech Pavlik wrote:
> > > On Mon, Oct 12, 2009 at 11:18:28AM +0300, Marin Mitov wrote:
> > > > Hello,
> > > > 
> > > > I'm testing 2.6.32-rc1/2,3,4 and I'm using 2.6.31.3.
> > > > There is a difference in keyboard/LED behavior.
> > > > If in BIOS NumLock is set on, when the kernel starts
> > > > it puts NumLock off and the LED state is off in 2.6.31.3
> > > > (which is consistent), but in 2.6.32-rc1/2,3,4 the LED
> > > > stays on (and this is not consistent with keyboard's state).
> > > > 
> > > > Comparing drivers/input/keyboard/atkbd.c (I'm using it)
> > > > in 2.6.31.3 and 2.6.32-rc1/2,3,4 among others changes I
> > > > found the following lines (see the patch bellow) deleted,
> > > > so I propose either to add them again (this will restore 
> > > > keyboard/LED consistency) or revert the patch that has 
> > > > deleted them.
> > > 
> > > Oh, wow, those lines are quite clearly needed, I wonder why they got
> > > deleted, and I'm too lazy to find the commit comment ...
> > > 
> > 
> > Hmm, it appears I deleted a bit too much.. At some point I was thinking
> > the input core would reset LEDs and repeat rate on newly created device
> > but it was wrong idea. Unfortunately I was careless in applying atkbd
> > patch removing resume support (which should be handled by the input
> > core)...
> > 
> 
> Could you please try the patch below and let me know if it fixes
> things?
> 
> Thanks!
> 

Yes, Dmitry, it fixes the problem, thank you.

Marin Mitov

Re: Regression?: keyboard state/LED inconsistency in 2.6.32-rc1/2,3,4

[Dmitry Torokhov]

On Tue, Oct 13, 2009 at 07:55:43AM +0300, Marin Mitov wrote:
> On Tuesday 13 October 2009 06:03:14 am Dmitry Torokhov wrote:
> > On Mon, Oct 12, 2009 at 01:57:08AM -0700, Dmitry Torokhov wrote:
> > > On Mon, Oct 12, 2009 at 10:30:15AM +0200, Vojtech Pavlik wrote:
> > > > On Mon, Oct 12, 2009 at 11:18:28AM +0300, Marin Mitov wrote:
> > > > > Hello,
> > > > > 
> > > > > I'm testing 2.6.32-rc1/2,3,4 and I'm using 2.6.31.3.
> > > > > There is a difference in keyboard/LED behavior.
> > > > > If in BIOS NumLock is set on, when the kernel starts
> > > > > it puts NumLock off and the LED state is off in 2.6.31.3
> > > > > (which is consistent), but in 2.6.32-rc1/2,3,4 the LED
> > > > > stays on (and this is not consistent with keyboard's state).
> > > > > 
> > > > > Comparing drivers/input/keyboard/atkbd.c (I'm using it)
> > > > > in 2.6.31.3 and 2.6.32-rc1/2,3,4 among others changes I
> > > > > found the following lines (see the patch bellow) deleted,
> > > > > so I propose either to add them again (this will restore 
> > > > > keyboard/LED consistency) or revert the patch that has 
> > > > > deleted them.
> > > > 
> > > > Oh, wow, those lines are quite clearly needed, I wonder why they got
> > > > deleted, and I'm too lazy to find the commit comment ...
> > > > 
> > > 
> > > Hmm, it appears I deleted a bit too much.. At some point I was thinking
> > > the input core would reset LEDs and repeat rate on newly created device
> > > but it was wrong idea. Unfortunately I was careless in applying atkbd
> > > patch removing resume support (which should be handled by the input
> > > core)...
> > > 
> > 
> > Could you please try the patch below and let me know if it fixes
> > things?
> > 
> > Thanks!
> > 
> 
> Yes, Dmitry, it fixes the problem, thank you.
> 

Thank you for testing. Mouse re-initialization is pretty delicate topic,
may I ask you to test some more just to make sure fix is reliable - a
few more resumes and cold/warm boots would do...

Thanks lot.

-- 
Dmitry

Re: Regression?: keyboard state/LED inconsistency in 2.6.32-rc1/2,3,4

[Dmitry Torokhov]

On Mon, Oct 12, 2009 at 10:17:38PM -0700, Dmitry Torokhov wrote:
> On Tue, Oct 13, 2009 at 07:55:43AM +0300, Marin Mitov wrote:
> > On Tuesday 13 October 2009 06:03:14 am Dmitry Torokhov wrote:
> > > On Mon, Oct 12, 2009 at 01:57:08AM -0700, Dmitry Torokhov wrote:
> > > > On Mon, Oct 12, 2009 at 10:30:15AM +0200, Vojtech Pavlik wrote:
> > > > > On Mon, Oct 12, 2009 at 11:18:28AM +0300, Marin Mitov wrote:
> > > > > > Hello,
> > > > > > 
> > > > > > I'm testing 2.6.32-rc1/2,3,4 and I'm using 2.6.31.3.
> > > > > > There is a difference in keyboard/LED behavior.
> > > > > > If in BIOS NumLock is set on, when the kernel starts
> > > > > > it puts NumLock off and the LED state is off in 2.6.31.3
> > > > > > (which is consistent), but in 2.6.32-rc1/2,3,4 the LED
> > > > > > stays on (and this is not consistent with keyboard's state).
> > > > > > 
> > > > > > Comparing drivers/input/keyboard/atkbd.c (I'm using it)
> > > > > > in 2.6.31.3 and 2.6.32-rc1/2,3,4 among others changes I
> > > > > > found the following lines (see the patch bellow) deleted,
> > > > > > so I propose either to add them again (this will restore 
> > > > > > keyboard/LED consistency) or revert the patch that has 
> > > > > > deleted them.
> > > > > 
> > > > > Oh, wow, those lines are quite clearly needed, I wonder why they got
> > > > > deleted, and I'm too lazy to find the commit comment ...
> > > > > 
> > > > 
> > > > Hmm, it appears I deleted a bit too much.. At some point I was thinking
> > > > the input core would reset LEDs and repeat rate on newly created device
> > > > but it was wrong idea. Unfortunately I was careless in applying atkbd
> > > > patch removing resume support (which should be handled by the input
> > > > core)...
> > > > 
> > > 
> > > Could you please try the patch below and let me know if it fixes
> > > things?
> > > 
> > > Thanks!
> > > 
> > 
> > Yes, Dmitry, it fixes the problem, thank you.
> > 
> 
> Thank you for testing. Mouse re-initialization is pretty delicate topic,
> may I ask you to test some more just to make sure fix is reliable - a
> few more resumes and cold/warm boots would do...
> 

Oh, wait, scratch that... I mixed up the issues ;) Sorry about the
confusion.

-- 
Dmitry