keyboard: prevent races enabling/disabling kb scanning

keyboard_scan_enable() is called from several contexts.  From a skim of
the code I found:
* keyboard_lid_change(), which is called from HOOK_LID_CHANGE
* enable_keyboard(), which is called from HOOK_CHIPSET_RESUME
* lidangle_keyscan_update(), which is called from motion_sense_task.
* check_for_power_off_event() which is called from power_handle_state()
  which is called from chipset_task.
* power_button_interrupt(), which is an interrupt
* power_button_change_deferred(), which is a deferred function

So, ummm, it's probably not a good idea to do a read-modify-write of a
variable without any locking.  ...and then to act on the resultant state
in various different contexts.

It's presumed that's just what happened to poor Julius.  Julius found
himself in the unfortunate situation where he resumed his device (with
the power button, I believe) and that everything worked (including
reading the battery state and including the accelerometer) but the
keyboard didn't work.  Now, it should be noted that Julius is a little
strange.  Well, maybe he's not strange and maybe just the way he uses
his laptop is strange.  He uses his veyron_minnie device as a smart
keyboard/trackpad.  Said another way: it is in tablet mode but is docked
to an HDMI monitor, the screen is face flat on his table, and he uses
the builtin keyboard and trackpad.  Nobody else that I know does this.
It's pretty darn cool, but I just don't think anyone else would think of
it.  Anyway, that might have something to do with how he reproduced
this.  ...or it might not.  He does that a lot and hasn't seen the
problem before now.

Anyway, I managed to reproduce a number of problems similar to what poor
Julius saw by adding a 200ms sleep in keyboard_scan_enable() after we
read disable_scanning_mask but before we did anything to it (I skipped
the sleep if this happened to be one of those people who was calling
from interrupt).

Since there appears to be no spin_lock_irqsave() in the EC, let's just
have the EC use atomic operations to mess with its masks.  Then we'll
leave all heavy lifting to the task.

This requires thinking through the task code a bit.

Conflicts:
    common/keyboard_scan.c
...due to commit 6112f20679df ("common: keyboard_scan: Add items to
.bss.slow.") in ToT.

BRANCH=ToT
BUG=chrome-os-partner:48470
TEST=Poke a lot with power button and lid; NTF.

Change-Id: I61b906505100186b0ca2c48e7b1a7ffaaa8a7d3e
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/317896
Reviewed-by: Alec Berg <alecaberg@chromium.org>
Reviewed-by: Randall Spangler <rspangler@chromium.org>
(cherry picked from commit 98ab7484d331a78fced870b58b4d82e79e2e0f4e)
Reviewed-on: https://chromium-review.googlesource.com/318292

1 files changed, 46 insertions, 26 deletions

diff --git a/common/keyboard_scan.c b/common/keyboard_scan.c
index bccc601125..5d93f28fc9 100644
--- a/common/keyboard_scan.c
+++ b/common/keyboard_scan.c
@@ -96,37 +96,27 @@ static uint32_t __bss_slow post_scan_clock_us;
 static int __bss_slow print_state_changes;
 
 /* Must init to 0 for scanning at boot */
-static int __bss_slow disable_scanning_mask;
+static volatile uint32_t __bss_slow disable_scanning_mask;
 
 /* Constantly incrementing counter of the number of times we polled */
 static volatile int kbd_polls;
 
 static int keyboard_scan_is_enabled(void)
 {
+	/* NOTE: this is just an instantaneous glimpse of the variable. */
 	return !disable_scanning_mask;
 }
 
 void keyboard_scan_enable(int enable, enum kb_scan_disable_masks mask)
 {
-	int old_disable_scanning = disable_scanning_mask;
-
-	disable_scanning_mask = enable ? (disable_scanning_mask & ~mask) :
-					 (disable_scanning_mask | mask);
-
-	if (disable_scanning_mask != old_disable_scanning)
-		CPRINTS("KB disable_scanning_mask changed: 0x%08x",
-				disable_scanning_mask);
+	/* Access atomically */
+	if (enable)
+		atomic_clear((uint32_t *)&disable_scanning_mask, mask);
+	else
+		atomic_or((uint32_t *)&disable_scanning_mask, mask);
 
-	if (old_disable_scanning && !disable_scanning_mask) {
-		/*
-		 * Scanning is being enabled, so wake up the scanning task to
-		 * unlock the task_wait_event() loop after enable_interrupt().
-		 */
-		task_wake(TASK_ID_KEYSCAN);
-	} else if (disable_scanning_mask && !old_disable_scanning) {
-		keyboard_raw_drive_column(KEYBOARD_COLUMN_NONE);
-		keyboard_clear_buffer();
-	}
+	/* Let the task figure things out */
+	task_wake(TASK_ID_KEYSCAN);
 }
 
 /**
@@ -622,6 +612,7 @@ void keyboard_scan_task(void)
 {
 	timestamp_t poll_deadline, start;
 	int wait_time;
+	uint32_t local_disable_scanning = 0;
 
 	print_state(debounced_state, "init state");
 
@@ -633,22 +624,51 @@ void keyboard_scan_task(void)
 	while (1) {
 		/* Enable all outputs */
 		CPRINTS("KB wait");
-		if (keyboard_scan_is_enabled())
-			keyboard_raw_drive_column(KEYBOARD_COLUMN_ALL);
+
 		keyboard_raw_enable_interrupt(1);
 
 		/* Wait for scanning enabled and key pressed. */
-		do {
+		while (1) {
+			uint32_t new_disable_scanning;
+
+			/* Read it once to get consistent glimpse */
+			new_disable_scanning = disable_scanning_mask;
+
+			if (local_disable_scanning != new_disable_scanning)
+				CPRINTS("KB disable_scanning_mask changed: "
+					"0x%08x", new_disable_scanning);
+
+			if (!new_disable_scanning) {
+				/* Enabled now */
+				keyboard_raw_drive_column(KEYBOARD_COLUMN_ALL);
+			} else if (!local_disable_scanning) {
+				/*
+				 * Scanning isn't enabled but it was last time
+				 * we looked.
+				 *
+				 * No race here even though we're basing on a
+				 * glimpse of disable_scanning_mask since if
+				 * someone changes disable_scanning_mask they
+				 * are guaranteed to call task_wake() on us
+				 * afterward so we'll run the loop again.
+				 */
+				keyboard_raw_drive_column(KEYBOARD_COLUMN_NONE);
+				keyboard_clear_buffer();
+			}
+
+			local_disable_scanning = new_disable_scanning;
+
 			/*
-			 * Don't wait if scanning is enabled and a key is
+			 * Done waiting if scanning is enabled and a key is
 			 * already pressed.  This prevents a race between the
 			 * user pressing a key and enable_interrupt()
 			 * starting to pay attention to edges.
 			 */
-			if (!keyboard_raw_read_rows() ||
-					!keyboard_scan_is_enabled())
+			if (!local_disable_scanning && keyboard_raw_read_rows())
+				break;
+			else
 				task_wait_event(-1);
-		} while (!keyboard_scan_is_enabled());
+		}
 
 		/* Enter polling mode */
 		CPRINTS("KB poll");