FYI: ACPI 'sleep 1' resets atkbd keycodes

FYI: ACPI 'sleep 1' resets atkbd keycodes

[P. Christeas]

This may be just a minor issue:
I had to use the setkeycodes utility to enable some extra keys (that weren't 
mapped by kernel's atkbd tables).
After waking from sleep 1, those keys were reset. That is, I had to use 
'setkeycodes' again to customize the tables again.

IMHO the way kernel works now is correct. It should *not* have extra code just 
to handle that. Just make sure anybody that alters his kbd tables puts some 
extra script to recover the tables after an ACPI wake.
This should be more like a note to Linux distributors.

Have a nice day.

Re: FYI: ACPI 'sleep 1' resets atkbd keycodes

[Vojtech Pavlik]

On Sun, Jan 25, 2004 at 11:37:19AM +0200, P. Christeas wrote:
> This may be just a minor issue:
> I had to use the setkeycodes utility to enable some extra keys (that weren't 
> mapped by kernel's atkbd tables).
> After waking from sleep 1, those keys were reset. That is, I had to use 
> 'setkeycodes' again to customize the tables again.
> 
> IMHO the way kernel works now is correct. It should *not* have extra code just 
> to handle that. Just make sure anybody that alters his kbd tables puts some 
> extra script to recover the tables after an ACPI wake.
> This should be more like a note to Linux distributors.

Hmm, I, on the other hand don't think it's correct. Because the kernel
has extra code to delete the tables on wake, which is wrong. Thanks for
noticing.

-- 
Vojtech Pavlik
SuSE Labs, SuSE CR

Re: FYI: ACPI 'sleep 1' resets atkbd keycodes

[Vojtech Pavlik]

[-- Attachment #1: Type: text/plain, Size: 763 bytes --]

On Sun, Jan 25, 2004 at 11:37:19AM +0200, P. Christeas wrote:

> This may be just a minor issue:
> I had to use the setkeycodes utility to enable some extra keys (that weren't 
> mapped by kernel's atkbd tables).
> After waking from sleep 1, those keys were reset. That is, I had to use 
> 'setkeycodes' again to customize the tables again.
> 
> IMHO the way kernel works now is correct. It should *not* have extra code just 
> to handle that. Just make sure anybody that alters his kbd tables puts some 
> extra script to recover the tables after an ACPI wake.
> This should be more like a note to Linux distributors.
> 
> Have a nice day.

Patch attached, please test. It'll make it into 2.6.3, with some luck
even 2.6.2.

-- 
Vojtech Pavlik
SuSE Labs, SuSE CR

[-- Attachment #2: atkbd-noreinit --]
[-- Type: text/plain, Size: 2009 bytes --]

ChangeSet@1.1519, 2004-01-25 12:58:24+01:00, vojtech@ucw.cz
  input: Bail out in atkbd.c if scancode set is changed, don't
         reinitialize scancode map. This is even more anoying than
         a new keyboard device in the unlikely case of set change.


 atkbd.c |   37 ++++++-------------------------------
 1 files changed, 6 insertions(+), 31 deletions(-)


diff -Nru a/drivers/input/keyboard/atkbd.c b/drivers/input/keyboard/atkbd.c
--- a/drivers/input/keyboard/atkbd.c	Sun Jan 25 12:59:07 2004
+++ b/drivers/input/keyboard/atkbd.c	Sun Jan 25 12:59:07 2004
@@ -743,43 +743,18 @@
 	struct serio_dev *dev = serio->dev;
 	int i;
 
-        if (!dev) {
-                printk(KERN_DEBUG "atkbd: reconnect request, but serio is disconnected, ignoring...\n");
-                return -1;
-        }
+	if (!dev) {
+		printk(KERN_DEBUG "atkbd: reconnect request, but serio is disconnected, ignoring...\n");
+		return -1;
+	}
 
 	if (atkbd->write) {
 		if (atkbd_probe(atkbd))
 			return -1;
-
-		atkbd->set = atkbd_set_3(atkbd);
+		if (atkbd->set != atkbd_set_3(atkbd))
+			return -1;
 		atkbd_enable(atkbd);
-	} else {
-		atkbd->set = 2;
-		atkbd->id = 0xab00;
 	}
-
-	/*
-	 * Here we probably should check if the keyboard has the same set that
-         * it had before and bail out if it's different. But this will most likely
-         * cause new keyboard device be created... and for the user it will look
-         * like keyboard is lost
-	 */
-
-	if (atkbd->translated) {
-		for (i = 0; i < 128; i++) {
-			atkbd->keycode[i] = atkbd_set2_keycode[atkbd_unxlate_table[i]];
-			atkbd->keycode[i | 0x80] = atkbd_set2_keycode[atkbd_unxlate_table[i] | 0x80];
-		}
-	} else if (atkbd->set == 2) {
-		memcpy(atkbd->keycode, atkbd_set2_keycode, sizeof(atkbd->keycode));
-	} else {
-		memcpy(atkbd->keycode, atkbd_set3_keycode, sizeof(atkbd->keycode));
-	}
-
-	for (i = 0; i < 512; i++)
-		if (atkbd->keycode[i] && atkbd->keycode[i] < 255)
-			set_bit(atkbd->keycode[i], atkbd->dev.keybit);
 
 	return 0;
 }

Re: FYI: ACPI 'sleep 1' resets atkbd keycodes

[P. Christeas]

> Patch attached, please test. It'll make it into 2.6.3, with some luck
> even 2.6.2.

Your patch works fine for me. (shame that I had to reboot, though)

Many thanks for your time.

Re: FYI: ACPI 'sleep 1' resets atkbd keycodes

[Dmitry Torokhov]

On Sunday 25 January 2004 06:59 am, Vojtech Pavlik wrote:
> On Sun, Jan 25, 2004 at 11:37:19AM +0200, P. Christeas wrote:
> > This may be just a minor issue:
> > I had to use the setkeycodes utility to enable some extra keys (that
> > weren't mapped by kernel's atkbd tables).
> > After waking from sleep 1, those keys were reset. That is, I had to
> > use 'setkeycodes' again to customize the tables again.
> >
> > IMHO the way kernel works now is correct. It should *not* have extra
> > code just to handle that. Just make sure anybody that alters his kbd
> > tables puts some extra script to recover the tables after an ACPI
> > wake.
> > This should be more like a note to Linux distributors.
> >
> > Have a nice day.
>
> Patch attached, please test. It'll make it into 2.6.3, with some luck
> even 2.6.2.

I feel a little uneasy about killing the input device if set changes after
resume. That would cause a new input device created but for user it would
look like keyboard is lost or am I missing someting?

What about the patch below which would reset keyboard to the default keymap
only if set changes?

-- 
Dmitry

===================================================================


ChangeSet@1.1543, 2004-02-02 23:33:52-05:00, dtor_core@ameritech.net
  Input: atkbd - load default keymap on resume only if set is changed


 atkbd.c |   95 ++++++++++++++++++++++++++++++++++------------------------------
 1 files changed, 51 insertions(+), 44 deletions(-)


===================================================================



diff -Nru a/drivers/input/keyboard/atkbd.c b/drivers/input/keyboard/atkbd.c
--- a/drivers/input/keyboard/atkbd.c	Mon Feb  2 23:35:33 2004
+++ b/drivers/input/keyboard/atkbd.c	Mon Feb  2 23:35:33 2004
@@ -619,6 +619,38 @@
 	kfree(atkbd);
 }
 
+static void atkbd_set_name(struct atkbd *atkbd)
+{
+	if (atkbd->set == 4) {
+		sprintf(atkbd->name, "AT Set 2 Extended keyboard");
+	} else
+		sprintf(atkbd->name, "AT %s Set %d keyboard",
+			atkbd->translated ? "Translated" : "Raw", atkbd->set);
+}
+
+/*
+ * Installs default keymap based on the detected mode (set)
+ */
+static void atkbd_install_keymap(struct atkbd *atkbd)
+{
+	int i;
+
+	if (atkbd->translated) {
+		for (i = 0; i < 128; i++) {
+			atkbd->keycode[i] = atkbd_set2_keycode[atkbd_unxlate_table[i]];
+			atkbd->keycode[i | 0x80] = atkbd_set2_keycode[atkbd_unxlate_table[i] | 0x80];
+		}
+	} else if (atkbd->set == 2) {
+		memcpy(atkbd->keycode, atkbd_set2_keycode, sizeof(atkbd->keycode));
+	} else {
+		memcpy(atkbd->keycode, atkbd_set3_keycode, sizeof(atkbd->keycode));
+	}
+
+	for (i = 0; i < 512; i++)
+		if (atkbd->keycode[i] && atkbd->keycode[i] < 255)
+			set_bit(atkbd->keycode[i], atkbd->dev.keybit);
+}
+
 /*
  * atkbd_connect() is called when the serio module finds and interface
  * that isn't handled yet by an appropriate device driver. We check if
@@ -629,7 +661,6 @@
 static void atkbd_connect(struct serio *serio, struct serio_dev *dev)
 {
 	struct atkbd *atkbd;
-	int i;
 
 	if (!(atkbd = kmalloc(sizeof(struct atkbd), GFP_KERNEL)))
 		return;
@@ -696,26 +727,12 @@
 		atkbd->id = 0xab00;
 	}
 
-	if (atkbd->set == 4) {
+	if (atkbd->set == 4)
 		atkbd->dev.ledbit[0] |= BIT(LED_COMPOSE) | BIT(LED_SUSPEND) | BIT(LED_SLEEP) | BIT(LED_MUTE) | BIT(LED_MISC);
-		sprintf(atkbd->name, "AT Set 2 Extended keyboard");
-	} else
-		sprintf(atkbd->name, "AT %s Set %d keyboard",
-			atkbd->translated ? "Translated" : "Raw", atkbd->set);
 
+	atkbd_set_name(atkbd);
 	sprintf(atkbd->phys, "%s/input0", serio->phys);
 
-	if (atkbd->translated) {
-		for (i = 0; i < 128; i++) {
-			atkbd->keycode[i] = atkbd_set2_keycode[atkbd_unxlate_table[i]];
-			atkbd->keycode[i | 0x80] = atkbd_set2_keycode[atkbd_unxlate_table[i] | 0x80];
-		}
-	} else if (atkbd->set == 2) {
-		memcpy(atkbd->keycode, atkbd_set2_keycode, sizeof(atkbd->keycode));
-	} else {
-		memcpy(atkbd->keycode, atkbd_set3_keycode, sizeof(atkbd->keycode));
-	}
-
 	atkbd->dev.name = atkbd->name;
 	atkbd->dev.phys = atkbd->phys;
 	atkbd->dev.id.bustype = BUS_I8042;
@@ -723,9 +740,7 @@
 	atkbd->dev.id.product = atkbd->translated ? 1 : atkbd->set;
 	atkbd->dev.id.version = atkbd->id;
 
-	for (i = 0; i < 512; i++)
-		if (atkbd->keycode[i] && atkbd->keycode[i] < 255)
-			set_bit(atkbd->keycode[i], atkbd->dev.keybit);
+	atkbd_install_keymap(atkbd);
 
 	input_register_device(&atkbd->dev);
 
@@ -741,12 +756,12 @@
 {
 	struct atkbd *atkbd = serio->private;
 	struct serio_dev *dev = serio->dev;
-	int i;
+	int old_set = atkbd->set;
 
-        if (!dev) {
-                printk(KERN_DEBUG "atkbd: reconnect request, but serio is disconnected, ignoring...\n");
-                return -1;
-        }
+	if (!dev) {
+		printk(KERN_DEBUG "atkbd: reconnect request, but serio is disconnected, ignoring...\n");
+		return -1;
+	}
 
 	if (atkbd->write) {
 		if (atkbd_probe(atkbd))
@@ -759,27 +774,19 @@
 		atkbd->id = 0xab00;
 	}
 
-	/*
-	 * Here we probably should check if the keyboard has the same set that
-         * it had before and bail out if it's different. But this will most likely
-         * cause new keyboard device be created... and for the user it will look
-         * like keyboard is lost
-	 */
+	if (atkbd->set != old_set) {
+		/*
+		 * ok, we woke up with a different keyboard attached. Alhtough we could just
+		 * signal failure it's not very nice as it will most likely cause new keyboard
+		 * device be created... and for the user it will look like keyboard is lost.
+		 */
+		atkbd_set_name(atkbd);
 
-	if (atkbd->translated) {
-		for (i = 0; i < 128; i++) {
-			atkbd->keycode[i] = atkbd_set2_keycode[atkbd_unxlate_table[i]];
-			atkbd->keycode[i | 0x80] = atkbd_set2_keycode[atkbd_unxlate_table[i] | 0x80];
-		}
-	} else if (atkbd->set == 2) {
-		memcpy(atkbd->keycode, atkbd_set2_keycode, sizeof(atkbd->keycode));
-	} else {
-		memcpy(atkbd->keycode, atkbd_set3_keycode, sizeof(atkbd->keycode));
-	}
+		printk(KERN_INFO "atkbd: new %s detected at %s, loading default keymap\n",
+			atkbd->name, atkbd->name);
 
-	for (i = 0; i < 512; i++)
-		if (atkbd->keycode[i] && atkbd->keycode[i] < 255)
-			set_bit(atkbd->keycode[i], atkbd->dev.keybit);
+		atkbd_install_keymap(atkbd);
+	}
 
 	return 0;
 }

Re: FYI: ACPI 'sleep 1' resets atkbd keycodes

[Vojtech Pavlik]

On Mon, Feb 02, 2004 at 11:38:41PM -0500, Dmitry Torokhov wrote:

> I feel a little uneasy about killing the input device if set changes after
> resume. That would cause a new input device created but for user it would
> look like keyboard is lost or am I missing someting?

The console will automatically and immediately grab the new keyboard and
everything will continue to work, even X, because it uses the console
for input.

Later, when X uses the event interface, it'll get a notification that
the keyboard was removed and added again, which will cause it to open
the new one, so all will be OK, too.

> What about the patch below which would reset keyboard to the default keymap
> only if set changes?

I don't think there is a problem with just reconnecting. It's extremely
unlikely that the set can change, too, because everyone uses translated
set 2, and that's the lowest common denominator.

Unless you specified extra arguments on the kernel command line, you
can't get any other set.

-- 
Vojtech Pavlik
SuSE Labs, SuSE CR

Re: FYI: ACPI 'sleep 1' resets atkbd keycodes

[bill davidsen]

In article <200401251137.21646.p_christ@hol.gr>,
P. Christeas <p_christ@hol.gr> wrote:
| This may be just a minor issue:
| I had to use the setkeycodes utility to enable some extra keys (that weren't 
| mapped by kernel's atkbd tables).
| After waking from sleep 1, those keys were reset. That is, I had to use 
| 'setkeycodes' again to customize the tables again.
| 
| IMHO the way kernel works now is correct. It should *not* have extra code just 
| to handle that. Just make sure anybody that alters his kbd tables puts some 
| extra script to recover the tables after an ACPI wake.
| This should be more like a note to Linux distributors.

I'm not totally sure I agree that the kernel should save this
information. With all the things the kernel does save, keyboard might be
a good thing. Consider someone with a REALLY hacked layout, like
modified dvorak or some of the keyboards for the access challenged. Now
think "can't use the keyboard enough to type in the ...ing commands to
do the load."

If we can carry the code for two competing disfunctional ACPI
implementations and a used-to-work but we broke it for some machines
APM, we can surely add a pair om memcpy lines to eliminate at least one
"oh-shit moment."

Obviously my opinion only, some breakage due to BIOS misfeatures,
contents may settle in shipping, etc.
-- 
bill davidsen <davidsen@tmr.com>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.

RE: FYI: ACPI 'sleep 1' resets atkbd keycodes

[Yu, Luming]

> > > This may be just a minor issue:
> > > I had to use the setkeycodes utility to enable some extra 
> keys (that
> > > weren't mapped by kernel's atkbd tables).
> > > After waking from sleep 1, those keys were reset. That 
> is, I had to
> > > use 'setkeycodes' again to customize the tables again.
> > >
> > > IMHO the way kernel works now is correct. It should *not* 
> have extra
> > > code just to handle that. Just make sure anybody that 
> alters his kbd
> > > tables puts some extra script to recover the tables after an ACPI
> > > wake.
> > > This should be more like a note to Linux distributors.
> > >
> > > Have a nice day.

Do you need a event through /proc/acpi/event that can notify userland of
resuming happend,
And let userland app have chance to invoke setkeycode, and other similar
things.

> -	/*
> -	 * Here we probably should check if the keyboard has 
> the same set that
> -         * it had before and bail out if it's different. But 
> this will most likely
> -         * cause new keyboard device be created... and for 
> the user it will look
> -         * like keyboard is lost
> -	 */
> +	if (atkbd->set != old_set) {
> +		/*
> +		 * ok, we woke up with a different keyboard 
> attached. Alhtough we could just
> +		 * signal failure it's not very nice as it will 
> most likely cause new keyboard
> +		 * device be created... and for the user it 
> will look like keyboard is lost.
> +		 */
> +		atkbd_set_name(atkbd);

Is it correct to think if the keyboard has the different set that it
had before , then keyboard has been replaced, without considering
it could be changed by setkeycode.

--Luming