at_keyboard and checkkey()

[Robert Millan <rmh@aybabtu.com>]

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


Hi,

at_keyboard's checkkey() implementation is buggy.  It will flush the
controller's input buffer, but checkkey()'s semantics require that it's
not flushed.  This results in some keys being lost when using at_keyboard.

A proper fix that adjusts to this API is complicated.  I implemented one [1],
only to realize afterwards that maybe this mess isn't really necessary.

A quick check at grub_checkkey() users reveals that none of them care about
the actual key, even if grub_checkkey returns it.  They just want to know
whether there's a key available for read at all.  If we removed this
constraint from the checkkey() semantics, perhaps at_keyboard can get a
checkkey() with less hassle.

However, when at_keyboard reads an event, it needs to determine whether it's
a MAKE (key hit) or BREAK (key unhit) event.  Which AFAICT can't be done
without reading KEYBOARD_REG_DATA.  But maybe I'm missing something.

Any bright ideas?

[1] See attachment.  This made me implement queues as well, perhaps some of
    this can be reused.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

[-- Attachment #2: at_keyboard_queue.diff --]
[-- Type: text/x-diff, Size: 6763 bytes --]

Index: conf/i386.rmk
===================================================================
--- conf/i386.rmk	(revision 2607)
+++ conf/i386.rmk	(working copy)
@@ -6,7 +6,7 @@ cpuid_mod_CFLAGS = $(COMMON_CFLAGS)
 cpuid_mod_LDFLAGS = $(COMMON_LDFLAGS)
 
 pkglib_MODULES += at_keyboard.mod
-at_keyboard_mod_SOURCES = term/i386/pc/at_keyboard.c
+at_keyboard_mod_SOURCES = term/i386/pc/at_keyboard.c kern/queue.c
 at_keyboard_mod_CFLAGS = $(COMMON_CFLAGS)
 at_keyboard_mod_LDFLAGS = $(COMMON_LDFLAGS)
 
Index: kern/queue.c
===================================================================
--- kern/queue.c	(revision 0)
+++ kern/queue.c	(revision 0)
@@ -0,0 +1,53 @@
+/* queue.c - grub queue functions */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2009  Free Software Foundation, Inc.
+ *
+ *  GRUB is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <grub/queue.h>
+#include <grub/misc.h>
+#include <grub/mm.h>
+
+void
+grub_queue_push (grub_queue_t queue, grub_queue_item_t item)
+{
+  item->next = queue->head;
+  item->prev = NULL;
+  if (queue->head)
+    queue->head->prev = item;
+  queue->head = item;
+
+  if (! queue->tail)
+    /* Empty queue becomes non-empty.  */
+    queue->tail = item;
+}
+
+void *
+grub_queue_pop (grub_queue_t queue)
+{
+  grub_queue_item_t item;
+
+  item = queue->tail;
+  if (queue->tail)
+    queue->tail = item->prev;
+  if (queue->tail)
+    queue->tail->next = NULL;
+  else
+    /* Non-empty queue becomes empty.  */
+    queue->head = NULL;
+
+  return item;
+}
Index: include/grub/queue.h
===================================================================
--- include/grub/queue.h	(revision 0)
+++ include/grub/queue.h	(revision 0)
@@ -0,0 +1,52 @@
+/* queue.h - header for grub queue */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2009  Free Software Foundation, Inc.
+ *
+ *  GRUB is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef GRUB_QUEUE_HEADER
+#define GRUB_QUEUE_HEADER 1
+
+#include <grub/symbol.h>
+#include <grub/types.h>
+#include <grub/list.h>		/* GRUB_FIELD_MATCH */
+
+struct grub_queue_item
+{
+  struct grub_queue_item *next, *prev;
+};
+typedef struct grub_queue_item *grub_queue_item_t;
+
+struct grub_queue
+{
+  grub_queue_item_t head, tail;
+};
+typedef struct grub_queue *grub_queue_t;
+
+void EXPORT_FUNC(grub_queue_push) (grub_queue_t queue, grub_queue_item_t item);
+void * EXPORT_FUNC(grub_queue_pop) (grub_queue_t queue);
+
+extern void* grub_assert_fail (void);
+
+#define GRUB_AS_QUEUE_ITEM(ptr) \
+  (GRUB_FIELD_MATCH (ptr, grub_queue_item_t, next) && GRUB_FIELD_MATCH (ptr, grub_queue_item_t, prev) ? \
+   (grub_queue_item_t) ptr : grub_assert_fail ())
+
+#define GRUB_AS_QUEUE_ITEM_P(pptr) \
+  (GRUB_FIELD_MATCH (*pptr, grub_queue_item_t, next) && GRUB_FIELD_MATCH (*pptr, grub_queue_item_t, prev) ? \
+   (grub_queue_item_t *) (void *) pptr : grub_assert_fail ())
+
+#endif /* ! GRUB_QUEUE_HEADER */
Index: term/i386/pc/at_keyboard.c
===================================================================
--- term/i386/pc/at_keyboard.c	(revision 2607)
+++ term/i386/pc/at_keyboard.c	(working copy)
@@ -1,6 +1,6 @@
 /*
  *  GRUB  --  GRand Unified Bootloader
- *  Copyright (C) 2007,2008  Free Software Foundation, Inc.
+ *  Copyright (C) 2007,2008,2009  Free Software Foundation, Inc.
  *
  *  GRUB is free software: you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License as published by
@@ -22,7 +22,52 @@
 #include <grub/i386/io.h>
 #include <grub/misc.h>
 #include <grub/term.h>
+#include <grub/mm.h>
+#include <grub/queue.h>
 
+struct grub_at_keyboard_queue_item
+{
+  struct grub_at_keyboard_queue_item *next, *prev;
+  int key;
+};
+typedef struct grub_at_keyboard_queue_item *grub_at_keyboard_queue_item_t;
+
+static struct grub_queue grub_at_keyboard_queue;
+
+static void
+grub_at_keyboard_queue_push (int key)
+{
+  grub_at_keyboard_queue_item_t item;
+  item = grub_malloc (sizeof (*item));
+  item->key = key;
+  grub_queue_push (&grub_at_keyboard_queue, GRUB_AS_QUEUE_ITEM (item));
+}
+
+static int
+grub_at_keyboard_queue_pop (void)
+{
+  grub_at_keyboard_queue_item_t item;
+  int key;
+  item = grub_queue_pop (&grub_at_keyboard_queue);
+  if (item)
+    {
+      key = item->key;
+      grub_free (item);
+    }
+  else
+    key = -1;
+  return key;
+}
+
+static int
+grub_at_keyboard_queue_last (void)
+{
+  if (grub_at_keyboard_queue.tail)
+    return ((grub_at_keyboard_queue_item_t) grub_at_keyboard_queue.tail)->key;
+  else
+    return -1;
+}
+
 static short at_keyboard_status = 0;
 
 #define KEYBOARD_STATUS_SHIFT_L		(1 << 0)
@@ -146,7 +191,7 @@ grub_keyboard_getkey (void)
 
 /* If there is a character pending, return it; otherwise return -1.  */
 static int
-grub_at_keyboard_checkkey (void)
+grub_at_keyboard_getkey_noblock (void)
 {
   int code, key;
   code = grub_keyboard_getkey ();
@@ -186,10 +231,24 @@ static int
 	      key += 'a' - 'A';
 	  }
     }
-  return (int) key;
+  return key;
 }
 
 static int
+grub_at_keyboard_checkkey (void)
+{
+  int key;
+  key = grub_at_keyboard_getkey_noblock ();
+
+  /* If there was a key, queue it.  */
+  if (key != -1)
+    grub_at_keyboard_queue_push (key);
+
+  /* Return last key from queue.  */
+  return grub_at_keyboard_queue_last ();
+}
+
+static int
 grub_at_keyboard_getkey (void)
 {
   int key;
@@ -197,6 +256,10 @@ grub_at_keyboard_getkey (void)
     {
       key = grub_at_keyboard_checkkey ();
     } while (key == -1);
+
+  /* Remove it from the queue.  */
+  (void) grub_at_keyboard_queue_pop ();
+  
   return key;
 }