motion: Control on which task sensor setting functions are running on

Analysis of motionsense shutdown revealed a race condition,
as function that change sensor state can run on parallel tasks, namely
CHIPSET and HOOK:

motion_sense_shutdown (CHIPSET) ---> (motion_sense_switch_sensor_rate)

suspend/resume (HOOK) ----> (motion_sense_switch_sensor_rate)

motion_sense_process (MOTIONSENSE) ---> motion_sense_set_data_rate (ACTIVE)
                                    \-> motion_sense_set_motion_intervals

          /----------- motion_sense_init --\
          |                                |
motion_sense_switch_sensor_rate ---> motion_sense_set_data_rate (ACTIVE)
          |                                  \----------- sensor->collection_rate = odr...; (ACTIVE)
          \------------> sensor->collection_rate = 0;   (INACTIVE)
           \-----------> motion_sense_set_motion_intervals

Running motion_sense_switch_sensor_rate() on HOOK task is necessary because
on some platform, the power line may be already off, when the device is going to S5.

- Always run motion_sense_switch_sensor_rate() on hook.
- When changing ODR is needed (sensor active), schedule MOTIONSENSE task.

The new sequencing is simplified:

suspend/resume/shutdown (HOOK) ----> (motion_sense_switch_sensor_rate)

          /----------- motion_sense_init --\
          |                                |
motion_sense_switch_sensor_rate ---> schedule MOTIONSENSE.
          \------------> sensor->collection_rate = 0;   (INACTIVE)

motion_sense_process (MOTIONSENSE) ---> motion_sense_set_data_rate (ACTIVE)
                                    \-> motion_sense_set_motion_intervals

BUG=b:170703322
BRANCH=kukui
TEST=Check on Volteer the sequence at suspend/resume/shutdown.

Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
Change-Id: I0238cae9b4720e487a1e70788296a4db1b1e186b
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/2553347
Reviewed-by: Aseda Aboagye <aaboagye@chromium.org>

1 files changed, 53 insertions, 31 deletions

diff --git a/common/motion_sense.c b/common/motion_sense.c
index df8168d31b..ea310a4f89 100644
--- a/common/motion_sense.c
+++ b/common/motion_sense.c
@@ -126,6 +126,8 @@ static enum sensor_config motion_sense_get_ec_config(void)
  *
  * Set the sensor data rate. It is altered when the AP change the data
  * rate or when the power state changes.
+ *
+ * NOTE: Always run in TASK_ID_MOTIONSENSE task.
  */
 int motion_sense_set_data_rate(struct motion_sensor_t *sensor)
 {
@@ -286,8 +288,6 @@ static int motion_sense_ec_rate(struct motion_sensor_t *sensor)
  *
  * Set the wake up interval for the motion sense thread.
  * It is set to the highest frequency one of the sensors need to be polled at.
- *
- * Note: Not static to be tested.
  */
 static void motion_sense_set_motion_intervals(void)
 {
@@ -311,18 +311,16 @@ static void motion_sense_set_motion_intervals(void)
 
 	ap_event_interval =
 		MAX(0, ec_int_rate - MOTION_SENSOR_INT_ADJUSTMENT_US);
-	/*
-	 * Wake up the motion sense task: we want to sensor task to take
-	 * in account the new period right away.
-	 */
-	task_wake(TASK_ID_MOTIONSENSE);
 }
 
+/* Note: Always run on HOOK task, trigger by events from CHIPSET task. */
 static inline int motion_sense_init(struct motion_sensor_t *sensor)
 {
 	int ret, cnt = 3;
 
 	BUILD_ASSERT(SENSOR_COUNT < 32);
+	ASSERT((task_get_current() == TASK_ID_HOOKS) ||
+	       (task_get_current() == TASK_ID_CONSOLE));
 
 	/* Initialize accelerometers. */
 	do {
@@ -333,7 +331,6 @@ static inline int motion_sense_init(struct motion_sensor_t *sensor)
 		sensor->state = SENSOR_INIT_ERROR;
 	} else {
 		sensor->state = SENSOR_INITIALIZED;
-		motion_sense_set_data_rate(sensor);
 	}
 
 	return ret;
@@ -371,6 +368,10 @@ static void motion_sense_switch_sensor_rate(void)
 {
 	int i, ret;
 	struct motion_sensor_t *sensor;
+	unsigned int sensor_setup_mask = 0;
+
+	ASSERT(task_get_current() == TASK_ID_HOOKS);
+
 	for (i = 0; i < motion_sensor_count; ++i) {
 		sensor = &motion_sensors[i];
 		if (SENSOR_ACTIVE(sensor)) {
@@ -379,15 +380,17 @@ static void motion_sense_switch_sensor_rate(void)
 			 * set.
 			 */
 			if (sensor->state == SENSOR_INITIALIZED) {
-				motion_sense_set_data_rate(sensor);
 				sensor->drv->set_range(sensor,
 						       sensor->current_range,
 						       1);
+				sensor_setup_mask |= BIT(i);
 			} else {
 				ret = motion_sense_init(sensor);
 				if (ret != EC_SUCCESS)
 					CPRINTS("%s: %d: init failed: %d",
 							sensor->name, i, ret);
+				else
+					sensor_setup_mask |= BIT(i);
 				/*
 				 * No tablet mode allowed if an accel
 				 * is not working.
@@ -402,31 +405,29 @@ static void motion_sense_switch_sensor_rate(void)
 		} else {
 			/* The sensors are being powered off */
 			if (sensor->state == SENSOR_INITIALIZED) {
+				/*
+				 * Use mutex to be sure we are not changing the
+				 * ODR in MOTIONSENSE, in case it is running.
+				 */
+				mutex_lock(&g_sensor_mutex);
 				sensor->collection_rate = 0;
+				mutex_unlock(&g_sensor_mutex);
 				sensor->state = SENSOR_NOT_INITIALIZED;
 			}
 		}
 	}
-	motion_sense_set_motion_intervals();
-}
-DECLARE_DEFERRED(motion_sense_switch_sensor_rate);
-
-static void motion_sense_shutdown(void)
-{
-	int i;
-	struct motion_sensor_t *sensor;
-	sensor_active = SENSOR_ACTIVE_S5;
-	for (i = 0; i < motion_sensor_count; i++) {
-		sensor = &motion_sensors[i];
-		/* Forget about changes made by the AP */
-		sensor->config[SENSOR_CONFIG_AP].odr = 0;
-		sensor->config[SENSOR_CONFIG_AP].ec_rate = 0;
-		sensor->current_range = sensor->default_range;
+	if (sensor_setup_mask) {
+		atomic_or(&odr_event_required, sensor_setup_mask);
+		task_set_event(TASK_ID_MOTIONSENSE,
+			       TASK_EVENT_MOTION_ODR_CHANGE, 0);
+	} else {
+		/* No sensor activated, reset host interval interval to 0. */
+		ap_event_interval = 0;
 	}
-	motion_sense_switch_sensor_rate();
 
 	/* Forget activities set by the AP */
-	if (IS_ENABLED(CONFIG_GESTURE_DETECTION)) {
+	if (IS_ENABLED(CONFIG_GESTURE_DETECTION) &&
+	    (sensor_active == SENSOR_ACTIVE_S5)) {
 		uint32_t enabled = 0, disabled, mask;
 
 		mask = CONFIG_GESTURE_DETECTION_MASK;
@@ -450,9 +451,28 @@ static void motion_sense_shutdown(void)
 				MOTIONSENSE_ACTIVITY_DOUBLE_TAP, 1, NULL);
 		}
 	}
-	/* disable the body detection since motion sensor is down */
-	if (IS_ENABLED(CONFIG_BODY_DETECTION))
-		body_detect_set_enable(false);
+}
+DECLARE_DEFERRED(motion_sense_switch_sensor_rate);
+
+static void motion_sense_shutdown(void)
+{
+	int i;
+	struct motion_sensor_t *sensor;
+
+	sensor_active = SENSOR_ACTIVE_S5;
+	for (i = 0; i < motion_sensor_count; i++) {
+		sensor = &motion_sensors[i];
+		/* Forget about changes made by the AP */
+		sensor->config[SENSOR_CONFIG_AP].odr = 0;
+		sensor->config[SENSOR_CONFIG_AP].ec_rate = 0;
+		sensor->current_range = sensor->default_range;
+	}
+
+	/*
+	 * Run motion_sense_switch_sensor_rate_data in the HOOK task,
+	 * To be sure no 2 rate changes happens in parralell.
+	 */
+	hook_call_deferred(&motion_sense_switch_sensor_rate_data, 0);
 }
 DECLARE_HOOK(HOOK_CHIPSET_SHUTDOWN, motion_sense_shutdown,
 	     MOTION_SENSE_HOOK_PRIO);
@@ -468,7 +488,7 @@ static void motion_sense_suspend(void)
 
 	sensor_active = SENSOR_ACTIVE_S3;
 
-	/* disable the body detection since motion sensor is suspended */
+	/* disable the body detection since AP is suspended */
 	if (IS_ENABLED(CONFIG_BODY_DETECTION))
 		body_detect_set_enable(false);
 	/*
@@ -671,6 +691,8 @@ static int motion_sense_process(struct motion_sensor_t *sensor,
 	int has_data_read = 0;
 	int sensor_num = sensor - motion_sensors;
 
+	ASSERT(task_get_current() == TASK_ID_MOTIONSENSE);
+
 	if (*event & TASK_EVENT_MOTION_ODR_CHANGE) {
 		const int sensor_bit = 1 << sensor_num;
 		int odr_pending = atomic_clear(&odr_event_required);
@@ -1129,7 +1151,7 @@ static enum ec_status host_cmd_motion_sense(struct host_cmd_handler_args *args)
 			 * in the FIFO. Flush it explicitly.
 			 */
 			atomic_or(&odr_event_required,
-				  1 << (sensor - motion_sensors));
+				  BIT(sensor - motion_sensors));
 			task_set_event(TASK_ID_MOTIONSENSE,
 					TASK_EVENT_MOTION_ODR_CHANGE, 0);
 		}