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/+/47083
to review the following change.
Change subject: libpayload/keyboard: Revise keyboard_cmd() error handling ......................................................................
libpayload/keyboard: Revise keyboard_cmd() error handling
Even if we are careful, it's still possible that we read spurious data from the keyboard, e.g. keystrokes. Namely, when we send the reset/disable command, there is a race before the command is pro- cessed.
So we should always process data from the keyboard in a loop. We break it, when an ACK (0xfa) or a NAK (0xfe) is received, and warn on unexpected data unless it might be due to the mentioned race.
Change-Id: I60a2643a8ff4b9231c63bf970c8749c97c7d8926 Signed-off-by: Nico Huber nico.h@gmx.de --- M payloads/libpayload/drivers/i8042/keyboard.c 1 file changed, 24 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/47083/1
diff --git a/payloads/libpayload/drivers/i8042/keyboard.c b/payloads/libpayload/drivers/i8042/keyboard.c index a9b864b..c26babb 100644 --- a/payloads/libpayload/drivers/i8042/keyboard.c +++ b/payloads/libpayload/drivers/i8042/keyboard.c @@ -28,6 +28,7 @@ */
#include <stdbool.h> +#include <stdint.h>
#include <keycodes.h> #include <libpayload-config.h> @@ -173,9 +174,31 @@
static bool keyboard_cmd(unsigned char cmd) { + const uint64_t timeout_us = 1*1000*1000; + const uint64_t start_time = timer_us(0); + i8042_write_data(cmd);
- return i8042_wait_read_ps2() == 0xfa; + do { + const int ret = i8042_wait_read_ps2(); + switch (ret) { + case -1: + /* We have our own timout handling. */ + continue; + case 0xfa: + return true; + case 0xfe: + return false; + default: + /* Warn only if we already disabled keyboard input. */ + if (cmd != I8042_KBCMD_DEFAULT_DIS) + printf("WARNING: Keyboard sent spurious 0x%02x.\n", ret); + continue; + } + } while (timer_us(start_time) < timeout_us); + + printf("ERROR: Keyboard command timed out.\n"); + return false; }
bool keyboard_havechar(void)
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47083 )
Change subject: libpayload/keyboard: Revise keyboard_cmd() error handling ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47083/1/payloads/libpayload/drivers... File payloads/libpayload/drivers/i8042/keyboard.c:
https://review.coreboot.org/c/coreboot/+/47083/1/payloads/libpayload/drivers... PS1, Line 186: /* We have our own timout handling. */ 'timout' may be misspelled - perhaps 'timeout'?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47083 )
Change subject: libpayload/keyboard: Revise keyboard_cmd() error handling ......................................................................
Patch Set 1: Code-Review+1
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47083 )
Change subject: libpayload/keyboard: Revise keyboard_cmd() error handling ......................................................................
Patch Set 1: Code-Review+1
The lint scripts found a typo.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47083 )
Change subject: libpayload/keyboard: Revise keyboard_cmd() error handling ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47083/1/payloads/libpayload/drivers... File payloads/libpayload/drivers/i8042/keyboard.c:
https://review.coreboot.org/c/coreboot/+/47083/1/payloads/libpayload/drivers... PS1, Line 183: const int ret = i8042_wait_read_ps2(); Might look better if we'd query i8042_data_ready_ps2() directly. That would also give us better control on timeouts.
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/+/47083
to look at the new patch set (#2).
Change subject: libpayload/keyboard: Revise keyboard_cmd() error handling ......................................................................
libpayload/keyboard: Revise keyboard_cmd() error handling
Even if we are careful, it's still possible that we read spurious data from the keyboard, e.g. keystrokes. Namely, when we send the reset/disable command, there is a race before the command is pro- cessed.
So we should always process data from the keyboard in a loop. We break it, when an ACK (0xfa) or a NAK (0xfe) is received, and warn on unexpected data unless it might be due to the mentioned race.
This also gives us the opportunity to use command-specific timeouts which we take from Linux: 1s for the keyboard self-test (as there are keyboards that perform the test before acking the command) and 200ms for all other commands.
Change-Id: I60a2643a8ff4b9231c63bf970c8749c97c7d8926 Signed-off-by: Nico Huber nico.h@gmx.de --- M payloads/libpayload/drivers/i8042/keyboard.c 1 file changed, 26 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/47083/2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47083 )
Change subject: libpayload/keyboard: Revise keyboard_cmd() error handling ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47083/1/payloads/libpayload/drivers... File payloads/libpayload/drivers/i8042/keyboard.c:
https://review.coreboot.org/c/coreboot/+/47083/1/payloads/libpayload/drivers... PS1, Line 183: const int ret = i8042_wait_read_ps2();
Might look better if we'd query i8042_data_ready_ps2() directly. That […]
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47083 )
Change subject: libpayload/keyboard: Revise keyboard_cmd() error handling ......................................................................
Patch Set 2: Code-Review+2
Hung-Te Lin has submitted this change. ( https://review.coreboot.org/c/coreboot/+/47083 )
Change subject: libpayload/keyboard: Revise keyboard_cmd() error handling ......................................................................
libpayload/keyboard: Revise keyboard_cmd() error handling
Even if we are careful, it's still possible that we read spurious data from the keyboard, e.g. keystrokes. Namely, when we send the reset/disable command, there is a race before the command is pro- cessed.
So we should always process data from the keyboard in a loop. We break it, when an ACK (0xfa) or a NAK (0xfe) is received, and warn on unexpected data unless it might be due to the mentioned race.
This also gives us the opportunity to use command-specific timeouts which we take from Linux: 1s for the keyboard self-test (as there are keyboards that perform the test before acking the command) and 200ms for all other commands.
Change-Id: I60a2643a8ff4b9231c63bf970c8749c97c7d8926 Signed-off-by: Nico Huber nico.h@gmx.de Reviewed-on: https://review.coreboot.org/c/coreboot/+/47083 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, 26 insertions(+), 1 deletion(-)
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 22747ff..42d4fdc 100644 --- a/payloads/libpayload/drivers/i8042/keyboard.c +++ b/payloads/libpayload/drivers/i8042/keyboard.c @@ -28,6 +28,7 @@ */
#include <stdbool.h> +#include <stdint.h>
#include <keycodes.h> #include <libpayload-config.h> @@ -173,9 +174,33 @@
static bool keyboard_cmd(unsigned char cmd) { + const uint64_t timeout_us = cmd == I8042_KBCMD_RESET ? 1*1000*1000 : 200*1000; + const uint64_t start_time = timer_us(0); + i8042_write_data(cmd);
- return i8042_wait_read_ps2() == 0xfa; + do { + if (!i8042_data_ready_ps2()) { + udelay(50); + continue; + } + + const uint8_t data = i8042_read_data_ps2(); + switch (data) { + case 0xfa: + return true; + case 0xfe: + return false; + default: + /* Warn only if we already disabled keyboard input. */ + if (cmd != I8042_KBCMD_DEFAULT_DIS) + printf("WARNING: Keyboard sent spurious 0x%02x.\n", data); + break; + } + } while (timer_us(start_time) < timeout_us); + + printf("ERROR: Keyboard command timed out.\n"); + return false; }
bool keyboard_havechar(void)
-
Angel Pons (Code Review) -
build bot (Jenkins) (Code Review)
-
Hung-Te Lin (Code Review)
-
Nico Huber (Code Review)
-
Paul Menzel (Code Review)