Skip to content

Commit

Permalink
i8042: Send command response from priority queue
Browse files Browse the repository at this point in the history 
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 <[email protected]>
Change-Id: I7ccfae99b3657ead5fa9e3c337db623aaffdb0bc
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/3901253
Reviewed-by: Aseda Aboagye <[email protected]>
Commit-Queue: Aseda Aboagye <[email protected]>
Code-Coverage: Zoss <[email protected]>
  • Loading branch information
@dnojiri
dnojiri authored and Chromeos LUCI committed Nov 28, 2022
1 parent dd451d0 commit 4dee64a
Show file tree
Hide file tree
Showing 2 changed files with 139 additions and 73 deletions.
111 changes: 86 additions & 25 deletions common/keyboard_8042.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,15 @@ enum scancode_set_list {
/* Number of bytes host can get behind before we start generating extra IRQs */
#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.
Expand All @@ -87,13 +96,15 @@ static mutex_t to_host_mutex;
enum {
CHAN_KBD = 0,
CHAN_AUX,
CHAN_CMD,
};
struct data_byte {
uint8_t chan;
uint8_t 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 {
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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,
Expand All @@ -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);
}
}
}
Expand Down Expand Up @@ -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();
}
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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 &&
Expand All @@ -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;
}
Expand Down
Loading

0 comments on commit 4dee64a

Please sign in to comment.