Hello Felix Singer, Matt Delco, Patrick Georgi, Furquan Shaikh, Julius Werner, Angel Pons, Patrick Rudolph, EricR Lai,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/47085
to review the following change.
Change subject: libpayload/keyboard: Avoid races around input draining ......................................................................
libpayload/keyboard: Avoid races around input draining
Draining the keyboard's buffer is only possible when the keyboard port is enabled. We should also disable input scanning before, as the buffer could be filled again with new keystrokes otherwise.
Change-Id: Ibac9c0d04880ff4a3efda5ac53da2f9731f6602c Signed-off-by: Nico Huber nico.h@gmx.de --- M payloads/libpayload/drivers/i8042/keyboard.c 1 file changed, 5 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/47085/1
diff --git a/payloads/libpayload/drivers/i8042/keyboard.c b/payloads/libpayload/drivers/i8042/keyboard.c index f8b68ab..efe6bcd 100644 --- a/payloads/libpayload/drivers/i8042/keyboard.c +++ b/payloads/libpayload/drivers/i8042/keyboard.c @@ -385,11 +385,13 @@ if (!i8042_probe() || !i8042_has_ps2()) return;
- keyboard_drain_input(); - /* Enable first PS/2 port */ i8042_cmd(I8042_CMD_EN_KB);
+ /* Disable scanning */ + keyboard_cmd(I8042_KBCMD_DEFAULT_DIS); + keyboard_drain_input(); + /* * We only support scancode set 1. The controller translation * would translate from set 2 to 1 for us, so we try to configure @@ -417,10 +419,9 @@ if (!i8042_has_ps2()) return;
- keyboard_drain_input(); - /* Disable scanning */ keyboard_cmd(I8042_KBCMD_DEFAULT_DIS); + keyboard_drain_input();
/* Send keyboard disconnect command */ i8042_cmd(I8042_CMD_DIS_KB);
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47085 )
Change subject: libpayload/keyboard: Avoid races around input draining ......................................................................
Patch Set 1: Code-Review+1
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47085 )
Change subject: libpayload/keyboard: Avoid races around input draining ......................................................................
Patch Set 1: Code-Review+1
Hello Felix Singer, build bot (Jenkins), Matt Delco, Patrick Georgi, Furquan Shaikh, Paul Menzel, Angel Pons, Julius Werner, Patrick Rudolph, EricR Lai,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47085
to look at the new patch set (#2).
Change subject: libpayload/keyboard: Avoid races around input draining ......................................................................
libpayload/keyboard: Avoid races around input draining
Draining the keyboard's buffer is only possible when the keyboard port is enabled. We should also disable input scanning before, as the buffer could be filled again with new keystrokes otherwise.
Change-Id: Ibac9c0d04880ff4a3efda5ac53da2f9731f6602c Signed-off-by: Nico Huber nico.h@gmx.de --- M payloads/libpayload/drivers/i8042/keyboard.c 1 file changed, 5 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/47085/2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47085 )
Change subject: libpayload/keyboard: Avoid races around input draining ......................................................................
Patch Set 2: Code-Review+2
I'm not sure if the double `keyboard_drain_input` is still needed, or removed later. We'll see.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47085 )
Change subject: libpayload/keyboard: Avoid races around input draining ......................................................................
Patch Set 2:
I'm not sure if the double `keyboard_drain_input` is still needed, or removed later. We'll see.
You mean inside keyboard_disconnect()? I left that there. Might be a bit overcautious, obviously there is nothing supposed to be in the hardware buffers and we didn't run anything that could have filled the fifo.
Hung-Te Lin has submitted this change. ( https://review.coreboot.org/c/coreboot/+/47085 )
Change subject: libpayload/keyboard: Avoid races around input draining ......................................................................
libpayload/keyboard: Avoid races around input draining
Draining the keyboard's buffer is only possible when the keyboard port is enabled. We should also disable input scanning before, as the buffer could be filled again with new keystrokes otherwise.
Change-Id: Ibac9c0d04880ff4a3efda5ac53da2f9731f6602c Signed-off-by: Nico Huber nico.h@gmx.de Reviewed-on: https://review.coreboot.org/c/coreboot/+/47085 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Angel Pons th3fanbus@gmail.com --- M payloads/libpayload/drivers/i8042/keyboard.c 1 file changed, 5 insertions(+), 4 deletions(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved
diff --git a/payloads/libpayload/drivers/i8042/keyboard.c b/payloads/libpayload/drivers/i8042/keyboard.c index 46afdc0..9da3902 100644 --- a/payloads/libpayload/drivers/i8042/keyboard.c +++ b/payloads/libpayload/drivers/i8042/keyboard.c @@ -374,11 +374,13 @@ if (!i8042_probe() || !i8042_has_ps2()) return;
- keyboard_drain_input(); - /* Enable first PS/2 port */ i8042_cmd(I8042_CMD_EN_KB);
+ /* Disable scanning */ + keyboard_cmd(I8042_KBCMD_DEFAULT_DIS); + keyboard_drain_input(); + i8042_set_kbd_translation(false);
if (set_scancode_set(2)) @@ -404,10 +406,9 @@ if (!i8042_has_ps2()) return;
- keyboard_drain_input(); - /* Disable scanning */ keyboard_cmd(I8042_KBCMD_DEFAULT_DIS); + keyboard_drain_input();
/* Send keyboard disconnect command */ i8042_cmd(I8042_CMD_DIS_KB);
-
Angel Pons (Code Review) -
Hung-Te Lin (Code Review)
-
Nico Huber (Code Review)
-
Paul Menzel (Code Review)