diff options
author | Daisuke Nojiri <dnojiri@chromium.org> | 2022-09-15 16:02:51 -0700 |
---|---|---|
committer | Chromeos LUCI <chromeos-scoped@luci-project-accounts.iam.gserviceaccount.com> | 2022-11-28 19:10:20 +0000 |
commit | 4dee64a4c53dc954ca9695108c10ea1d0b578209 (patch) | |
tree | 2ba86e08d48d848ff6fec6c4f80dd4fe04c76fc3 | |
parent | dd451d0aab9c78f0fc7cff91c39a7f5c50f2790d (diff) | |
download | chrome-ec-4dee64a4c53dc954ca9695108c10ea1d0b578209.tar.gz |
i8042: Send command response from priority queue
This patch also makes KEYPROTO task hold sending a scancode when it
receives SETLEDS command until it returns ACK to the second byte (and
leaves STATE_ATKBD_SETLEDS).
This patch also removes and repositions some kblog_put calls because
checking OBF and writing to DBBOUT must be done as atomically as
possible to minimize the race condition.
BUG=b:237981131,b:247795316
BRANCH=None
TEST=Taniks. Press search+alt then 'k' 100 times.
Signed-off-by: Daisuke Nojiri <dnojiri@chromium.org>
Change-Id: I7ccfae99b3657ead5fa9e3c337db623aaffdb0bc
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/3901253
Reviewed-by: Aseda Aboagye <aaboagye@chromium.org>
Commit-Queue: Aseda Aboagye <aaboagye@chromium.org>
Code-Coverage: Zoss <zoss-cl-coverage@prod.google.com>
-rw-r--r-- | common/keyboard_8042.c | 111 | ||||
-rw-r--r-- | test/kb_8042.c | 101 |
2 files changed, 139 insertions, 73 deletions
diff --git a/common/keyboard_8042.c b/common/keyboard_8042.c index e0371ddd5c..ce27828e1d 100644 --- a/common/keyboard_8042.c +++ b/common/keyboard_8042.c @@ -78,6 +78,15 @@ enum scancode_set_list { #define KB_TO_HOST_RETRIES 3 /* + * Timeout for SETLEDS command. Kernel is supposed to send the second byte + * within this period. When timeout occurs, the second byte is received as + * 'Unsupported AT keyboard command 0x00' (or 0x04). You can evaluate your + * timeout is too long or too short by calculating the duration between 'KB + * SETLEDS' and 'Unsupported AT...'. + */ +#define SETLEDS_TIMEOUT (30 * MSEC) + +/* * Mutex to control write access to the to-host buffer head. Don't need to * mutex the tail because reads are only done in one place. */ @@ -87,6 +96,7 @@ static mutex_t to_host_mutex; enum { CHAN_KBD = 0, CHAN_AUX, + CHAN_CMD, }; struct data_byte { uint8_t chan; @@ -94,6 +104,7 @@ struct data_byte { }; static struct queue const to_host = QUEUE_NULL(16, struct data_byte); +static struct queue const to_host_cmd = QUEUE_NULL(16, struct data_byte); /* Queue command/data from the host */ enum { @@ -162,6 +173,7 @@ static int typematic_inter_delay; static int typematic_len; /* length of typematic_scan_code */ static uint8_t typematic_scan_code[MAX_SCAN_CODE_LEN]; static timestamp_t typematic_deadline; +static timestamp_t setleds_deadline; #define KB_SYSJUMP_TAG 0x4b42 /* "KB" */ #define KB_HOOK_VERSION 2 @@ -182,18 +194,16 @@ struct kblog_t { /* * Type: * - * s = byte enqueued to send to host * a = aux byte enqueued to send to host - * t = to-host queue tail pointer before type='s' bytes enqueued - * - * d = data byte from host * c = command byte from host - * - * k = to-host queue head pointer before byte dequeued - * K = byte actually sent to host via LPC - * A = byte actually sent to host via LPC as AUX - * + * d = data byte from host + * r = typematic + * s = byte enqueued to send to host + * t = to-host queue tail pointer before type='s' bytes enqueued + * u = byte enqueued to send to host with priority * x = to_host queue was cleared + * A = byte actually sent to host via LPC as AUX + * K = byte actually sent to host via LPC * * The to-host head and tail pointers are logged pre-wrapping to the * queue size. This means that they continually increment as units @@ -265,7 +275,7 @@ static void aux_enable_irq(int enable) * host cannot read the previous byte away in time. * * @param len Number of bytes to send to the host - * @param to_host Data to send + * @param bytes Data to send * @param chan Channel to send data on */ static void i8042_send_to_host(int len, const uint8_t *bytes, uint8_t chan, @@ -281,15 +291,29 @@ static void i8042_send_to_host(int len, const uint8_t *bytes, uint8_t chan, for (i = 0; i < len; i++) kblog_put('r', bytes[i]); } else { - for (i = 0; i < len; i++) - kblog_put(chan == CHAN_AUX ? 'a' : 's', bytes[i]); + struct queue const *queue = &to_host; + + if (chan == CHAN_CMD) + queue = &to_host_cmd; - if (queue_space(&to_host) >= len) { - kblog_put('t', to_host.state->tail); + for (i = 0; i < len; i++) { + char type; + + if (chan == CHAN_AUX) + type = 'a'; + else if (chan == CHAN_CMD) + type = 'u'; + else + type = 's'; + kblog_put(type, bytes[i]); + } + + if (queue_space(queue) >= len) { + kblog_put('t', queue->state->tail); for (i = 0; i < len; i++) { data.chan = chan; data.byte = bytes[i]; - queue_add_unit(&to_host, &data); + queue_add_unit(queue, &data); } } } @@ -417,6 +441,7 @@ void keyboard_clear_buffer(void) mutex_lock(&to_host_mutex); kblog_put('x', queue_count(&to_host)); queue_init(&to_host); + queue_init(&to_host_cmd); mutex_unlock(&to_host_mutex); lpc_keyboard_clear_buffer(); } @@ -661,6 +686,8 @@ static int handle_keyboard_data(uint8_t data, uint8_t *output) /* Chrome OS doesn't have keyboard LEDs, so ignore */ output[out_len++] = ATKBD_RET_ACK; data_port_state = STATE_ATKBD_SETLEDS; + setleds_deadline.val = get_time().val + SETLEDS_TIMEOUT; + CPRINTS5("KB SETLEDS"); break; case ATKBD_CMD_EX_SETLEDS: @@ -868,20 +895,23 @@ static void i8042_handle_from_host(void) struct host_byte h; int ret_len; uint8_t output[MAX_SCAN_CODE_LEN]; - uint8_t chan = CHAN_KBD; + uint8_t chan; while (queue_remove_unit(&from_host, &h)) { if (h.type == HOST_COMMAND) { ret_len = handle_keyboard_command(h.byte, output); + chan = CHAN_KBD; } else { CPRINTS5("KB recv data: 0x%02x", h.byte); kblog_put('d', h.byte); if (IS_ENABLED(CONFIG_8042_AUX) && - handle_mouse_data(h.byte, output, &ret_len)) + handle_mouse_data(h.byte, output, &ret_len)) { chan = CHAN_AUX; - else + } else { ret_len = handle_keyboard_data(h.byte, output); + chan = CHAN_CMD; + } } i8042_send_to_host(ret_len, output, chan, 0); @@ -925,10 +955,14 @@ void keyboard_protocol_task(void *u) i8042_handle_from_host(); /* Check if we have data to send to host */ - if (queue_is_empty(&to_host)) + if (queue_is_empty(&to_host) && + queue_is_empty(&to_host_cmd)) break; - /* Handle data waiting for host */ + /* + * Check if the output buffer is full. We can't proceed + * until the host read the data. + */ if (lpc_keyboard_has_char()) { /* If interrupts disabled, nothing we can do */ if (!i8042_keyboard_irq_enabled && @@ -946,26 +980,53 @@ void keyboard_protocol_task(void *u) * data? Send it another interrupt in case it * somehow missed the first one. */ - CPRINTS("KB extra IRQ"); + CPRINTS("KB host not responding"); lpc_keyboard_resume_irq(); retries = 0; break; } + /* + * We know DBBOUT is empty but we need act quickly as + * the host might be sending a byte to DBBIN. + * + * So be cautious if you're adding any code below up to + * lpc_keyboard_put_char since that'll increase the race + * condition. For example, you don't want to add CPRINTS + * or kblog_put. + * + * We should claim OBF=1 atomically to prevent the host + * from writing to DBBIN (i.e. set-ibf-if-not-obf). It's + * not possible for NPCX because NPCX's HIKMST-IBF is + * read-only. + */ + /* Get a char from buffer. */ - kblog_put('k', to_host.state->head); - queue_remove_unit(&to_host, &entry); + if (queue_count(&to_host_cmd)) { + queue_remove_unit(&to_host_cmd, &entry); + } else if (data_port_state == STATE_ATKBD_SETLEDS) { + /* to_host_cmd is empty but in SETLEDS */ + if (!timestamp_expired(setleds_deadline, &t)) + /* Let's wait for the 2nd byte. */ + break; + /* Didn't receive 2nd byte. Go back to CMD. */ + CPRINTS("KB SETLEDS timeout"); + data_port_state = STATE_ATKBD_CMD; + } else { + /* to_host isn't empty && not in SETLEDS */ + queue_remove_unit(&to_host, &entry); + } /* Write to host. */ if (entry.chan == CHAN_AUX && IS_ENABLED(CONFIG_8042_AUX)) { - kblog_put('A', entry.byte); lpc_aux_put_char(entry.byte, i8042_aux_irq_enabled); + kblog_put('A', entry.byte); } else { - kblog_put('K', entry.byte); lpc_keyboard_put_char( entry.byte, i8042_keyboard_irq_enabled); + kblog_put('K', entry.byte); } retries = 0; } diff --git a/test/kb_8042.c b/test/kb_8042.c index e7d7690cff..eb043d5f82 100644 --- a/test/kb_8042.c +++ b/test/kb_8042.c @@ -261,6 +261,7 @@ static int _read_cmd_byte(uint8_t *cmd) void before_test(void) { /* Make sure all tests start with the controller in the same state */ + keyboard_clear_buffer(); _write_cmd_byte(I8042_XLATE | I8042_AUX_DIS | I8042_KBD_DIS); } @@ -274,7 +275,7 @@ void after_test(void) } } -static int test_8042_aux_loopback(void) +test_static int test_8042_aux_loopback(void) { /* Disable all IRQs */ WRITE_CMD_BYTE(0); @@ -293,7 +294,7 @@ static int test_8042_aux_loopback(void) return EC_SUCCESS; } -static int test_8042_aux_two_way_communication(void) +test_static int test_8042_aux_two_way_communication(void) { /* Enable AUX IRQ */ WRITE_CMD_BYTE(I8042_ENIRQ12); @@ -311,7 +312,7 @@ static int test_8042_aux_two_way_communication(void) return EC_SUCCESS; } -static int test_8042_aux_inhibit(void) +test_static int test_8042_aux_inhibit(void) { /* Enable AUX IRQ, but inhibit the AUX device from sending data. */ WRITE_CMD_BYTE(I8042_ENIRQ12 | I8042_AUX_DIS); @@ -339,7 +340,7 @@ static int test_8042_aux_inhibit(void) return EC_SUCCESS; } -static int test_8042_aux_controller_commands(void) +test_static int test_8042_aux_controller_commands(void) { uint8_t ctrl; @@ -359,7 +360,7 @@ static int test_8042_aux_controller_commands(void) return EC_SUCCESS; } -static int test_8042_aux_test_command(void) +test_static int test_8042_aux_test_command(void) { i8042_write_cmd(I8042_TEST_MOUSE); @@ -368,7 +369,7 @@ static int test_8042_aux_test_command(void) return EC_SUCCESS; } -static int test_8042_self_test(void) +test_static int test_8042_self_test(void) { i8042_write_cmd(I8042_RESET_SELF_TEST); VERIFY_LPC_CHAR("\x55"); @@ -376,7 +377,7 @@ static int test_8042_self_test(void) return EC_SUCCESS; } -static int test_8042_keyboard_test_command(void) +test_static int test_8042_keyboard_test_command(void) { i8042_write_cmd(I8042_TEST_KB_PORT); VERIFY_LPC_CHAR("\x00"); /* Data and Clock are not stuck */ @@ -384,7 +385,7 @@ static int test_8042_keyboard_test_command(void) return EC_SUCCESS; } -static int test_8042_keyboard_controller_commands(void) +test_static int test_8042_keyboard_controller_commands(void) { uint8_t ctrl; @@ -404,7 +405,7 @@ static int test_8042_keyboard_controller_commands(void) return EC_SUCCESS; } -static int test_8042_keyboard_key_pressed_while_inhibited(void) +test_static int test_8042_keyboard_key_pressed_while_inhibited(void) { ENABLE_KEYSTROKE(1); @@ -440,7 +441,8 @@ static int test_8042_keyboard_key_pressed_while_inhibited(void) return EC_SUCCESS; } -static int test_8042_keyboard_key_pressed_before_inhibit_using_cmd_byte(void) +test_static int +test_8042_keyboard_key_pressed_before_inhibit_using_cmd_byte(void) { ENABLE_KEYSTROKE(1); /* Simulate a keypress on the keyboard */ @@ -472,7 +474,7 @@ static int test_8042_keyboard_key_pressed_before_inhibit_using_cmd_byte(void) return EC_SUCCESS; } -static int +test_static int test_8042_keyboard_key_pressed_before_inhibit_using_cmd_byte_with_read(void) { uint8_t cmd; @@ -524,7 +526,7 @@ test_8042_keyboard_key_pressed_before_inhibit_using_cmd_byte_with_read(void) return EC_SUCCESS; } -static int test_8042_keyboard_key_pressed_before_inhibit_using_cmd(void) +test_static int test_8042_keyboard_key_pressed_before_inhibit_using_cmd(void) { ENABLE_KEYSTROKE(1); /* Simulate a keypress on the keyboard */ @@ -556,7 +558,7 @@ static int test_8042_keyboard_key_pressed_before_inhibit_using_cmd(void) return EC_SUCCESS; } -static int test_single_key_press(void) +test_static int test_single_key_press(void) { ENABLE_KEYSTROKE(1); press_key(1, 1, 1); @@ -572,7 +574,7 @@ static int test_single_key_press(void) return EC_SUCCESS; } -static int test_disable_keystroke(void) +test_static int test_disable_keystroke(void) { ENABLE_KEYSTROKE(0); press_key(1, 1, 1); @@ -583,7 +585,7 @@ static int test_disable_keystroke(void) return EC_SUCCESS; } -static int test_typematic(void) +test_static int test_typematic(void) { ENABLE_KEYSTROKE(1); @@ -610,7 +612,7 @@ static int test_typematic(void) return EC_SUCCESS; } -static int test_atkbd_get_scancode(void) +test_static int test_atkbd_get_scancode(void) { SET_SCANCODE(1); @@ -635,7 +637,7 @@ static int test_atkbd_get_scancode(void) return EC_SUCCESS; } -static int test_atkbd_set_scancode_with_keystroke_disabled(void) +test_static int test_atkbd_set_scancode_with_keystroke_disabled(void) { ENABLE_KEYSTROKE(0); @@ -647,7 +649,7 @@ static int test_atkbd_set_scancode_with_keystroke_disabled(void) return EC_SUCCESS; } -static int test_atkbd_set_scancode_with_key_press_before_set(void) +test_static int test_atkbd_set_scancode_with_key_press_before_set(void) { ENABLE_KEYSTROKE(0); ENABLE_KEYSTROKE(1); @@ -660,26 +662,17 @@ static int test_atkbd_set_scancode_with_key_press_before_set(void) * ATKBD_CMD_SSCANSET should cause the keyboard to stop scanning, flush * the keyboards output queue, and reset the typematic key. */ - keyboard_host_write(ATKBD_CMD_SSCANSET, 0); - /* Read out the scan code that got pushed into the output buffer before - * the command was sent. - */ - VERIFY_LPC_CHAR("\x01"); + i8042_write_data(ATKBD_CMD_SSCANSET); + VERIFY_ATKBD_ACK(); /* * FIXME: This is wrong. The keyboard's output queue should have been * flushed when it received the `ATKBD_CMD_SSCANSET` command. */ - VERIFY_LPC_CHAR("\x81"); - - /* This is the ACK for `ATKBD_CMD_SSCANSET`. */ - VERIFY_ATKBD_ACK(); - - /* The keyboard has flushed the buffer so no more keys. */ - VERIFY_NO_CHAR(); + VERIFY_LPC_CHAR("\x01\x81"); /* Finish setting scan code 1 */ - keyboard_host_write(1, 0); + i8042_write_data(1); VERIFY_ATKBD_ACK(); /* Key scanning should be restored. */ @@ -690,7 +683,7 @@ static int test_atkbd_set_scancode_with_key_press_before_set(void) return EC_SUCCESS; } -static int test_atkbd_set_scancode_with_key_press_during_set(void) +test_static int test_atkbd_set_scancode_with_key_press_during_set(void) { ENABLE_KEYSTROKE(1); @@ -698,7 +691,7 @@ static int test_atkbd_set_scancode_with_key_press_during_set(void) * ATKBD_CMD_SSCANSET should cause the keyboard to stop scanning, flush * the keyboards output queue, and reset the typematic key. */ - keyboard_host_write(ATKBD_CMD_SSCANSET, 0); + i8042_write_data(ATKBD_CMD_SSCANSET); VERIFY_ATKBD_ACK(); /* These keypresses should be dropped. */ @@ -711,7 +704,7 @@ static int test_atkbd_set_scancode_with_key_press_during_set(void) VERIFY_LPC_CHAR("\x01\x81"); /* Finish setting scan code 1 */ - keyboard_host_write(1, 0); + i8042_write_data(1); VERIFY_ATKBD_ACK(); /* Key scanning should be restored. */ @@ -722,7 +715,7 @@ static int test_atkbd_set_scancode_with_key_press_during_set(void) return EC_SUCCESS; } -static int test_atkbd_echo(void) +test_static int test_atkbd_echo(void) { i8042_write_data(ATKBD_CMD_DIAG_ECHO); VERIFY_ATKBD_ACK(); @@ -732,7 +725,7 @@ static int test_atkbd_echo(void) return EC_SUCCESS; } -static int test_atkbd_get_id(void) +test_static int test_atkbd_get_id(void) { i8042_write_data(ATKBD_CMD_GETID); VERIFY_ATKBD_ACK(); @@ -747,8 +740,10 @@ static int test_atkbd_get_id(void) return EC_SUCCESS; } -static int test_atkbd_set_leds_keypress_during(void) +test_static int test_atkbd_set_leds_keypress_during(void) { + ENABLE_KEYSTROKE(1); + /* This should pause scanning. */ i8042_write_data(ATKBD_CMD_SETLEDS); VERIFY_ATKBD_ACK(); @@ -756,16 +751,20 @@ static int test_atkbd_set_leds_keypress_during(void) /* Simulate keypress while keyboard is waiting for option byte */ press_key(1, 1, 1); press_key(1, 1, 0); - /* FIXME: This is wrong, we shouldn't have any key strokes */ - VERIFY_LPC_CHAR("\x01\x81"); + + /* Scancode is kept in queue during SETLEDS. */ + VERIFY_NO_CHAR(); i8042_write_data(0x01); VERIFY_ATKBD_ACK(); + /* Scancode previously queued should be sent now. */ + VERIFY_LPC_CHAR("\x01\x81"); + return EC_SUCCESS; } -static int test_atkbd_set_leds_abort_set(void) +test_static int test_atkbd_set_leds_abort_set(void) { i8042_write_data(ATKBD_CMD_SETLEDS); VERIFY_ATKBD_ACK(); @@ -785,7 +784,7 @@ static int test_atkbd_set_leds_abort_set(void) return EC_SUCCESS; } -static int test_atkbd_set_ex_leds(void) +test_static int test_atkbd_set_ex_leds(void) { i8042_write_data(ATKBD_CMD_EX_SETLEDS); VERIFY_ATKBD_ACK(); @@ -801,9 +800,10 @@ static int test_atkbd_set_ex_leds(void) return EC_SUCCESS; } -static int test_scancode_set2(void) +test_static int test_scancode_set2(void) { SET_SCANCODE(2); + ENABLE_KEYSTROKE(1); WRITE_CMD_BYTE(READ_CMD_BYTE() | I8042_XLATE); press_key(1, 1, 1); @@ -820,7 +820,7 @@ static int test_scancode_set2(void) return EC_SUCCESS; } -static int test_power_button(void) +test_static int test_power_button(void) { ENABLE_KEYSTROKE(0); @@ -857,7 +857,7 @@ static int test_power_button(void) return EC_SUCCESS; } -static int test_sysjump(void) +test_static int test_sysjump(void) { SET_SCANCODE(2); ENABLE_KEYSTROKE(1); @@ -868,15 +868,17 @@ static int test_sysjump(void) return EC_ERROR_UNKNOWN; } -static int test_sysjump_cont(void) +test_static int test_sysjump_cont(void) { WRITE_CMD_BYTE(READ_CMD_BYTE() | I8042_XLATE); + press_key(1, 1, 1); VERIFY_LPC_CHAR("\x01"); press_key(1, 1, 0); VERIFY_LPC_CHAR("\x81"); WRITE_CMD_BYTE(READ_CMD_BYTE() & ~I8042_XLATE); + press_key(1, 1, 1); VERIFY_LPC_CHAR("\x76"); press_key(1, 1, 0); @@ -885,7 +887,7 @@ static int test_sysjump_cont(void) return EC_SUCCESS; } -static const struct ec_response_keybd_config keybd_config = { +test_static const struct ec_response_keybd_config keybd_config = { .num_top_row_keys = 13, .action_keys = { TK_BACK, /* T1 */ @@ -910,7 +912,7 @@ board_vivaldi_keybd_config(void) return &keybd_config; } -static int test_ec_cmd_get_keybd_config(void) +test_static int test_ec_cmd_get_keybd_config(void) { struct ec_response_keybd_config resp; int rv; @@ -931,22 +933,25 @@ static int test_ec_cmd_get_keybd_config(void) return EC_SUCCESS; } -static int test_vivaldi_top_keys(void) +test_static int test_vivaldi_top_keys(void) { SET_SCANCODE(2); /* Test REFRESH key */ WRITE_CMD_BYTE(READ_CMD_BYTE() | I8042_XLATE); + press_key(2, 3, 1); /* Press T2 */ VERIFY_LPC_CHAR("\xe0\x67"); /* Check REFRESH scancode in set-1 */ /* Test SNAPSHOT key */ WRITE_CMD_BYTE(READ_CMD_BYTE() | I8042_XLATE); + press_key(4, 3, 1); /* Press T2 */ VERIFY_LPC_CHAR("\xe0\x13"); /* Check SNAPSHOT scancode in set-1 */ /* Test VOL_UP key */ WRITE_CMD_BYTE(READ_CMD_BYTE() | I8042_XLATE); + press_key(5, 3, 1); /* Press T2 */ VERIFY_LPC_CHAR("\xe0\x30"); /* Check VOL_UP scancode in set-1 */ |