keyboard_scan: detect inconsistent state in read_matrix

Current read_matrix() function didn't handle keyboard state change
during the scanning loop.

For example, consider keys J(c6r4), L(c9r4), H(c6r1) and F9(c9r1), and the
following sequence:

- User presses and holds J
- Key scan task reads col 6, got state[6] = 0x10 (J)
- User presses H+J+L, ghost key F9 is also "pressed" at this point.
- Key scan task reads col 9, got state[9] = 0x12 (L+F9)
- state[6] and state[9] has only one common bit, so it passes has_ghosting
  check.
- EC thinks J+L+F9 clicked.

Implemented a simple heuristic to detect this case, and update
state[] array to something likely to be the state after the key press.

With this change, we will no longer distinguish J -> H+J+L and J->F9+J+L.
The latter used to be accepted but it'll be rejected by this change.

BUG=b:145405136
TEST=hold J and L, press H repeatedly, make sure F9 never triggered.
BRANCH=kukui,hatch
TEST=make run-kb_scan (uncomment kb_scan in test/build.mk).
TEST=The average wait_time is reduced about 100 usec on Nami:
1746 msec (old) v.s. 1636 msec (new).

Change-Id: Ia20d5a283639d291530e5983254f6163f5c3537f
Signed-off-by: Ting Shen <phoenixshen@google.com>
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/1955105
Reviewed-by: Daisuke Nojiri <dnojiri@chromium.org>
Commit-Queue: Ting Shen <phoenixshen@chromium.org>
Tested-by: Ting Shen <phoenixshen@chromium.org>

1 files changed, 48 insertions, 17 deletions

diff --git a/common/keyboard_scan.c b/common/keyboard_scan.c
index b0b182fb47..c19140cf0a 100644
--- a/common/keyboard_scan.c
+++ b/common/keyboard_scan.c
@@ -16,6 +16,7 @@
 #include "keyboard_protocol.h"
 #include "keyboard_raw.h"
 #include "keyboard_scan.h"
+#include "keyboard_test.h"
 #include "lid_switch.h"
 #include "switch.h"
 #include "system.h"
@@ -228,51 +229,81 @@ static void simulate_key(int row, int col, int pressed)
  * Used in pre-init, so must not make task-switching-dependent calls; udelay()
  * is ok because it's a spin-loop.
  *
- * @param state		Destination for new state (must be KEYBOARD_COLS_MAX long).
+ * @param state		Destination for new state (must be KEYBOARD_COLS_MAX
+ *			long).
  *
  * @return 1 if at least one key is pressed, else zero.
  */
 static int read_matrix(uint8_t *state)
 {
 	int c;
-	uint8_t r;
 	int pressed = 0;
 
+	/* 1. Read input pins */
 	for (c = 0; c < keyboard_cols; c++) {
 		/*
-		 * Stop if scanning becomes disabled. Note, scanning is enabled
-		 * on boot by default.
+		 * Skip if scanning becomes disabled. Clear the state
+		 * to make sure we don't mix new and old states in the
+		 * same array.
+		 *
+		 * Note, scanning is enabled on boot by default.
 		 */
-		if (!keyboard_scan_is_enabled())
-			break;
+		if (!keyboard_scan_is_enabled()) {
+			state[c] = 0;
+			continue;
+		}
 
 		/* Select column, then wait a bit for it to settle */
 		keyboard_raw_drive_column(c);
 		udelay(keyscan_config.output_settle_us);
 
 		/* Read the row state */
-		r = keyboard_raw_read_rows();
+		state[c] = keyboard_raw_read_rows();
+
+		/* Use simulated keyscan sequence instead if testing active */
+		if (IS_ENABLED(CONFIG_KEYBOARD_TEST))
+			state[c] = keyscan_seq_get_scan(c, state[c]);
+	}
+
+	/* 2. Detect transitional ghost */
+	for (c = 0; c < keyboard_cols; c++) {
+		int c2;
+
+		for (c2 = 0; c2 < c; c2++) {
+			/*
+			 * If two columns shares at least one key but their
+			 * states are different, maybe the state changed between
+			 * two "keyboard_raw_read_rows"s. If this happened,
+			 * update both columns to the union of them.
+			 *
+			 * Note that in theory we need to rescan from col 0 if
+			 * anything is updated, to make sure the newly added
+			 * bits does not introduce more inconsistency.
+			 * Let's ignore this rare case for now.
+			 */
+			if ((state[c] & state[c2]) && (state[c] != state[c2])) {
+				uint8_t merged = state[c] | state[c2];
+
+				state[c] = state[c2] = merged;
+			}
+		}
+	}
 
+	/* 3. Fix result */
+	for (c = 0; c < keyboard_cols; c++) {
 		/* Add in simulated keypresses */
-		r |= simulated_key[c];
+		state[c] |= simulated_key[c];
 
 		/*
 		 * Keep track of what keys appear to be pressed.  Even if they
 		 * don't exist in the matrix, they'll keep triggering
 		 * interrupts, so we can't leave scanning mode.
 		 */
-		pressed |= r;
+		pressed |= state[c];
 
 		/* Mask off keys that don't exist on the actual keyboard */
-		r &= keyscan_config.actual_key_mask[c];
-
-#ifdef CONFIG_KEYBOARD_TEST
-		/* Use simulated keyscan sequence instead if testing active */
-		r = keyscan_seq_get_scan(c, r);
-#endif
+		state[c] &= keyscan_config.actual_key_mask[c];
 
-		/* Store the masked state */
-		state[c] = r;
 	}
 
 	keyboard_raw_drive_column(KEYBOARD_COLUMN_NONE);