Re: [SeaBIOS] [PATCH] ps2: disable the keyboard and mouse before flushing the queue
On Thu, Apr 25, 2013 at 07:08:26PM -0400, Kevin O'Connor wrote: > My understanding is that the i8042 has two queues - one for the > results of i8042 commands and one for the keyboard. Upon sending an > i8042 command any reads from PORT_PS2_DATA will return data from the > i8042 queue until that queue is drained. Only after that would a read > from PORT_PS2_DATA return data from the keyboard queue. Otherwise, > I'm not sure how one could ever reliably read the results from an > i8042 command. > > The above aside, we can't disable the keyboard before flushing, > because we don't know the i8042 exists at all. Also, we can't send an > i8042 command without verifying the i8042 is ready for a command (ie, > using i8042_command(I8042_CMD_KBD_DISABLE)). However, we can't really > do that until we know the i8042 exists - so it's a bit circular. Thinking about this further, I guess one could move up the existing keyboard disable command with the patch below. That should be safe even if there is no i8042. I'd still like to further understand why keyboard data would be misinterpreted as i8042 data though. -Kevin --- a/src/ps2port.c +++ b/src/ps2port.c @@ -424,6 +424,16 @@ ps2_keyboard_setup(void *data) if (ret) return; +// Disable keyboard and mouse events. +Ps2ctr = I8042_CTR_KBDDIS | I8042_CTR_AUXDIS; +ret = i8042_command(I8042_CMD_CTL_WCTR, &Ps2ctr); +if (ret) +return; + +ret = i8042_flush(); +if (ret) +return; + // Controller self-test. u8 param[2]; ret = i8042_command(I8042_CMD_CTL_TEST, param); @@ -443,9 +453,6 @@ ps2_keyboard_setup(void *data) return; } -// Disable keyboard and mouse events. -SET_LOW(Ps2ctr, I8042_CTR_KBDDIS | I8042_CTR_AUXDIS); - /* --- keyboard side */ /* reset keyboard and self test (keyboard side) */ @@ -479,7 +486,7 @@ ps2_keyboard_setup(void *data) return; // Keyboard Mode: disable mouse, scan code convert, enable kbd IRQ -SET_LOW(Ps2ctr, I8042_CTR_AUXDIS | I8042_CTR_XLATE | I8042_CTR_KBDINT); +Ps2ctr = I8042_CTR_AUXDIS | I8042_CTR_XLATE | I8042_CTR_KBDINT; /* Enable keyboard */ ret = ps2_kbd_command(ATKBD_CMD_ENABLE, NULL); ___ SeaBIOS mailing list SeaBIOS@seabios.org http://www.seabios.org/mailman/listinfo/seabios
Re: [SeaBIOS] [PATCH] ps2: disable the keyboard and mouse before flushing the queue
On Thu, Apr 25, 2013 at 10:33:57AM -0500, Anthony Liguori wrote:
> Kevin O'Connor writes:
> > Thanks. I don't understand why keyboard/mouse events would be a
> > problem. Those events should get discarded with the "Discarding ps2
> > data %02x (status=%02x)" message. (The role of the flush is to make
> > sure there are no i8042 command responses pending - I don't think
> > keyboard/mouse events are a problem.)
>
> This is much too late. I think what's happening is:
>
> >/* flush incoming keys */
> >int ret = i8042_flush();
> >if (ret)
> >return;
>
> This drains the PS/2 queue but if the keyboard is enabled, then more
> data can appear in the queue.
>
> >// Controller self-test.
> >u8 param[2];
> >ret = i8042_command(I8042_CMD_CTL_TEST, param);
> >if (ret)
> >return;
> >if (param[0] != 0x55) {
> >dprintf(1, "i8042 self test failed (got %x not 0x55)\n", param[0]);
> >return;
> >}
>
> The patch below in QEMU will effectively queue the injected queue
> keycode before the 0x55 that self-test returns. The command queue is
> shared with the keyboard and the i8042 so if the keyboard is active, you
> may get keyboard data before getting the command responses.
My understanding is that the i8042 has two queues - one for the
results of i8042 commands and one for the keyboard. Upon sending an
i8042 command any reads from PORT_PS2_DATA will return data from the
i8042 queue until that queue is drained. Only after that would a read
from PORT_PS2_DATA return data from the keyboard queue. Otherwise,
I'm not sure how one could ever reliably read the results from an
i8042 command.
The above aside, we can't disable the keyboard before flushing,
because we don't know the i8042 exists at all. Also, we can't send an
i8042 command without verifying the i8042 is ready for a command (ie,
using i8042_command(I8042_CMD_KBD_DISABLE)). However, we can't really
do that until we know the i8042 exists - so it's a bit circular.
-Kevin
___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios
[SeaBIOS] [PATCH] ps2: disable the keyboard and mouse before flushing the queue
If SeaBIOS is run as a payload via coreboot (and presumably as a
CSM), then it's possible the keyboard or mouse will still be
enabled. This can lead to data being queued even after the flush
function attempts to clear the queue.
Disabling the keyboard/mouse prior to flushing is pretty standard
in DOS programming so it's not surprising that it's needed here.
I believe this problem manifests with the Chromebook Pixel. People
have reported that sometimes the 'ESC to Select Boot Devices'
doesn't work. I can reproduce this faithfully by holding 'Ctrl-L'
in the firmware screen during SeaBIOS initialization.
I can't test this fix on an actual Pixel because I don't know how
to update SeaBIOS but I have tested the patch under QEMU.
Signed-off-by: Anthony Liguori
---
src/ps2port.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/src/ps2port.c b/src/ps2port.c
index 9b760fd..2169171 100644
--- a/src/ps2port.c
+++ b/src/ps2port.c
@@ -55,6 +55,12 @@ static int
i8042_flush(void)
{
dprintf(7, "i8042_flush\n");
+
+/* Disable the keyboard and mouse to prevent additional data from
+ * being queued. */
+outb(0xad, PORT_PS2_STATUS);
+outb(0xa7, PORT_PS2_STATUS);
+
int i;
for (i=0; ihttp://www.seabios.org/mailman/listinfo/seabios
Re: [SeaBIOS] [PATCH] ps2: disable the keyboard and mouse before flushing the queue
Kevin O'Connor writes:
> On Wed, Apr 24, 2013 at 08:32:09PM -0500, Anthony Liguori wrote:
>> If SeaBIOS is run as a payload via coreboot (and presumably as a
>> CSM), then it's possible the keyboard or mouse will still be
>> enabled. This can lead to data being queued even after the flush
>> function attempts to clear the queue.
>>
>> Disabling the keyboard/mouse prior to flushing is pretty standard
>> in DOS programming so it's not surprising that it's needed here.
>>
>> I believe this problem manifests with the Chromebook Pixel. People
>> have reported that sometimes the 'ESC to Select Boot Devices'
>> doesn't work. I can reproduce this faithfully by holding 'Ctrl-L'
>> in the firmware screen during SeaBIOS initialization.
>
> Thanks. I don't understand why keyboard/mouse events would be a
> problem. Those events should get discarded with the "Discarding ps2
> data %02x (status=%02x)" message. (The role of the flush is to make
> sure there are no i8042 command responses pending - I don't think
> keyboard/mouse events are a problem.)
This is much too late. I think what's happening is:
>/* flush incoming keys */
>int ret = i8042_flush();
>if (ret)
>return;
This drains the PS/2 queue but if the keyboard is enabled, then more
data can appear in the queue.
>// Controller self-test.
>u8 param[2];
>ret = i8042_command(I8042_CMD_CTL_TEST, param);
>if (ret)
>return;
>if (param[0] != 0x55) {
>dprintf(1, "i8042 self test failed (got %x not 0x55)\n", param[0]);
>return;
>}
The patch below in QEMU will effectively queue the injected queue
keycode before the 0x55 that self-test returns. The command queue is
shared with the keyboard and the i8042 so if the keyboard is active, you
may get keyboard data before getting the command responses.
>> I can't test this fix on an actual Pixel because I don't know how
>> to update SeaBIOS but I have tested the patch under QEMU.
>
> Are you able to reproduce the problem under QEMU?
I can with the following patch. It's very hard to get the right timing
manually because IO writes are much faster in QEMU. We also don't
emulate key repeat correctly in QEMU so just holding a key won't do it.
But the following patch is a reasonable approximation. kbd_put_keycode()
can happen at any time in QEMU (this is what VNC/GTK will call to inject
a keycode). This patch just makes it happen at exactly the right time
to reproduce the bug.
This replicates the same behavior in QEMU as I see on the Pixel.
(And the patch I sent fixes the problem with my hacked QEMU).
diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
index 08ceb9f..bc35dd6 100644
--- a/hw/input/pckbd.c
+++ b/hw/input/pckbd.c
@@ -26,6 +26,7 @@
#include "hw/i386/pc.h"
#include "hw/input/ps2.h"
#include "sysemu/sysemu.h"
+#include "ui/console.h"
/* debug PC keyboard */
//#define DEBUG_KBD
@@ -267,6 +268,7 @@ static void kbd_write_command(void *opaque, hwaddr addr,
break;
case KBD_CCMD_SELF_TEST:
s->status |= KBD_STAT_SELFTEST;
+kbd_put_keycode(0x1d);
kbd_queue(s, 0x55, 0);
break;
case KBD_CCMD_KBD_TEST:
Regards,
Anthony Liguori
> I'd guess someone on the coreboot list could describe how to reflash
> the pixel. I don't own one myself.
>
> -Kevin
___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios
Re: [SeaBIOS] [PATCH] ps2: disable the keyboard and mouse before flushing the queue
On Wed, Apr 24, 2013 at 08:32:09PM -0500, Anthony Liguori wrote: > If SeaBIOS is run as a payload via coreboot (and presumably as a > CSM), then it's possible the keyboard or mouse will still be > enabled. This can lead to data being queued even after the flush > function attempts to clear the queue. > > Disabling the keyboard/mouse prior to flushing is pretty standard > in DOS programming so it's not surprising that it's needed here. > > I believe this problem manifests with the Chromebook Pixel. People > have reported that sometimes the 'ESC to Select Boot Devices' > doesn't work. I can reproduce this faithfully by holding 'Ctrl-L' > in the firmware screen during SeaBIOS initialization. Thanks. I don't understand why keyboard/mouse events would be a problem. Those events should get discarded with the "Discarding ps2 data %02x (status=%02x)" message. (The role of the flush is to make sure there are no i8042 command responses pending - I don't think keyboard/mouse events are a problem.) > I can't test this fix on an actual Pixel because I don't know how > to update SeaBIOS but I have tested the patch under QEMU. Are you able to reproduce the problem under QEMU? I'd guess someone on the coreboot list could describe how to reflash the pixel. I don't own one myself. -Kevin ___ SeaBIOS mailing list SeaBIOS@seabios.org http://www.seabios.org/mailman/listinfo/seabios
