Re: Patch that fixes an 'at_keyboard' module issue (unreliable key presses)

[Michael Bideau]

Hi Paul,

Thank you very much for your feedback.

My answer is inline, below.

Le mardi 27 août 2019 à 11:57 +0200, Paul Menzel a écrit :
> Dear Michael,
> 
> 
> On 8/24/19 9:09 PM, Michael Bideau wrote:
> 
> > This patch fixes an issue that prevented the 'at_keyboard' module
> > to
> > work (for me).
> > 
> > The cause is a bad/wrong return value in the function
> > 'grub_at_keyboard_getkey()' in file 
> > 'grub-core/term/at_keyboard.c' at line 234.
> > 
> > 
> > ///////// patch /////////
> > diff --git a/grub-core/term/at_keyboard.c b/grub-
> > core/term/at_keyboard.c
> > index f0a986eb1..597111077 100644
> > --- a/grub-core/term/at_keyboard.c
> > +++ b/grub-core/term/at_keyboard.c
> > @@ -234,7 +234,7 @@ grub_at_keyboard_getkey (struct grub_term_input
> > *term __attribute__ ((unused)))
> >      return GRUB_TERM_NO_KEY;
> >  
> >    if (! KEYBOARD_ISREADY (grub_inb (KEYBOARD_REG_STATUS)))
> > -    return -1;
> > +    return GRUB_TERM_NO_KEY;
> >    at_key = grub_inb (KEYBOARD_REG_DATA);
> >    old_led = ps2_state.led_status;
> > ///////// end of patch /////////
> > 
> > 
> > My symptoms were to have an unresponsive keyboard: keys needed to
> > be
> > pressed 10x and more to
> > effectively be printed, sometimes generating multiple key presses
> > (after 1 or 2 sec of no printing).
> > Very problematic for typing passphrase in early stage (with
> > GRUB_ENABLE_CRYPTODISK).
> > When switching to 'console' terminal input, keyboard works
> > perfectly.
> > It also worked great with grub 2.02 packaged by Debian (2.02+dfsg1-
> > 20).
> > It was not an output issue, but an input one.
> 
> […]
> 
> Welcome, and thank you very much for your contribution.
> 
> I think I had a similar issue and tried to fix it in commit d3a3543a
> (normal/menu: Do not treat error values as key presses) [1], present
> in
> GRUB 2.04. Do you have that commit in your tree?

Yes I have this/your commit in my tree.

///////// your patch d3a3543a /////////
diff --git a/grub-core/normal/menu.c b/grub-core/normal/menu.c
index e7a83c2d6..d5e0c79a7 100644
--- a/grub-core/normal/menu.c
+++ b/grub-core/normal/menu.c
@@ -698,7 +698,8 @@ run_menu (grub_menu_t menu, int nested, int *auto_boot)
 
       c = grub_getkey_noblock ();
 
-      if (c != GRUB_TERM_NO_KEY)
+      /* Negative values are returned on error. */
+      if ((c != GRUB_TERM_NO_KEY) && (c > 0))
        {
          if (timeout >= 0)
            {
///////// end of your patch d3a3543a /////////

For what I understand, your patch seems to "only" address normal/menu
timeout issue (but I might be wrong, I'm still very new to grub2 code),
but mine fixes the keyboard (if input is 'at_keyboard') wherever it is
used in the normal/menu or in the grub terminal/shell or even with the
cryptomount utility (typing passphrase was my issue at the beginning).

To be clear, with the last checkout (commit 4e75b2ae3), that includes
your commit d3a3543a, I have my keyboard not working reliably (as described
previously), and with my patch it works perfectly (meaning in the grub
terminal/shell I can type commands seamlessly and even use different
keyboard layouts).

As I have described in my previous mail, I think it was just an
incomplete cut-paste, missing a small manual correction. So it might be
better to fix it anyway ☺

PS: there is a typo in my introduction of my patch: it is at the line
237 and not 234 as mentioned. Do you think I should answer to my first
email to say it (so people not reading this sub-thread will know)?

> 
> Kind regards,
> 
> Paul
> 
> 
> [1]: 
> https://git.savannah.gnu.org/cgit/grub.git/commit/?id=d3a3543a5666c1dd180ae6027948ca753dcffc18


Thank you again for your time and your review of my contribution.

Best regards,

Michael Bideau [FR|France]