[PATCH] at_keyboard: Fix keyboards that report IBM PC AT scan codes

[PATCH] at_keyboard: Fix keyboards that report IBM PC AT scan codes

[Javier Martinez Canillas]

GRUB has support for both the IBM PC XT (Set 1) and IBM PC AT (Set 2) key
scan code mappings. But the driver always hardcodes to Set 1, overwriting
the mode that was queried in the grub_keyboard_controller_init() function.

This breaks keyboard mapping on machines whose firmwares are not using the
Set 1. For example, this is the case in Lenovo P71 and HP Elitebook 8770W
laptops that report key scan codes using Set 2.

Instead of hardcoding to Set 1, use the value that was queried before.

Reported-by: Renaud Métrich <rmetrich@redhat.com>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

 grub-core/term/at_keyboard.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/grub-core/term/at_keyboard.c b/grub-core/term/at_keyboard.c
index 597111077bd..ed2ea0904c1 100644
--- a/grub-core/term/at_keyboard.c
+++ b/grub-core/term/at_keyboard.c
@@ -165,7 +165,7 @@ set_scancodes (void)
     }
 
 #if !USE_SCANCODE_SET
-  ps2_state.current_set = 1;
+  ps2_state.current_set = grub_keyboard_orig_set;
   return;
 #else
 
-- 
2.28.0

Re: [PATCH] at_keyboard: Fix keyboards that report IBM PC AT scan codes

[Renaud Métrich]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 2891 bytes --]

Hi,

Testing the proposed patch on my old Asus N53SN in Legacy failed: as 
soon as at_keyboard is selected, the keys are corrupted and it's 
impossible to do anything.

Digging into this, it appears that query_mode() returns 2 (so set2 needs 
to be used), but in fact internally the keycode are the ones expected by 
set1.

Then I tried always executing the set_scancodes() code querying the 
driver and it ended up working perfectly: for real set2 was then to be used.

Hence the new proposal below (Vladimir please review this, it may be 
"too dirty"):

commit 5e8d6963c50cd727bfb5614448f9c80743a59de1 (HEAD -> master)
Author: Renaud Métrich <rmetrich@redhat.com>
Date:   Thu Dec 3 09:43:21 2020 +0100

     Force using set_scancodes() code querying the driver.
     
     This solves issues when query_mode() returns 2 but the driver continues
     to send keycodes suitable for set 1 (seen on a Asus N53SN).
     
     Resolves: rhbz#1897587
     
     Signed-off-by: Renaud Métrich <rmetrich@redhat.com>

diff --git a/grub-core/term/at_keyboard.c b/grub-core/term/at_keyboard.c
index 0df54d5..c026034 100644
--- a/grub-core/term/at_keyboard.c
+++ b/grub-core/term/at_keyboard.c
@@ -172,7 +172,14 @@ set_scancodes (void)
        return;
      }
  
-#if !USE_SCANCODE_SET
+/*
+ * Code removed: querying the driver seems to be more robust than just querying
+ * the mode being returned. This was tested on both:
+ * - a Lenovo T460s (query_mode() returning 2 and code below working)
+ * - a Asus N53SN (query_mode() returning 2 but code below now working: driver
+ *   was expecting set1 keycodes)
+ */
+#if 0 && !USE_SCANCODE_SET

Renaud.

On 11/6/20 2:48 PM, Javier Martinez Canillas wrote:
> GRUB has support for both the IBM PC XT (Set 1) and IBM PC AT (Set 2) key
> scan code mappings. But the driver always hardcodes to Set 1, overwriting
> the mode that was queried in the grub_keyboard_controller_init() function.
>
> This breaks keyboard mapping on machines whose firmwares are not using the
> Set 1. For example, this is the case in Lenovo P71 and HP Elitebook 8770W
> laptops that report key scan codes using Set 2.
>
> Instead of hardcoding to Set 1, use the value that was queried before.
>
> Reported-by: Renaud Métrich <rmetrich@redhat.com>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
>
>   grub-core/term/at_keyboard.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/grub-core/term/at_keyboard.c b/grub-core/term/at_keyboard.c
> index 597111077bd..ed2ea0904c1 100644
> --- a/grub-core/term/at_keyboard.c
> +++ b/grub-core/term/at_keyboard.c
> @@ -165,7 +165,7 @@ set_scancodes (void)
>       }
>   
>   #if !USE_SCANCODE_SET
> -  ps2_state.current_set = 1;
> +  ps2_state.current_set = grub_keyboard_orig_set;
>     return;
>   #else
>   

[-- Attachment #1.1.1.2: Type: text/html, Size: 3601 bytes --]

[-- Attachment #1.1.2: OpenPGP_0x5D129094FB6E4326.asc --]
[-- Type: application/pgp-keys, Size: 3131 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH] at_keyboard: Fix keyboards that report IBM PC AT scan codes

[Vladimir 'phcoder' Serbinenko]

On Fri, Dec 4, 2020 at 5:53 AM Renaud Métrich <rmetrich@redhat.com> wrote:
>
> Hi,
>
> Testing the proposed patch on my old Asus N53SN in Legacy failed: as soon as at_keyboard is selected, the keys are corrupted and it's impossible to do anything.
>
> Digging into this, it appears that query_mode() returns 2 (so set2 needs to be used), but in fact internally the keycode are the ones expected by set1.

This is because the patch doesn't take into account that controller is
in "translate" mode.
@Javier Martinez Canillas : Can you make your patch check whether
KEYBOARD_AT_TRANSLATE is set ?

Re: [PATCH] at_keyboard: Fix keyboards that report IBM PC AT scan codes

[Javier Martinez Canillas]

Hello Vladimir,

On 12/11/20 3:08 PM, Vladimir 'phcoder' Serbinenko wrote:
> On Fri, Dec 4, 2020 at 5:53 AM Renaud Métrich <rmetrich@redhat.com> wrote:
>>
>> Hi,
>>
>> Testing the proposed patch on my old Asus N53SN in Legacy failed: as soon as at_keyboard is selected, the keys are corrupted and it's impossible to do anything.
>>
>> Digging into this, it appears that query_mode() returns 2 (so set2 needs to be used), but in fact internally the keycode are the ones expected by set1.
> 
> This is because the patch doesn't take into account that controller is
> in "translate" mode.
> @Javier Martinez Canillas : Can you make your patch check whether
> KEYBOARD_AT_TRANSLATE is set ?
>

Thanks a lot for you feedback. Renaud is working on a new patch that
takes this into account and will post to the mailing list after doing
some testing on the machines that where having the problem.

Best regards,
Javier

Re: [PATCH] at_keyboard: Fix keyboards that report IBM PC AT scan codes

[Renaud Métrich]

[-- Attachment #1.1.1: Type: text/plain, Size: 1419 bytes --]

Hi Vladimir,

Thanks for the hint, this was obvious now.

Please find attached the new patch which definitely fixes the issue.

It has been tested on various hardware (see git commit details).

In a nutshell the solution is to stick to set 1 if controller is in 
Translate mode, and use set X otherwise, X being the queried mode.

Additionally, in controller_fini, nothing has to be restored, since 
nothing was changed. This fixes an issue when switching between 
at_keyboard, console, and at_keyboard again, in case queried set is not 
the actual used set.

Renaud.

On 12/11/20 3:08 PM, Vladimir 'phcoder' Serbinenko wrote:
> On Fri, Dec 4, 2020 at 5:53 AM Renaud Métrich <rmetrich@redhat.com> wrote:
>> Hi,
>>
>> Testing the proposed patch on my old Asus N53SN in Legacy failed: as soon as at_keyboard is selected, the keys are corrupted and it's impossible to do anything.
>>
>> Digging into this, it appears that query_mode() returns 2 (so set2 needs to be used), but in fact internally the keycode are the ones expected by set1.
> This is because the patch doesn't take into account that controller is
> in "translate" mode.
> @Javier Martinez Canillas : Can you make your patch check whether
> KEYBOARD_AT_TRANSLATE is set ?
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

[-- Attachment #1.1.2: 0001-at_keyboard-use-set-1-when-keyboard-is-in-Translate-.patch --]
[-- Type: text/x-patch, Size: 4192 bytes --]

From eac16e0623fa99ae5a6642852321cb8f9c74b33d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Renaud=20M=C3=A9trich?= <rmetrich@redhat.com>
Date: Thu, 3 Dec 2020 09:13:24 +0100
Subject: [PATCH] at_keyboard: use set 1 when keyboard is in Translate mode
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

When keyboard controller acts in Translate mode (0x40 mask), then use
set 1 since translation is done.
Otherwise use the mode queried from the controller (usually set 2).

Added "atkeyb" debugging messages in at_keyboard module as well.

Resolves: rhbz#1897587

Tested on:
- Asus N53SN (set 1 used)
- Dell Precision (set 1 used)
- HP Elitebook (set 2 used)
- HP G5430 (set 1 used, keyboard in XT mode!)
- Lenovo P71 & Lenovo T460s (set 2 used)
- QEMU/KVM (set 1 used)

Signed-off-by: Renaud Métrich <rmetrich@redhat.com>
---
 grub-core/term/at_keyboard.c | 29 ++++++++++++++++++++++++-----
 include/grub/at_keyboard.h   |  4 ++++
 2 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/grub-core/term/at_keyboard.c b/grub-core/term/at_keyboard.c
index 597111077..260143826 100644
--- a/grub-core/term/at_keyboard.c
+++ b/grub-core/term/at_keyboard.c
@@ -135,20 +135,28 @@ query_mode (void)
   int e;
 
   e = write_mode (0);
-  if (!e)
+  if (!e) {
+    grub_dprintf("atkeyb", "query_mode: write_mode(0) failed\n");
     return 0;
+  }
 
   do {
     keyboard_controller_wait_until_ready ();
     ret = grub_inb (KEYBOARD_REG_DATA);
   } while (ret == GRUB_AT_ACK);
   /* QEMU translates the set even in no-translate mode.  */
-  if (ret == 0x43 || ret == 1)
+  if (ret == 0x43 || ret == 1) {
+    grub_dprintf("atkeyb", "query_mode: returning 1 (ret=0x%x)\n", ret);
     return 1;
-  if (ret == 0x41 || ret == 2)
+  }
+  if (ret == 0x41 || ret == 2) {
+    grub_dprintf("atkeyb", "query_mode: returning 2 (ret=0x%x)\n", ret);
     return 2;
-  if (ret == 0x3f || ret == 3)
+  }
+  if (ret == 0x3f || ret == 3) {
+    grub_dprintf("atkeyb", "query_mode: returning 3 (ret=0x%x)\n", ret);
     return 3;
+  }
   return 0;
 }
 
@@ -165,7 +173,13 @@ set_scancodes (void)
     }
 
 #if !USE_SCANCODE_SET
-  ps2_state.current_set = 1;
+  if ((grub_keyboard_controller_orig & KEYBOARD_AT_TRANSLATE) == KEYBOARD_AT_TRANSLATE) {
+    grub_dprintf ("atkeyb", "queried set is %d but keyboard in Translate mode, so actually in set 1\n", grub_keyboard_orig_set);
+    ps2_state.current_set = 1;
+  } else {
+    grub_dprintf ("atkeyb", "using queried set %d\n", grub_keyboard_orig_set);
+    ps2_state.current_set = grub_keyboard_orig_set;
+  }
   return;
 #else
 
@@ -266,6 +280,7 @@ grub_keyboard_controller_init (void)
   grub_keyboard_orig_set = 2;
 #else
   grub_keyboard_controller_orig = grub_keyboard_controller_read ();
+  grub_dprintf ("atkeyb", "grub_keyboard_controller_orig = 0x%x\n", grub_keyboard_controller_orig);
   grub_keyboard_orig_set = query_mode ();
 #endif
   set_scancodes ();
@@ -275,11 +290,15 @@ grub_keyboard_controller_init (void)
 static grub_err_t
 grub_keyboard_controller_fini (struct grub_term_input *term __attribute__ ((unused)))
 {
+/* In !USE_SCANCODE_SET mode, we didn't change anything, so nothing to restore */
+#if USE_SCANCODE_SET
   if (ps2_state.current_set == 0)
     return GRUB_ERR_NONE;
+  grub_dprintf ("atkeyb", "restoring set %d, controller 0x%x\n", grub_keyboard_orig_set, grub_keyboard_controller_orig);
   if (grub_keyboard_orig_set)
     write_mode (grub_keyboard_orig_set);
   grub_keyboard_controller_write (grub_keyboard_controller_orig);
+#endif
   return GRUB_ERR_NONE;
 }
 
diff --git a/include/grub/at_keyboard.h b/include/grub/at_keyboard.h
index bcb4d9ba7..9414dc1b9 100644
--- a/include/grub/at_keyboard.h
+++ b/include/grub/at_keyboard.h
@@ -19,6 +19,10 @@
 #ifndef GRUB_AT_KEYBOARD_HEADER
 #define GRUB_AT_KEYBOARD_HEADER	1
 
+/*
+ * Refer to https://wiki.osdev.org/%228042%22_PS/2_Controller for details.
+ */
+
 /* Used for sending commands to the controller.  */
 #define KEYBOARD_COMMAND_ISREADY(x)	!((x) & 0x02)
 #define KEYBOARD_COMMAND_READ		0x20
-- 
2.29.2


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH] at_keyboard: Fix keyboards that report IBM PC AT scan codes

[Renaud Métrich]

[-- Attachment #1.1.1: Type: text/plain, Size: 2278 bytes --]

Hello,

It appears that the proposal works fine on all systems I could test 
except one: a HP DL380p Gen 8.

On that system, querying the set fails: the ACK bytes in write_mode(0) 
work perfectly, but then 0xFE ("NACK") is read, as if the keyboard 
didn't want to send back the set in use.

Hence, query_mode() returns 0, causing set1 to be used, but 
unfortunately the system expects set2 to be used.

I tried using the "resend" command, but nothing helps.

I'm hence proposing a fallback solution for this kind of hardware where 
the admin can add a "at_keyboard_fallback_set" environment variable in 
grub.cfg that would end using a specific set if query_mode() fails.

See proposed patch in attachment.

Renaud.

On 12/14/20 5:47 PM, Renaud Métrich wrote:
> Hi Vladimir,
>
> Thanks for the hint, this was obvious now.
>
> Please find attached the new patch which definitely fixes the issue.
>
> It has been tested on various hardware (see git commit details).
>
> In a nutshell the solution is to stick to set 1 if controller is in 
> Translate mode, and use set X otherwise, X being the queried mode.
>
> Additionally, in controller_fini, nothing has to be restored, since 
> nothing was changed. This fixes an issue when switching between 
> at_keyboard, console, and at_keyboard again, in case queried set is 
> not the actual used set.
>
> Renaud.
>
> On 12/11/20 3:08 PM, Vladimir 'phcoder' Serbinenko wrote:
>> On Fri, Dec 4, 2020 at 5:53 AM Renaud Métrich <rmetrich@redhat.com> 
>> wrote:
>>> Hi,
>>>
>>> Testing the proposed patch on my old Asus N53SN in Legacy failed: as 
>>> soon as at_keyboard is selected, the keys are corrupted and it's 
>>> impossible to do anything.
>>>
>>> Digging into this, it appears that query_mode() returns 2 (so set2 
>>> needs to be used), but in fact internally the keycode are the ones 
>>> expected by set1.
>> This is because the patch doesn't take into account that controller is
>> in "translate" mode.
>> @Javier Martinez Canillas : Can you make your patch check whether
>> KEYBOARD_AT_TRANSLATE is set ?
>>
>> _______________________________________________
>> Grub-devel mailing list
>> Grub-devel@gnu.org
>> https://lists.gnu.org/mailman/listinfo/grub-devel

[-- Attachment #1.1.2: 0001-at_keyboard-New-at_keyboard_fallback_set-environment.patch --]
[-- Type: text/x-patch, Size: 9770 bytes --]

From baa977fa82ca10e60ad9e344e9383f42d08b13cb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Renaud=20M=C3=A9trich?= <rmetrich@redhat.com>
Date: Fri, 18 Dec 2020 15:39:26 +0100
Subject: [PATCH] at_keyboard: New 'at_keyboard_fallback_set' environment
 variable to force the set manually upon query_mode() failing
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This seems required with HP DL380p Gen 8 systems.
Indeed, with this system, we can see the following sequence:

1. controller is queried to get current configuration (returns 0x30 which is quite standard)
2. controller is queried to get the current keyboard set in used, using code 0xf0 (first part)
3. controller answers with 0xfa which means "ACK" (== ok)
4. then we send "0" to tell "we want to know which set your are supporting"
5. controller answers with 0xfa ("ACK")
6. controller should then give us 1, 2, 3 or 0x43, 0x41, 0x3f, but here it gives us 0xfe which means "NACK"

Since there seems no way to determine the current set, and in fact the
controller expects set2 to be used, we need to rely on an environment
variable.
Everything has been tested on this system: using 0xFE (resend command),
making sure we wait for ACK in the 2 steps "write_mode", etc.

Below is litterature I used to come up with "there is no other
solution":
- https://wiki.osdev.org/%228042%22_PS/2_Controller
- http://www-ug.eecg.toronto.edu/msl/nios_devices/datasheets/PS2%20Keyboard%20Protocol.htm
- http://www.s100computers.com/My%20System%20Pages/MSDOS%20Board/PC%20Keyboard.pdf

Signed-off-by: Renaud Métrich <rmetrich@redhat.com>
---
 grub-core/term/at_keyboard.c | 127 ++++++++++++++++++++++++++++-------
 1 file changed, 101 insertions(+), 26 deletions(-)

diff --git a/grub-core/term/at_keyboard.c b/grub-core/term/at_keyboard.c
index 260143826..51e14419a 100644
--- a/grub-core/term/at_keyboard.c
+++ b/grub-core/term/at_keyboard.c
@@ -31,6 +31,7 @@ GRUB_MOD_LICENSE ("GPLv3+");
 static grub_uint8_t grub_keyboard_controller_orig;
 static grub_uint8_t grub_keyboard_orig_set;
 struct grub_ps2_state ps2_state;
+static int fallback_set;
 
 static int ping_sent;
 
@@ -76,6 +77,8 @@ at_command (grub_uint8_t data)
 	break;
       return 0;
     }
+  if (i == GRUB_AT_TRIES)
+    grub_dprintf ("atkeyb", "at_command() timed out! (stopped after %d tries)\n", i);
   return (i != GRUB_AT_TRIES);
 }
 
@@ -105,6 +108,21 @@ grub_keyboard_controller_read (void)
 
 #endif
 
+static int
+resend_last_result (void)
+{
+  grub_uint8_t ret;
+  keyboard_controller_wait_until_ready ();
+  grub_dprintf ("atkeyb", "resend_last_result: sending 0xfe\n");
+  grub_outb (0xfe, KEYBOARD_REG_DATA);
+  ret = wait_ack ();
+  grub_dprintf ("atkeyb", "resend_last_result: wait_ack() returned 0x%x\n", ret);
+  keyboard_controller_wait_until_ready ();
+  ret = grub_inb (KEYBOARD_REG_DATA);
+  grub_dprintf ("atkeyb", "resend_last_result: read 0x%x from controller\n", ret);
+  return ret;
+}
+
 static int
 write_mode (int mode)
 {
@@ -113,11 +131,17 @@ write_mode (int mode)
     {
       grub_uint8_t ack;
       keyboard_controller_wait_until_ready ();
+      grub_dprintf ("atkeyb", "write_mode: sending 0xf0\n");
       grub_outb (0xf0, KEYBOARD_REG_DATA);
+      ack = wait_ack ();
+      grub_dprintf ("atkeyb", "write_mode: wait_ack() returned 0x%x\n", ack);
+      if (ack != GRUB_AT_ACK)
+	continue;
       keyboard_controller_wait_until_ready ();
+      grub_dprintf ("atkeyb", "write_mode: sending mode %d\n", mode);
       grub_outb (mode, KEYBOARD_REG_DATA);
-      keyboard_controller_wait_until_ready ();
       ack = wait_ack ();
+      grub_dprintf ("atkeyb", "write_mode: wait_ack() returned 0x%x\n", ack);
       if (ack == GRUB_AT_NACK)
 	continue;
       if (ack == GRUB_AT_ACK)
@@ -125,6 +149,9 @@ write_mode (int mode)
       return 0;
     }
 
+  if (i == GRUB_AT_TRIES)
+    grub_dprintf ("atkeyb", "write_mode() timed out! (stopped after %d tries)\n", i);
+
   return (i != GRUB_AT_TRIES);
 }
 
@@ -132,31 +159,66 @@ static int
 query_mode (void)
 {
   grub_uint8_t ret;
+  grub_uint64_t endtime;
+  unsigned i;
   int e;
+  char *envvar;
 
-  e = write_mode (0);
-  if (!e) {
-    grub_dprintf("atkeyb", "query_mode: write_mode(0) failed\n");
-    return 0;
-  }
+  for (i = 0; i < GRUB_AT_TRIES; i++) {
+    grub_dprintf ("atkeyb", "query_mode: sending command to controller\n");
+    e = write_mode (0);
+    if (!e) {
+      grub_dprintf ("atkeyb", "query_mode: write_mode(0) failed\n");
+      return 0;
+    }
 
-  do {
-    keyboard_controller_wait_until_ready ();
-    ret = grub_inb (KEYBOARD_REG_DATA);
-  } while (ret == GRUB_AT_ACK);
-  /* QEMU translates the set even in no-translate mode.  */
-  if (ret == 0x43 || ret == 1) {
-    grub_dprintf("atkeyb", "query_mode: returning 1 (ret=0x%x)\n", ret);
-    return 1;
-  }
-  if (ret == 0x41 || ret == 2) {
-    grub_dprintf("atkeyb", "query_mode: returning 2 (ret=0x%x)\n", ret);
-    return 2;
+    endtime = grub_get_time_ms () + 20;
+    do {
+      keyboard_controller_wait_until_ready ();
+      ret = grub_inb (KEYBOARD_REG_DATA);
+      grub_dprintf ("atkeyb", "query_mode/loop: read 0x%x from controller\n", ret);
+    } while ((ret == GRUB_AT_ACK || ret == GRUB_AT_NACK) && grub_get_time_ms () < endtime);
+    if (ret == 0xfe) {
+      grub_dprintf ("atkeyb", "query_mode: asking controller to resend last result\n");
+      ret = resend_last_result();
+      grub_dprintf ("atkeyb", "query_mode: read 0x%x from controller\n", ret);
+    }
+    /* QEMU translates the set even in no-translate mode.  */
+    if (ret == 0x43 || ret == 1) {
+      grub_dprintf ("atkeyb", "query_mode: controller returned 0x%x, returning 1\n", ret);
+      return 1;
+    }
+    if (ret == 0x41 || ret == 2) {
+      grub_dprintf ("atkeyb", "query_mode: controller returned 0x%x, returning 2\n", ret);
+      return 2;
+    }
+    if (ret == 0x3f || ret == 3) {
+      grub_dprintf ("atkeyb", "query_mode: controller returned 0x%x, returning 3\n", ret);
+      return 3;
+    }
+    grub_dprintf ("atkeyb", "query_mode: controller returned unexpected value 0x%x, retrying\n", ret);
   }
-  if (ret == 0x3f || ret == 3) {
-    grub_dprintf("atkeyb", "query_mode: returning 3 (ret=0x%x)\n", ret);
-    return 3;
+
+  /*
+   * Falling here means we tried querying and the controller returned something
+   * we don't understand, try to use 'at_keyboard_fallback_set' if it exists,
+   * otherwise return 0.
+   */
+  envvar = grub_env_get ("at_keyboard_fallback_set");
+  if (envvar) {
+    fallback_set = grub_strtoul (envvar, 0, 10);
+    if ((grub_errno) || (fallback_set < 1) || (fallback_set > 3)) {
+      grub_dprintf ("atkeyb", "WARNING: ignoring unexpected value '%s' for '%s' variable\n",
+		    envvar, "at_keyboard_fallback_set");
+      fallback_set = 0;
+    } else {
+      grub_dprintf ("atkeyb", "query_mode: '%s' specified in environment, returning %d\n",
+		    "at_keyboard_fallback_set", fallback_set);
+    }
+    return fallback_set;
   }
+  grub_dprintf ("atkeyb", "WARNING: no '%s' specified in environment, returning 0\n",
+		"at_keyboard_fallback_set");
   return 0;
 }
 
@@ -165,14 +227,25 @@ set_scancodes (void)
 {
   /* You must have visited computer museum. Keyboard without scancode set
      knowledge. Assume XT. */
-  if (!grub_keyboard_orig_set)
-    {
-      grub_dprintf ("atkeyb", "No sets support assumed\n");
-      ps2_state.current_set = 1;
+  if (!grub_keyboard_orig_set) {
+    if (fallback_set) {
+      grub_dprintf ("atkeyb", "No sets support assumed but set forced to %d\n", fallback_set);
+      ps2_state.current_set = fallback_set;
       return;
     }
+    grub_dprintf ("atkeyb", "No sets support assumed, forcing to set 1\n");
+    ps2_state.current_set = 1;
+    return;
+  }
 
 #if !USE_SCANCODE_SET
+  if (fallback_set) {
+    grub_dprintf ("atkeyb", "queried set is %d but set forced to %d\n",
+		  grub_keyboard_orig_set, fallback_set);
+    ps2_state.current_set = fallback_set;
+    return;
+  }
+
   if ((grub_keyboard_controller_orig & KEYBOARD_AT_TRANSLATE) == KEYBOARD_AT_TRANSLATE) {
     grub_dprintf ("atkeyb", "queried set is %d but keyboard in Translate mode, so actually in set 1\n", grub_keyboard_orig_set);
     ps2_state.current_set = 1;
@@ -229,6 +302,7 @@ grub_at_keyboard_is_alive (void)
 
   if (KEYBOARD_COMMAND_ISREADY (grub_inb (KEYBOARD_REG_STATUS)))
     {
+      grub_dprintf ("atkeyb", "grub_at_keyboard_is_alive: controller mode before self-test: 0x%x\n", grub_keyboard_controller_read());
       grub_outb (0xaa, KEYBOARD_REG_STATUS);
       ping_sent = 1;
     }
@@ -261,6 +335,7 @@ grub_at_keyboard_getkey (struct grub_term_input *term __attribute__ ((unused)))
 static void
 grub_keyboard_controller_init (void)
 {
+  grub_dprintf ("atkeyb", "initializing the controller\n");
   ps2_state.at_keyboard_status = 0;
   /* Drain input buffer. */
   while (1)
@@ -282,6 +357,7 @@ grub_keyboard_controller_init (void)
   grub_keyboard_controller_orig = grub_keyboard_controller_read ();
   grub_dprintf ("atkeyb", "grub_keyboard_controller_orig = 0x%x\n", grub_keyboard_controller_orig);
   grub_keyboard_orig_set = query_mode ();
+  grub_dprintf ("atkeyb", "grub_keyboard_orig_set = %d\n", grub_keyboard_orig_set);
 #endif
   set_scancodes ();
   keyboard_controller_led (ps2_state.led_status);
@@ -329,7 +405,6 @@ grub_at_restore_hw (void)
   return GRUB_ERR_NONE;
 }
 
-
 static struct grub_term_input grub_at_keyboard_term =
   {
     .name = "at_keyboard",
-- 
2.29.2


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH] at_keyboard: Fix keyboards that report IBM PC AT scan codes

[Javier Martinez Canillas]

Hello Vladimir and Daniel,

Could you please let us know your thoughts on Renaud's investigation
and patches? Thanks a lot for your help.

On 1/8/21 3:00 PM, Renaud Métrich wrote:
> Hello,
> 
> It appears that the proposal works fine on all systems I could test 
> except one: a HP DL380p Gen 8.
> 
> On that system, querying the set fails: the ACK bytes in write_mode(0) 
> work perfectly, but then 0xFE ("NACK") is read, as if the keyboard 
> didn't want to send back the set in use.
> 
> Hence, query_mode() returns 0, causing set1 to be used, but 
> unfortunately the system expects set2 to be used.
> 
> I tried using the "resend" command, but nothing helps.
> 
> I'm hence proposing a fallback solution for this kind of hardware where 
> the admin can add a "at_keyboard_fallback_set" environment variable in 
> grub.cfg that would end using a specific set if query_mode() fails.
> 
> See proposed patch in attachment.
> 
> Renaud.
> 
> On 12/14/20 5:47 PM, Renaud Métrich wrote:
>> Hi Vladimir,
>>
>> Thanks for the hint, this was obvious now.
>>
>> Please find attached the new patch which definitely fixes the issue.
>>
>> It has been tested on various hardware (see git commit details).
>>
>> In a nutshell the solution is to stick to set 1 if controller is in 
>> Translate mode, and use set X otherwise, X being the queried mode.
>>
>> Additionally, in controller_fini, nothing has to be restored, since 
>> nothing was changed. This fixes an issue when switching between 
>> at_keyboard, console, and at_keyboard again, in case queried set is 
>> not the actual used set.
>>
>> Renaud.
>>

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

Re: [PATCH] at_keyboard: Fix keyboards that report IBM PC AT scan codes

[Daniel Kiper]

Hi Javier and Renaud,

Sorry for late reply but I am busy with other important stuff.
I will try to take a look at the patch next week.

Daniel

On Tue, Jan 19, 2021 at 09:07:30AM +0100, Javier Martinez Canillas wrote:
> Hello Vladimir and Daniel,
>
> Could you please let us know your thoughts on Renaud's investigation
> and patches? Thanks a lot for your help.
>
> On 1/8/21 3:00 PM, Renaud Métrich wrote:
> > Hello,
> >
> > It appears that the proposal works fine on all systems I could test
> > except one: a HP DL380p Gen 8.
> >
> > On that system, querying the set fails: the ACK bytes in write_mode(0)
> > work perfectly, but then 0xFE ("NACK") is read, as if the keyboard
> > didn't want to send back the set in use.
> >
> > Hence, query_mode() returns 0, causing set1 to be used, but
> > unfortunately the system expects set2 to be used.
> >
> > I tried using the "resend" command, but nothing helps.
> >
> > I'm hence proposing a fallback solution for this kind of hardware where
> > the admin can add a "at_keyboard_fallback_set" environment variable in
> > grub.cfg that would end using a specific set if query_mode() fails.
> >
> > See proposed patch in attachment.
> >
> > Renaud.
> >
> > On 12/14/20 5:47 PM, Renaud Métrich wrote:
> >> Hi Vladimir,
> >>
> >> Thanks for the hint, this was obvious now.
> >>
> >> Please find attached the new patch which definitely fixes the issue.
> >>
> >> It has been tested on various hardware (see git commit details).
> >>
> >> In a nutshell the solution is to stick to set 1 if controller is in
> >> Translate mode, and use set X otherwise, X being the queried mode.
> >>
> >> Additionally, in controller_fini, nothing has to be restored, since
> >> nothing was changed. This fixes an issue when switching between
> >> at_keyboard, console, and at_keyboard again, in case queried set is
> >> not the actual used set.
> >>
> >> Renaud.
> >>
>
> Best regards,
> --
> Javier Martinez Canillas
> Software Engineer - Desktop Hardware Enablement
> Red Hat

Re: [PATCH] at_keyboard: Fix keyboards that report IBM PC AT scan codes

[Renaud Métrich]

[-- Attachment #1.1.1: Type: text/plain, Size: 2883 bytes --]

Hello,

Please find an updated version of the patch proposing a fallback 
mechanism in case grub is not able to detect the set to be used.

I has been tested on the following systems:

- Lenovo P71

- Asus N53SN

- Dell Precision

- HP Elitebook

- Lenovo T460s

- QEMU/KVM

- HP G5430

On all these above, the fallback mechanism is not used at all, set is 
detected correctly.

However the mechanism is necessary for HP DL380p Gen 8 systems.

Renaud.

On 1/8/21 3:00 PM, Renaud Métrich wrote:
> Hello,
>
> It appears that the proposal works fine on all systems I could test 
> except one: a HP DL380p Gen 8.
>
> On that system, querying the set fails: the ACK bytes in write_mode(0) 
> work perfectly, but then 0xFE ("NACK") is read, as if the keyboard 
> didn't want to send back the set in use.
>
> Hence, query_mode() returns 0, causing set1 to be used, but 
> unfortunately the system expects set2 to be used.
>
> I tried using the "resend" command, but nothing helps.
>
> I'm hence proposing a fallback solution for this kind of hardware 
> where the admin can add a "at_keyboard_fallback_set" environment 
> variable in grub.cfg that would end using a specific set if 
> query_mode() fails.
>
> See proposed patch in attachment.
>
> Renaud.
>
> On 12/14/20 5:47 PM, Renaud Métrich wrote:
>> Hi Vladimir,
>>
>> Thanks for the hint, this was obvious now.
>>
>> Please find attached the new patch which definitely fixes the issue.
>>
>> It has been tested on various hardware (see git commit details).
>>
>> In a nutshell the solution is to stick to set 1 if controller is in 
>> Translate mode, and use set X otherwise, X being the queried mode.
>>
>> Additionally, in controller_fini, nothing has to be restored, since 
>> nothing was changed. This fixes an issue when switching between 
>> at_keyboard, console, and at_keyboard again, in case queried set is 
>> not the actual used set.
>>
>> Renaud.
>>
>> On 12/11/20 3:08 PM, Vladimir 'phcoder' Serbinenko wrote:
>>> On Fri, Dec 4, 2020 at 5:53 AM Renaud Métrich <rmetrich@redhat.com> 
>>> wrote:
>>>> Hi,
>>>>
>>>> Testing the proposed patch on my old Asus N53SN in Legacy failed: 
>>>> as soon as at_keyboard is selected, the keys are corrupted and it's 
>>>> impossible to do anything.
>>>>
>>>> Digging into this, it appears that query_mode() returns 2 (so set2 
>>>> needs to be used), but in fact internally the keycode are the ones 
>>>> expected by set1.
>>> This is because the patch doesn't take into account that controller is
>>> in "translate" mode.
>>> @Javier Martinez Canillas : Can you make your patch check whether
>>> KEYBOARD_AT_TRANSLATE is set ?
>>>
>>> _______________________________________________
>>> Grub-devel mailing list
>>> Grub-devel@gnu.org
>>> https://lists.gnu.org/mailman/listinfo/grub-devel

[-- Attachment #1.1.2: 0001-at_keyboard-New-at_keyboard_fallback_set-environment.patch --]
[-- Type: text/x-patch, Size: 9270 bytes --]

From 3d290e4b6129dc1e6458815635d978302002c859 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Renaud=20M=C3=A9trich?= <rmetrich@redhat.com>
Date: Fri, 18 Dec 2020 15:39:26 +0100
Subject: [PATCH] at_keyboard: New 'at_keyboard_fallback_set' environment
 variable to force the set manually upon query_mode() failing
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This seems required with HP DL380p Gen 8 systems.
Indeed, with this system, we can see the following sequence:

1. controller is queried to get current configuration (returns 0x30 which is quite standard)
2. controller is queried to get the current keyboard set in used, using code 0xf0 (first part)
3. controller answers with 0xfa which means "ACK" (== ok)
4. then we send "0" to tell "we want to know which set your are supporting"
5. controller answers with 0xfa ("ACK")
6. controller should then give us 1, 2, 3 or 0x43, 0x41, 0x3f, but here it gives us 0xfe which means "NACK"

Since there seems no way to determine the current set, and in fact the
controller expects set2 to be used, we need to rely on an environment
variable.
Everything has been tested on this system: using 0xFE (resend command),
making sure we wait for ACK in the 2 steps "write_mode", etc.

Below is litterature I used to come up with "there is no other
solution":
- https://wiki.osdev.org/%228042%22_PS/2_Controller
- http://www-ug.eecg.toronto.edu/msl/nios_devices/datasheets/PS2%20Keyboard%20Protocol.htm
- http://www.s100computers.com/My%20System%20Pages/MSDOS%20Board/PC%20Keyboard.pdf

Signed-off-by: Renaud Métrich <rmetrich@redhat.com>
---
 grub-core/term/at_keyboard.c | 121 +++++++++++++++++++++++++++--------
 1 file changed, 96 insertions(+), 25 deletions(-)

diff --git a/grub-core/term/at_keyboard.c b/grub-core/term/at_keyboard.c
index 260143826..dac0f946f 100644
--- a/grub-core/term/at_keyboard.c
+++ b/grub-core/term/at_keyboard.c
@@ -31,6 +31,7 @@ GRUB_MOD_LICENSE ("GPLv3+");
 static grub_uint8_t grub_keyboard_controller_orig;
 static grub_uint8_t grub_keyboard_orig_set;
 struct grub_ps2_state ps2_state;
+static int fallback_set;
 
 static int ping_sent;
 
@@ -76,6 +77,8 @@ at_command (grub_uint8_t data)
 	break;
       return 0;
     }
+  if (i == GRUB_AT_TRIES)
+    grub_dprintf ("atkeyb", "at_command() timed out! (stopped after %d tries)\n", i);
   return (i != GRUB_AT_TRIES);
 }
 
@@ -105,6 +108,21 @@ grub_keyboard_controller_read (void)
 
 #endif
 
+static int
+resend_last_result (void)
+{
+  grub_uint8_t ret;
+  keyboard_controller_wait_until_ready ();
+  grub_dprintf ("atkeyb", "resend_last_result: sending 0xfe\n");
+  grub_outb (0xfe, KEYBOARD_REG_DATA);
+  ret = wait_ack ();
+  grub_dprintf ("atkeyb", "resend_last_result: wait_ack() returned 0x%x\n", ret);
+  keyboard_controller_wait_until_ready ();
+  ret = grub_inb (KEYBOARD_REG_DATA);
+  grub_dprintf ("atkeyb", "resend_last_result: read 0x%x from controller\n", ret);
+  return ret;
+}
+
 static int
 write_mode (int mode)
 {
@@ -113,11 +131,14 @@ write_mode (int mode)
     {
       grub_uint8_t ack;
       keyboard_controller_wait_until_ready ();
+      grub_dprintf ("atkeyb", "write_mode: sending 0xf0\n");
       grub_outb (0xf0, KEYBOARD_REG_DATA);
       keyboard_controller_wait_until_ready ();
+      grub_dprintf ("atkeyb", "write_mode: sending mode %d\n", mode);
       grub_outb (mode, KEYBOARD_REG_DATA);
       keyboard_controller_wait_until_ready ();
       ack = wait_ack ();
+      grub_dprintf ("atkeyb", "write_mode: wait_ack() returned 0x%x\n", ack);
       if (ack == GRUB_AT_NACK)
 	continue;
       if (ack == GRUB_AT_ACK)
@@ -125,6 +146,9 @@ write_mode (int mode)
       return 0;
     }
 
+  if (i == GRUB_AT_TRIES)
+    grub_dprintf ("atkeyb", "write_mode() timed out! (stopped after %d tries)\n", i);
+
   return (i != GRUB_AT_TRIES);
 }
 
@@ -132,31 +156,66 @@ static int
 query_mode (void)
 {
   grub_uint8_t ret;
+  grub_uint64_t endtime;
+  unsigned i;
   int e;
+  char *envvar;
 
-  e = write_mode (0);
-  if (!e) {
-    grub_dprintf("atkeyb", "query_mode: write_mode(0) failed\n");
-    return 0;
-  }
+  for (i = 0; i < GRUB_AT_TRIES; i++) {
+    grub_dprintf ("atkeyb", "query_mode: sending command to controller\n");
+    e = write_mode (0);
+    if (!e) {
+      grub_dprintf ("atkeyb", "query_mode: write_mode(0) failed\n");
+      return 0;
+    }
 
-  do {
-    keyboard_controller_wait_until_ready ();
-    ret = grub_inb (KEYBOARD_REG_DATA);
-  } while (ret == GRUB_AT_ACK);
-  /* QEMU translates the set even in no-translate mode.  */
-  if (ret == 0x43 || ret == 1) {
-    grub_dprintf("atkeyb", "query_mode: returning 1 (ret=0x%x)\n", ret);
-    return 1;
-  }
-  if (ret == 0x41 || ret == 2) {
-    grub_dprintf("atkeyb", "query_mode: returning 2 (ret=0x%x)\n", ret);
-    return 2;
+    endtime = grub_get_time_ms () + 20;
+    do {
+      keyboard_controller_wait_until_ready ();
+      ret = grub_inb (KEYBOARD_REG_DATA);
+      grub_dprintf ("atkeyb", "query_mode/loop: read 0x%x from controller\n", ret);
+    } while ((ret == GRUB_AT_ACK || ret == GRUB_AT_NACK) && grub_get_time_ms () < endtime);
+    if (ret == 0xfe) {
+      grub_dprintf ("atkeyb", "query_mode: asking controller to resend last result\n");
+      ret = resend_last_result();
+      grub_dprintf ("atkeyb", "query_mode: read 0x%x from controller\n", ret);
+    }
+    /* QEMU translates the set even in no-translate mode.  */
+    if (ret == 0x43 || ret == 1) {
+      grub_dprintf ("atkeyb", "query_mode: controller returned 0x%x, returning 1\n", ret);
+      return 1;
+    }
+    if (ret == 0x41 || ret == 2) {
+      grub_dprintf ("atkeyb", "query_mode: controller returned 0x%x, returning 2\n", ret);
+      return 2;
+    }
+    if (ret == 0x3f || ret == 3) {
+      grub_dprintf ("atkeyb", "query_mode: controller returned 0x%x, returning 3\n", ret);
+      return 3;
+    }
+    grub_dprintf ("atkeyb", "query_mode: controller returned unexpected value 0x%x, retrying\n", ret);
   }
-  if (ret == 0x3f || ret == 3) {
-    grub_dprintf("atkeyb", "query_mode: returning 3 (ret=0x%x)\n", ret);
-    return 3;
+
+  /*
+   * Falling here means we tried querying and the controller returned something
+   * we don't understand, try to use 'at_keyboard_fallback_set' if it exists,
+   * otherwise return 0.
+   */
+  envvar = grub_env_get ("at_keyboard_fallback_set");
+  if (envvar) {
+    fallback_set = grub_strtoul (envvar, 0, 10);
+    if ((grub_errno) || (fallback_set < 1) || (fallback_set > 3)) {
+      grub_dprintf ("atkeyb", "WARNING: ignoring unexpected value '%s' for '%s' variable\n",
+		    envvar, "at_keyboard_fallback_set");
+      fallback_set = 0;
+    } else {
+      grub_dprintf ("atkeyb", "query_mode: '%s' specified in environment, returning %d\n",
+		    "at_keyboard_fallback_set", fallback_set);
+    }
+    return fallback_set;
   }
+  grub_dprintf ("atkeyb", "WARNING: no '%s' specified in environment, returning 0\n",
+		"at_keyboard_fallback_set");
   return 0;
 }
 
@@ -165,14 +224,25 @@ set_scancodes (void)
 {
   /* You must have visited computer museum. Keyboard without scancode set
      knowledge. Assume XT. */
-  if (!grub_keyboard_orig_set)
-    {
-      grub_dprintf ("atkeyb", "No sets support assumed\n");
-      ps2_state.current_set = 1;
+  if (!grub_keyboard_orig_set) {
+    if (fallback_set) {
+      grub_dprintf ("atkeyb", "No sets support assumed but set forced to %d\n", fallback_set);
+      ps2_state.current_set = fallback_set;
       return;
     }
+    grub_dprintf ("atkeyb", "No sets support assumed, forcing to set 1\n");
+    ps2_state.current_set = 1;
+    return;
+  }
 
 #if !USE_SCANCODE_SET
+  if (fallback_set) {
+    grub_dprintf ("atkeyb", "queried set is %d but set forced to %d\n",
+		  grub_keyboard_orig_set, fallback_set);
+    ps2_state.current_set = fallback_set;
+    return;
+  }
+
   if ((grub_keyboard_controller_orig & KEYBOARD_AT_TRANSLATE) == KEYBOARD_AT_TRANSLATE) {
     grub_dprintf ("atkeyb", "queried set is %d but keyboard in Translate mode, so actually in set 1\n", grub_keyboard_orig_set);
     ps2_state.current_set = 1;
@@ -261,6 +331,7 @@ grub_at_keyboard_getkey (struct grub_term_input *term __attribute__ ((unused)))
 static void
 grub_keyboard_controller_init (void)
 {
+  grub_dprintf ("atkeyb", "initializing the controller\n");
   ps2_state.at_keyboard_status = 0;
   /* Drain input buffer. */
   while (1)
@@ -282,6 +353,7 @@ grub_keyboard_controller_init (void)
   grub_keyboard_controller_orig = grub_keyboard_controller_read ();
   grub_dprintf ("atkeyb", "grub_keyboard_controller_orig = 0x%x\n", grub_keyboard_controller_orig);
   grub_keyboard_orig_set = query_mode ();
+  grub_dprintf ("atkeyb", "grub_keyboard_orig_set = %d\n", grub_keyboard_orig_set);
 #endif
   set_scancodes ();
   keyboard_controller_led (ps2_state.led_status);
@@ -329,7 +401,6 @@ grub_at_restore_hw (void)
   return GRUB_ERR_NONE;
 }
 
-
 static struct grub_term_input grub_at_keyboard_term =
   {
     .name = "at_keyboard",
-- 
2.29.2


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]