diff options
author | Shawn Nematbakhsh <shawnn@chromium.org> | 2017-11-09 11:20:25 -0800 |
---|---|---|
committer | chrome-bot <chrome-bot@chromium.org> | 2017-11-10 18:45:50 -0800 |
commit | 14ef8d73f1df635c26477687033de3539f6591e9 (patch) | |
tree | db3e51c144dd39fb5cc10b833bbd4770ef7e94af /common | |
parent | cab93b613b1c47fa01b140b507ddb08f9d48b415 (diff) | |
download | chrome-ec-14ef8d73f1df635c26477687033de3539f6591e9.tar.gz |
keyboard_mkbp: Fix FIFO locking
keyboard_clear_buffer() should not trash FIFO contents without
synchronization from fifo_add() / fifo_remove(), otherwise a bad
FIFO state may ensue.
BUG=chromium:781554
BRANCH=gru
TEST=Verify KB is functional on kevin through suspend / resume, verify
keyboard functions as S3 wake.
Signed-off-by: Shawn Nematbakhsh <shawnn@chromium.org>
Change-Id: I5d28c72359f6e1ce8778725a15c51cdfcd8ab90b
Reviewed-on: https://chromium-review.googlesource.com/761300
Commit-Ready: Shawn N <shawnn@chromium.org>
Tested-by: Shawn N <shawnn@chromium.org>
Reviewed-by: Aseda Aboagye <aaboagye@chromium.org>
Diffstat (limited to 'common')
-rw-r--r-- | common/keyboard_mkbp.c | 40 |
1 files changed, 33 insertions, 7 deletions
diff --git a/common/keyboard_mkbp.c b/common/keyboard_mkbp.c index 785ea65d63..df2ae73045 100644 --- a/common/keyboard_mkbp.c +++ b/common/keyboard_mkbp.c @@ -55,7 +55,16 @@ static uint32_t fifo_start; /* first entry */ static uint32_t fifo_end; /* last entry */ static uint32_t fifo_entries; /* number of existing entries */ static struct ec_response_get_next_event fifo[FIFO_DEPTH]; -static struct mutex fifo_mutex; +/* + * Mutex for critical sections of mkbp_fifo_add(), which is called + * from various tasks. + */ +static struct mutex fifo_add_mutex; +/* + * Mutex for critical sections of fifo_remove(), which is called from the + * hostcmd task and from keyboard_clear_buffer(). + */ +static struct mutex fifo_remove_mutex; /* Button and switch state. */ static uint32_t mkbp_button_state; @@ -111,6 +120,7 @@ static int fifo_remove(uint8_t *buffp) { int size; + mutex_lock(&fifo_remove_mutex); if (!fifo_entries) { /* no entry remaining in FIFO : return last known state */ int last = (fifo_start + FIFO_DEPTH - 1) % FIFO_DEPTH; @@ -118,6 +128,7 @@ static int fifo_remove(uint8_t *buffp) size = get_data_size(fifo[last].event_type); memcpy(buffp, &fifo[last].data, size); + mutex_unlock(&fifo_remove_mutex); /* * Bail out without changing any FIFO indices and let the @@ -128,12 +139,15 @@ static int fifo_remove(uint8_t *buffp) } /* Return just the event data. */ - size = get_data_size(fifo[fifo_start].event_type); - memcpy(buffp, &fifo[fifo_start].data, size); /* skip over event_type. */ + if (buffp) { + size = get_data_size(fifo[fifo_start].event_type); + /* skip over event_type. */ + memcpy(buffp, &fifo[fifo_start].data, size); + } fifo_start = (fifo_start + 1) % FIFO_DEPTH; - atomic_sub(&fifo_entries, 1); + mutex_unlock(&fifo_remove_mutex); return EC_SUCCESS; } @@ -152,11 +166,22 @@ void mkbp_clear_fifo(void) CPRINTS("clear MKBP fifo"); + /* + * Order of these locks is important to prevent deadlock since + * mkbp_fifo_add() may call fifo_remove(). + */ + mutex_lock(&fifo_add_mutex); + mutex_lock(&fifo_remove_mutex); + fifo_start = 0; fifo_end = 0; + /* This assignment is safe since both mutexes are held. */ fifo_entries = 0; for (i = 0; i < FIFO_DEPTH; i++) memset(&fifo[i], 0, sizeof(struct ec_response_get_next_event)); + + mutex_unlock(&fifo_remove_mutex); + mutex_unlock(&fifo_add_mutex); } test_mockable int keyboard_fifo_add(const uint8_t *buffp) @@ -176,7 +201,9 @@ test_mockable int mkbp_fifo_add(uint8_t event_type, const uint8_t *buffp) (event_type == EC_MKBP_EVENT_KEY_MATRIX)) return EC_SUCCESS; + mutex_lock(&fifo_add_mutex); if (fifo_entries >= config.fifo_max_depth) { + mutex_unlock(&fifo_add_mutex); CPRINTS("MKBP common FIFO depth %d reached", config.fifo_max_depth); @@ -184,7 +211,6 @@ test_mockable int mkbp_fifo_add(uint8_t event_type, const uint8_t *buffp) } size = get_data_size(event_type); - mutex_lock(&fifo_mutex); fifo[fifo_end].event_type = event_type; memcpy(&fifo[fifo_end].data, buffp, size); fifo_end = (fifo_end + 1) % FIFO_DEPTH; @@ -196,9 +222,9 @@ test_mockable int mkbp_fifo_add(uint8_t event_type, const uint8_t *buffp) * another event just woke the host (and wake is already in progress). */ if (!mkbp_send_event(event_type) && fifo_entries == 1) - mkbp_clear_fifo(); + fifo_remove(NULL); - mutex_unlock(&fifo_mutex); + mutex_unlock(&fifo_add_mutex); return EC_SUCCESS; } |