driver: bmi160: Prevent crash when FIFO is not valid

When all BMI160 sensors are suspended, FIFO is invalid.
Put the test to check if all sensors are disable within the processing
loop: otherwise, the FIFO can become invalid while we are processing it.

Add printf to be sure we are not processing invalid FIFO.

Add a macro around ODR to really check the ODR rate, excluding the
roundup flag.

BRANCH=smaug
BUG=chrome-os-partner:44381
TEST=Using a special patch (see 44381#14) add delay
to simulate a loaded EC (like at resume).
Using a script flip-flop sensors frequency (to simulate suspend/resume).
Check that:
- we are not crashing anymore (we were before this patch)
- the driver is not hitting invalid FIFO content.

Change-Id: I7c9e86f5dcfc231ab89472a6ea03af22e2c2ac32
Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/297178
Reviewed-by: Alec Berg <alecaberg@chromium.org>

3 files changed, 57 insertions, 17 deletions

diff --git a/common/motion_sense.c b/common/motion_sense.c
index f2650fbc52..21066c7f40 100644
--- a/common/motion_sense.c
+++ b/common/motion_sense.c
@@ -81,14 +81,31 @@ void motion_sense_fifo_add_unit(struct ec_response_motion_sensor_data *data,
 	mutex_unlock(&g_sensor_mutex);
 
 	if (valid_data) {
-		int ap_odr = sensor->config[SENSOR_CONFIG_AP].odr &
-			~ROUND_UP_FLAG;
+		int ap_odr = BASE_ODR(sensor->config[SENSOR_CONFIG_AP].odr);
 		int rate = INT_TO_FP(sensor->drv->get_data_rate(sensor));
 
-		/* If the AP does not want sensor info, skip */
+		/*
+		 * If the AP does not want sensor info, skip.
+		 * It happens:
+		 * - only the EC needs the data
+		 * - when there is event waiting in the FIFO when the AP
+		 *   put the sensor in suspend.
+		 */
 		if (ap_odr == 0)
 			return;
 
+		/*
+		 * BM160 FIFO can return bad data (see chrome-os-partner:43339.
+		 * It looks like an accelrator event.
+		 * It can happen if we are in middle of setting a new ODR
+		 * while we are processing the FIFO.
+		 */
+		if (rate == 0) {
+			CPRINTS("%s: unexpected event: 0x%04x", sensor->name,
+				data->data[0]);
+			return;
+		}
+
 		/* Skip if EC is oversampling */
 		if (sensor->oversampling < 0) {
 			sensor->oversampling += fp_div(INT_TO_FP(1000), rate);
@@ -174,11 +191,11 @@ int motion_sense_set_data_rate(struct motion_sensor_t *sensor)
 
 	/* Check the AP setting first. */
 	if (sensor_active != SENSOR_ACTIVE_S5)
-		odr = sensor->config[SENSOR_CONFIG_AP].odr & ~ROUND_UP_FLAG;
+		odr = BASE_ODR(sensor->config[SENSOR_CONFIG_AP].odr);
 
 	/* check if the EC set the sensor ODR at a higher frequency */
 	config_id = motion_sense_get_ec_config();
-	ec_odr = sensor->config[config_id].odr & ~ROUND_UP_FLAG;
+	ec_odr = BASE_ODR(sensor->config[config_id].odr);
 	if (ec_odr > odr)
 		odr = ec_odr;
 	else
diff --git a/driver/accelgyro_bmi160.c b/driver/accelgyro_bmi160.c
index 3044fe1c66..b4cfaf43d9 100644
--- a/driver/accelgyro_bmi160.c
+++ b/driver/accelgyro_bmi160.c
@@ -430,7 +430,7 @@ static int set_data_rate(const struct motion_sensor_t *s,
 
 #ifdef CONFIG_ACCEL_FIFO
 	/* FIFO start collecting events if AP wants them */
-	if (s->config[SENSOR_CONFIG_AP].odr != 0)
+	if (BASE_ODR(s->config[SENSOR_CONFIG_AP].odr) != 0)
 		enable_fifo(s, 1);
 #endif
 
@@ -802,22 +802,44 @@ static int load_fifo(struct motion_sensor_t *s)
 	if (s->type != MOTIONSENSE_TYPE_ACCEL)
 		return EC_SUCCESS;
 
-	if (!(data->flags &
-	      (BMI160_FIFO_ALL_MASK << BMI160_FIFO_FLAG_OFFSET))) {
-		/*
-		 * Flush potiential left over:
-		 *
-		 * When sensor is resumed, we don't want to read old data.
-		 */
-		raw_write8(s->addr, BMI160_CMD_REG, BMI160_CMD_FIFO_FLUSH);
-		return EC_SUCCESS;
-	}
-
 	do {
 		enum fifo_state state = FIFO_HEADER;
 		uint8_t *bp = bmi160_buffer;
+		uint32_t beginning;
+
+		if (!(data->flags &
+		      (BMI160_FIFO_ALL_MASK << BMI160_FIFO_FLAG_OFFSET))) {
+			/*
+			 * The FIFO was disable while were processing it.
+			 *
+			 * Flush potential left over:
+			 * When sensor is resumed, we won't read old data.
+			 */
+			raw_write8(s->addr, BMI160_CMD_REG,
+				   BMI160_CMD_FIFO_FLUSH);
+			return EC_SUCCESS;
+		}
+
 		raw_read_n(s->addr, BMI160_FIFO_DATA, bmi160_buffer,
 				sizeof(bmi160_buffer));
+		beginning = *(uint32_t *)bmi160_buffer;
+		/*
+		 * FIFO is invalid when reading while the sensors are all
+		 * suspended.
+		 * Instead of returning the empty frame, it can return a
+		 * pattern that looks like a valid header: 84 or 40.
+		 * If we see those, assume the sensors have been disabled
+		 * while this thread was running.
+		 */
+		if (beginning == 0x84848484 ||
+		    (beginning & 0xdcdcdcdc) == 0x40404040) {
+			CPRINTS("Suspended FIFO: accel ODR/rate: %d/%d: 0x%08x",
+				BASE_ODR(s->config[SENSOR_CONFIG_AP].odr),
+				get_data_rate(s),
+				beginning);
+			return EC_SUCCESS;
+		}
+
 		while (!done && bp != BUFFER_END(bmi160_buffer)) {
 			switch (state) {
 			case FIFO_HEADER: {
diff --git a/include/motion_sense.h b/include/motion_sense.h
index d01c2b907e..4185573b41 100644
--- a/include/motion_sense.h
+++ b/include/motion_sense.h
@@ -47,6 +47,7 @@ enum sensor_config {
 #define MAX_MOTION_SENSE_WAIT_TIME (60000 * MSEC)
 
 #define ROUND_UP_FLAG (1 << 31)
+#define BASE_ODR(_odr) ((_odr) & ~ROUND_UP_FLAG)
 
 struct motion_data_t {
 	/*