From b60a51a20df1f4df0026a3683bb115d7b5e2c3f7 Mon Sep 17 00:00:00 2001 From: Enrico Granata Date: Thu, 6 Dec 2018 17:47:07 -0800 Subject: bmi160: Keep timestamp and FIFO synchronized The BMI160 will clear the FIFO watermark/full interrupt when the FIFO crosses the threshold condition - which happens upon reading from it in load_fifo(). As a result of this, and the fact that the soft IRQ handler for the BMI160 tries to loop until it has finished serving all interrupt causes, there are possibilities for a race condition such as the following to occur: read timestamp, value is t0 read interrupt cause: it's a watermark interrupt load fifo(t0): this clears the interrupt, allowing a new one to happen read timestamp, value is still t0 a watermark interrupt happens at t1 read interrupt cause: watermark load fifo(t0) At this point, the fifo will process events at t1 as-if they had happened at t0, which is bad and causes incorrect timestamps on the AP side. This changes moves reading the timestamp value as close as possible to load_fifo, and in a relative order where a new watermark interrupt cannot happen, which ensures the timestamp and the FIFO data for it are synchronized BUG=b:120508077 TEST=on Bobba and Nocturne, observe that no events happen where an identical "a" time matches two distinct (b,c) times BRANCH=nocturne,bobba Change-Id: I454b257366bccf2b9e4d78df5dc005a8ad7313a0 Signed-off-by: Enrico Granata Reviewed-on: https://chromium-review.googlesource.com/1367013 Reviewed-by: Alexandru M Stan Reviewed-by: Diana Z (cherry picked from commit e5fc358df19e1c4147d35749b6ddf91e22cebbf2) Reviewed-on: https://chromium-review.googlesource.com/c/1380711 Reviewed-by: Gwendal Grignou Reviewed-by: Philip Chen --- driver/accelgyro_bmi160.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/driver/accelgyro_bmi160.c b/driver/accelgyro_bmi160.c index 0913ba4731..b2c4a630dc 100644 --- a/driver/accelgyro_bmi160.c +++ b/driver/accelgyro_bmi160.c @@ -1095,20 +1095,19 @@ static void irq_set_orientation(struct motion_sensor_t *s, */ static int irq_handler(struct motion_sensor_t *s, uint32_t *event) { - uint32_t interrupt, last_ts; + uint32_t interrupt; + int rv; if ((s->type != MOTIONSENSE_TYPE_ACCEL) || (!(*event & CONFIG_ACCELGYRO_BMI160_INT_EVENT))) return EC_ERROR_NOT_HANDLED; do { - /* - * Collect timestamp before resetting the interrupt line: - * After reading, it is possible for the timestamp to change, - * before we can process the FIFO. - */ - last_ts = last_interrupt_timestamp; - raw_read32(s->port, s->addr, BMI160_INT_STATUS_0, &interrupt); + rv = raw_read32(s->port, s->addr, BMI160_INT_STATUS_0, + &interrupt); + /* Bail out of this loop if the sensor isn't powered. */ + if (rv == EC_ERROR_NOT_POWERED) + return rv; #ifdef CONFIG_GESTURE_SENSOR_BATTERY_TAP if (interrupt & BMI160_D_TAP_INT) @@ -1120,7 +1119,7 @@ static int irq_handler(struct motion_sensor_t *s, uint32_t *event) #endif #ifdef CONFIG_ACCEL_FIFO if (interrupt & (BMI160_FWM_INT | BMI160_FFULL_INT)) - load_fifo(s, last_ts); + load_fifo(s, last_interrupt_timestamp); #endif #ifdef CONFIG_BMI160_ORIENTATION_SENSOR irq_set_orientation(s, interrupt); -- cgit v1.2.1