driver: lsm6dsm: Fix race condition and sensor labeling issues

This commit fixes two issues with the LSM6DSM driver:
  - it fixes a race condition in load_fifo() which could lead
    to samples being labeled at the wrong timestamp if the FIFO
    was cleared, and a new interrupt occurred, between reading the
    last sample, and pushing it to the motion sense queue

 - it drops samples even when the ODR is changed to the same final
   normalized rate, since the sensor is turned off/on regardless
   of this condition; additionally, it adds one more sample to the
   number of samples to be discarded, since I was experimentally
   seeing occasional spurious data in the first sample after a rate
   change.

BUG=b:124085261
TEST=on Meep with magnetometer stuffed, and CL:1470775
     enabled, run CTS sensor test cases

Change-Id: I9701d90fa4e86488840b776e2c7afe4dd89570e7
Signed-off-by: Enrico Granata <egranata@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1509718
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Tested-by: Gwendal Grignou <gwendal@chromium.org>
Reviewed-by: Gwendal Grignou <gwendal@chromium.org>
Reviewed-by: Diana Z <dzigterman@chromium.org>

2 files changed, 11 insertions, 11 deletions

diff --git a/driver/accelgyro_lsm6dsm.c b/driver/accelgyro_lsm6dsm.c
index d7b24a5add..cdf5535c7d 100644
--- a/driver/accelgyro_lsm6dsm.c
+++ b/driver/accelgyro_lsm6dsm.c
@@ -308,6 +308,7 @@ static void push_fifo_data(struct motion_sensor_t *accel, uint8_t *fifo,
 
 static int load_fifo(struct motion_sensor_t *s, const struct fstatus *fsts)
 {
+	uint32_t int_ts = last_interrupt_timestamp;
 	int err, left, length;
 	uint8_t fifo[FIFO_READ_LEN];
 
@@ -341,12 +342,13 @@ static int load_fifo(struct motion_sensor_t *s, const struct fstatus *fsts)
 			return err;
 
 		/*
-		 * Manage patterns and push data. Data should be pushed with the
-		 * timestamp of the last IRQ before the FIFO was read, so make a
-		 * copy of the current time in case another interrupt comes in
-		 * during processing.
+		 * Manage patterns and push data. Data is pushed with the
+		 * timestamp of the interrupt that got us into this function
+		 * in the first place. This avoids a potential race condition
+		 * where we empty the FIFO, and a new IRQ comes in between
+		 * reading the last sample and pushing it into the FIFO.
 		 */
-		push_fifo_data(s, fifo, length, last_interrupt_timestamp);
+		push_fifo_data(s, fifo, length, int_ts);
 		left -= length;
 	} while (left > 0);
 
@@ -474,7 +476,6 @@ int lsm6dsm_set_data_rate(const struct motion_sensor_t *s, int rate, int rnd)
 #ifdef CONFIG_ACCEL_FIFO
 	const struct motion_sensor_t *accel = LSM6DSM_MAIN_SENSOR(s);
 	struct lsm6dsm_data *private = LSM6DSM_GET_DATA(accel);
-	int old_normalized_rate = data->base.odr;
 #endif
 	int ret = EC_SUCCESS, normalized_rate = 0;
 	uint8_t ctrl_reg, reg_val = 0;
@@ -533,9 +534,8 @@ int lsm6dsm_set_data_rate(const struct motion_sensor_t *s, int rate, int rnd)
 	if (ret == EC_SUCCESS) {
 		data->base.odr = normalized_rate;
 #ifdef CONFIG_ACCEL_FIFO
-		if (data->base.odr != old_normalized_rate)
-			private->samples_to_discard[s->type] =
-				LSM6DSM_DISCARD_SAMPLES;
+		private->samples_to_discard[s->type] =
+			LSM6DSM_DISCARD_SAMPLES;
 		ret = fifo_enable(accel);
 		if (ret != EC_SUCCESS)
 			CPRINTS("Failed to enable FIFO. Error: %d", ret);
diff --git a/driver/accelgyro_lsm6dsm.h b/driver/accelgyro_lsm6dsm.h
index 1f76195339..f9a4deff59 100644
--- a/driver/accelgyro_lsm6dsm.h
+++ b/driver/accelgyro_lsm6dsm.h
@@ -351,10 +351,10 @@ struct lsm6dsm_data {
 /*
  * Note: The specific number of samples to discard depends on the filters
  * configured for the chip, as well as the ODR being set.  For most of our
- * allowed ODRs, 4 should suffice.
+ * allowed ODRs, 5 should suffice.
  * See: ST's LSM6DSM application notes (AN4987) Tables 17 and 19 for details
  */
-#define LSM6DSM_DISCARD_SAMPLES 4
+#define LSM6DSM_DISCARD_SAMPLES 5
 
 #define LSM6DSM_ST_DATA(g, type) (&(&(g))->st_data[(type)])