[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

kernel/1432: some mf2-kbd hangs the kernel while updating leds




>Number:         1432
>Category:       kernel
>Synopsis:       some mf2-kbd hangs the kernel while updating leds
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    bugs
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Thu Oct  5 11:30:02 MDT 2000
>Last-Modified:
>Originator:     Mathias Schmocker
>Organization:
        Mathias Schmocker              SMAT Engineering LLC
        Tel. +41 22 800 3400           Bvd Georges-Favon 20
        Fax. +41 22 800 3401           P.O. Box
        mailto:smat@acm.org            CH-1211 Geneva 11
        http://www.smat.ch/            Switzerland
>Release:        OpenBSD 2.8-beta (GENERIC) #29 updated 1-OCT-2000
>Environment:
        System      : OpenBSD 2.8, 2.7 also affected
        Architecture: OpenBSD.i386
        Machine     : i386
>Description:
        Keyboard leds update hangs the pcvt keyboard driver on some
        (half-broken) keyboards. It is an old but rare problem (see
        the /sys/arch/i382/isa/pcvt/ sources). I could reproduce it
        with one Cherry kbd (playing with SHIFT and CAPS_LOCK  keys
        at the login prompt). Another similar Cherry kbd was unaffected.
        This problem is not linked with the new cut and paste feature.
>How-To-Repeat:
        Play with NUM_LOCK, CAPS_LOCK and SCROLL_LOCK with some system
        activity (e.g. kernel compile) at the console.
        Starting X, then killing it increase the blink rate of the
        NUM_LOCK led at the console (feature).
>Fix:
        I patched the pcvt driver for having an interrupt driven led
        update.

        The following files are affected:

        /sys/arch/i386/isa/pcvt/pcvt_ext.c --> update_led(1), param gives
        origin of the call.
--- pcvt_ext.c.ORI      Sun Oct  1 16:43:48 2000
+++ pcvt_ext.c  Wed Oct  4 20:59:47 2000
@@ -2320,7 +2320,7 @@
 
        if(!newgrafx)
        {
-               update_led();   /* update led's */
+               update_led(1);  /* update led's */
 
                /* if we switch to a vt with force 24 lines mode and    */
                /* pure VT emulation and 25 rows charset, then we have  */
------ END OF PATCH

        /sys/arch/i386/isa/pcvt/pcvt_hdr.h --> update_led( u_char) prototype
        changed, param gives origin of the call.
--- pcvt_hdr.h.ORI      Wed Oct  4 19:59:54 2000
+++ pcvt_hdr.h  Wed Oct  4 21:03:21 2000
@@ -1021,7 +1021,7 @@
 void   toggl_bell ( struct video_state *svsp );
 void   toggl_columns ( struct video_state *svsp );
 void   toggl_sevenbit ( struct video_state *svsp );
-void   update_led ( void );
+void   update_led ( u_char );
 void   vga10_vga10 ( u_char *invga, u_char *outvga );
 void   vga10_vga14 ( u_char *invga, u_char *outvga );
 void   vga10_vga16 ( u_char *invga, u_char *outvga );
------- END OF PATCH

        /sys/arch/i386/isa/pcvt/pcvt_kbd.c --> the core of this patch
--- pcvt_kbd.c.ORI      Sun Oct  1 16:44:05 2000
+++ pcvt_kbd.c  Thu Oct  5 10:26:29 2000
@@ -79,6 +79,7 @@
 #include "pcvt_hdr.h"          /* global include */
 
 #define LEDSTATE_UPDATE_PENDING (1 << 3)
+#define LEDSTATE_UPDATING      (1 << 4)
 
 static void fkey1(void), fkey2(void),  fkey3(void),  fkey4(void);
 static void fkey5(void), fkey6(void),  fkey7(void),  fkey8(void);
@@ -168,6 +169,7 @@
  * in the interest of robustness.  It may be possible that interrupts
  * get lost other times as well.
  */
+ /* Previous comment obsolete, update_led() now interrupt driven */
 
 static int lost_intr_timeout_queued = 0;
 
@@ -188,26 +190,37 @@
  *     update keyboard led's
  *---------------------------------------------------------------------------*/
 void
-update_led(void)
+update_led(u_char resp)
 {
 #if !PCVT_NO_LED_UPDATE
        /* Don't update LED's unless necessary. */
 
-       int opri, new_ledstate, response1, response2;
+       int opri, new_ledstate;
 
        opri = spltty();
        new_ledstate = ((vsp->scroll_lock) | (vsp->num_lock * 2) |
                        (vsp->caps_lock * 4));
 
        if (new_ledstate != ledstate) {
-               ledstate = LEDSTATE_UPDATE_PENDING;
 
-               if (kbd_cmd(KEYB_C_LEDS) != 0) {
-                       printf("pcvt: kbd led cmd timeout\n");
+               if ((resp == KBD_SCROLL) || (resp == KBD_NUM) ||
+                   (resp == KBD_CAPS) ||
+                   ((resp == 1) && (!do_initialization)) ||
+                   ((resp == 2) && (!do_initialization)) ||
+                   ((resp == KEYB_R_RESEND) &&
+                       (ledstate == LEDSTATE_UPDATE_PENDING))) {
+
+                       if (kbd_cmd(KEYB_C_LEDS) != 0) {
+                               printf("pcvt: kbd led cmd timeout\n");
+                               goto bail;
+                       }
+                       ledstate = LEDSTATE_UPDATE_PENDING;
+                       if (resp == KEYB_R_RESEND)
+                               printf("pcvt: kbd led cmd resend\n");
                        goto bail;
                }
 
-               /*
+               /* 
                 * For some keyboards or keyboard controllers, it is an
                 * error to issue a command without waiting long enough
                 * for an ACK for the previous command.  The keyboard
@@ -228,19 +241,32 @@
                 * reconnects.  The keyboard hardware is very simple and
                 * well designed :-).
                 */
-               response1 = kbd_response();
 
-               if (kbd_cmd(new_ledstate) != 0) {
-                       printf("pcvt: kbd led data timeout\n");
+               /*
+                * Previous comment obsolete, update_led() now interrupt driven.
+                * The PCVT_UPDLED_LOSES_INTR code is not used and could
+                * be removed (crashes at boot time).
+                * Mathias Schmocker smat@acm.org 05-OCT-2000
+                */
+
+               if (((resp == KEYB_R_ACK) &&
+                       (ledstate == LEDSTATE_UPDATE_PENDING)) ||
+                   ((resp == KEYB_R_RESEND) &&
+                       (ledstate == LEDSTATE_UPDATING))) {
+
+                       if (kbd_cmd(new_ledstate) != 0) {
+                               printf("pcvt: kbd led data timeout\n");
+                               goto bail;
+                       }
+                       ledstate = LEDSTATE_UPDATING;
+                       if (resp == KEYB_R_RESEND)
+                               printf("pcvt:kbd led data resend\n");
                        goto bail;
                }
-               response2 = kbd_response();
 
-               if (response1 == KEYB_R_ACK && response2 == KEYB_R_ACK)
+               if ((resp == KEYB_R_ACK) && (ledstate == LEDSTATE_UPDATING)) {
                        ledstate = new_ledstate;
-               else {
-                       printf("pcvt: kbd led cmd not ack'd (resp %#x %#x)\n",
-                           response1, response2);
+                       goto bail;
                }
 
 #if PCVT_UPDLED_LOSES_INTR
@@ -885,7 +911,7 @@
                        more_chars = (u_char *)"\033OP"; /* PF1 */
                else {
                        vsp->num_lock ^= 1;
-                       update_led();
+                       update_led(KBD_NUM);
                }
                return (more_chars);
 
@@ -993,6 +1019,9 @@
                dt = inb(CONTROLLER_DATA);      /* yes, get data */
 #endif /* !PCVT_KBD_FIFO */
 
+               if ((dt == KEYB_R_ACK) || (dt == KEYB_R_RESEND))
+                       update_led(dt); /* handle ACK/NACK correctly in X */
+
                /*
                 * If x mode is active, only care for locking keys, then
                 * return the scan code instead of any key translation.
@@ -1050,15 +1079,17 @@
 
        /* lets look what we got */
        switch (dt) {
+       case KEYB_R_ACK:        /* acknowledge after command has rx'd*/
+       case KEYB_R_RESEND:     /* keyboard wants us to resend cmnd */
+               update_led(dt); /* handle ACK/NACK correctly, no X */
+               break;
        case KEYB_R_OVERRUN0:   /* keyboard buffer overflow */
 #if PCVT_SCANSET == 2
        case KEYB_R_SELFOK:     /* keyboard selftest ok */
 #endif /* PCVT_SCANSET == 2 */
        case KEYB_R_ECHO:       /* keyboard response to KEYB_C_ECHO */
-       case KEYB_R_ACK:        /* acknowledge after command has rx'd*/
        case KEYB_R_SELFBAD:    /* keyboard selftest FAILED */
        case KEYB_R_DIAGBAD:    /* keyboard self diagnostic failure */
-       case KEYB_R_RESEND:     /* keyboard wants us to resend cmnd */
        case KEYB_R_OVERRUN1:   /* keyboard buffer overflow */
                break;
 
@@ -1310,14 +1341,14 @@
        case KBD_CAPS:
                if (!kbd_status.breakseen && key != kbd_lastkey) {
                        vsp->caps_lock ^= 1;
-                       update_led();
+                       update_led(KBD_CAPS);
                }
                break;
 
        case KBD_SCROLL:
                if (!kbd_status.breakseen && key != kbd_lastkey) {
                        vsp->scroll_lock ^= 1;
-                       update_led();
+                       update_led(KBD_SCROLL);
 
                        if (!(vsp->scroll_lock))
                                wakeup((caddr_t)&(vsp->scroll_lock));
@@ -1370,7 +1401,7 @@
        vsp->scroll_lock = snc & 1;
        vsp->num_lock    = (snc & 2) ? 1 : 0;
        vsp->caps_lock   = (snc & 4) ? 1 : 0;
-       update_led();
+       update_led(1);
 }
 
 /*---------------------------------------------------------------------------*
@@ -1616,7 +1647,7 @@
 vt_keynum(struct video_state *svsp)
 {
        svsp->num_lock = 1;
-       update_led();
+       update_led(1);
 }
 
 /*---------------------------------------------------------------------------*
@@ -1626,7 +1657,7 @@
 vt_keyappl(struct video_state *svsp)
 {
        svsp->num_lock = 0;
-       update_led();
+       update_led(1);
 }
 
 /*---------------------------------------------------------------------------*
------- END OF PATCH

        /sys/arch/i386/isa/pcvt/pcvt_out.c --> update_led(1), param gives
        origin of the call.
--- pcvt_out.c.ORI      Sun Oct  1 16:44:13 2000
+++ pcvt_out.c  Wed Oct  4 21:14:37 2000
@@ -1219,7 +1219,7 @@
 
        /* update keyboard led's */
 
-       update_led();
+       update_led(1);
 }
 
 /*---------------------------------------------------------------------------*
------- END OF PATCH

        /sys/arch/i386/isa/pcvt/pcvt_vtf.c --> update_led(2), param gives
        origin of the call.
--- pcvt_vtf.c.ORI      Sun Oct  1 16:44:28 2000
+++ pcvt_vtf.c  Wed Oct  4 21:15:42 2000
@@ -557,7 +557,7 @@
        svsp->selchar = 0;                      /* selective attribute off */
        vt_initsel(svsp);
 
-       update_led();                           /* update keyboard LED's */
+       update_led(2);                          /* update keyboard LED's */
 }
 
 /*---------------------------------------------------------------------------*
------- END OF PATCH

        /sys/arch/i386/isa/pcvt/Doc/BugList --> bug hopefully solved.
--- BugList.ORI Wed Oct 18 09:50:40 1995
+++ BugList     Thu Oct  5 10:32:47 2000
@@ -1,5 +1,5 @@
 
-List of known bugs                    Last Edit-Date: [Tue Sep  5 17:52:05 1995]
+List of known bugs                    Last Edit-Date: [5-OCT-2000]
 ================================================================================
 
 
@@ -42,9 +42,9 @@
 chips other than C00,C10 and C11. C30
 chips are detected as C11s ....
 -------------------------------------------    --------------------------------
-On one keyboard, if a "Lock" key is pressed
-the leds do not get updated and the key-
-board hangs.
+On one keyboard, if a "Lock" key is pressed    update_led() in pcvt_kbd.c now
+the leds do not get updated and the key-       interrupt driven. 5-OCT-2000
+board hangs.                                   Mathias Schmocker smat@acm.org
 -------------------------------------------    --------------------------------
 HP function key labels code needs to set
 the user defined fkey string somehow!
------- END OF PATCH

        /sys/arch/i386/isa/pcvt/pcvt_drv.c was also changed, it is not
        directly related with the led update problem. I had a fifo overflow
        followed by an instant reboot of the system while banging at my
        keyboard and testing the changes. I'm not so sure of this patch,
        I could'nt reproduce the problem.
--- pcvt_drv.c.ORI      Sun Oct  1 16:43:38 2000
+++ pcvt_drv.c  Wed Oct  4 20:57:03 2000
@@ -469,8 +469,10 @@
 
                dt = inb(CONTROLLER_DATA);              /* get it 8042 data */
 
-               if (pcvt_kbd_count >= PCVT_KBD_FIFO_SZ) /* fifo overflow ? */
+               if (pcvt_kbd_count >= PCVT_KBD_FIFO_SZ) { /* fifo overflow ? */
                        log (LOG_WARNING, "pcvt: keyboard buffer overflow\n");
+                       pcvt_kbd_count--; /* SILENTLY DECR, AVOID REBOOT ? */
+               }
                else {
                        pcvt_kbd_fifo[pcvt_kbd_wptr++] = dt; /* data -> fifo */
------- END OF PATCH

        Please contact me for other infos.
        Mathias smat@acm.org

Note:	I'm sending it again with Netscape, the maillog said "Data format error" when sent
	with sendbug version 3.97:
-- BLAH
	Oct  5 13:01:01 polaris sendmail[4751]: e95B0vH04751: from=smat, size=10803, cla
	ss=0, nrcpts=2, msgid=<200010051100.e95B0vH04751@polaris.smat.ch>, relay=smat@lo
	calhost
	Oct  5 13:01:13 polaris sendmail[29566]: e95B0vH04751: to=gnats@openbsd.org, ctl
	addr=smat (1000/20), delay=00:00:16, xdelay=00:00:12, mailer=esmtp, pri=70803, r
	elay=openbsd.cs.colorado.edu. [128.138.192.83], dsn=5.6.0, stat=Data format erro
	r
	Oct  5 13:01:19 polaris sendmail[29566]: e95B0vH04751: to=smat@acm.org, ctladdr=
	smat (1000/20), delay=00:00:22, xdelay=00:00:06, mailer=esmtp, pri=70803, relay=
	mail.acm.org. [199.222.69.4], dsn=5.6.0, stat=Data format error
	Oct  5 13:01:19 polaris sendmail[29566]: e95B0vH04751: e95B1JG29566: DSN: Data f
	ormat error
-- END OF BLAH

>Audit-Trail:
>Unformatted: