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.

 Conflicts:
	common/motion_sense.c: On/off body not implemented,
                               still use #ifdef,
                               current_range not added.

Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/2553347
Reviewed-by: Aseda Aboagye <aaboagye@chromium.org>
(cherry picked from commit 104f5257a6b5527574904ec0bb94061f4790f423)
Change-Id: Ic36b62ce48c54b7e3c9f1d34fcf5d6a42195abcd
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/2567457

1 files changed, 62 insertions, 36 deletions

diff --git a/common/motion_sense.c b/common/motion_sense.c
index 7495a29fe0..f39ba4e6db 100644
--- a/common/motion_sense.c
+++ b/common/motion_sense.c
@@ -137,6 +137,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)
 {
@@ -293,8 +295,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)
 {
@@ -318,17 +318,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;
 
+	ASSERT((task_get_current() == TASK_ID_HOOKS) ||
+	       (task_get_current() == TASK_ID_CONSOLE));
+
 	/* Initialize accelerometers. */
 	do {
 		ret = sensor->drv->init(sensor);
@@ -338,7 +337,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;
@@ -377,12 +375,16 @@ 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)) {
 			/* Initialize or just back the odr previously set. */
 			if (sensor->state == SENSOR_INITIALIZED) {
-				motion_sense_set_data_rate(sensor);
+				sensor_setup_mask |= BIT(i);
 			} else {
 				ret = motion_sense_init(sensor);
 				if (ret != EC_SUCCESS) {
@@ -398,17 +400,59 @@ static void motion_sense_switch_sensor_rate(void)
 						tablet_set_mode(0);
 					}
 #endif
+				} else {
+					sensor_setup_mask |= BIT(i);
 				}
 			}
 		} 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();
+	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;
+	}
+
+	/* Forget activities set by the AP */
+#ifdef CONFIG_GESTURE_DETECTION_MASK
+	if (sensor_active == SENSOR_ACTIVE_S5) {
+		uint32_t enabled = 0, disabled;
+		uint32_t mask = CONFIG_GESTURE_DETECTION_MASK;
+
+		while (mask) {
+			i = get_next_bit(&mask);
+			sensor = &motion_sensors[i];
+			if (sensor->state != SENSOR_INITIALIZED)
+				continue;
+			sensor->drv->list_activities(sensor,
+					&enabled, &disabled);
+			/* exclude double tap, it is used internally. */
+			enabled &= ~BIT(MOTIONSENSE_ACTIVITY_DOUBLE_TAP);
+			while (enabled) {
+				int activity = get_next_bit(&enabled);
+				sensor->drv->manage_activity(
+						sensor, activity, 0, NULL);
+			}
+			/* Re-enable double tap in case AP disabled it */
+			sensor->drv->manage_activity(sensor,
+				MOTIONSENSE_ACTIVITY_DOUBLE_TAP, 1, NULL);
+		}
+	}
+#endif
 }
 DECLARE_DEFERRED(motion_sense_switch_sensor_rate);
 
@@ -416,9 +460,6 @@ static void motion_sense_shutdown(void)
 {
 	int i;
 	struct motion_sensor_t *sensor;
-#ifdef CONFIG_GESTURE_DETECTION_MASK
-	uint32_t enabled = 0, disabled, mask;
-#endif
 
 	sensor_active = SENSOR_ACTIVE_S5;
 	for (i = 0; i < motion_sensor_count; i++) {
@@ -427,29 +468,12 @@ static void motion_sense_shutdown(void)
 		sensor->config[SENSOR_CONFIG_AP].odr = 0;
 		sensor->config[SENSOR_CONFIG_AP].ec_rate = 0;
 	}
-	motion_sense_switch_sensor_rate();
 
-	/* Forget activities set by the AP */
-#ifdef CONFIG_GESTURE_DETECTION_MASK
-	mask = CONFIG_GESTURE_DETECTION_MASK;
-	while (mask) {
-		i = get_next_bit(&mask);
-		sensor = &motion_sensors[i];
-		if (sensor->state != SENSOR_INITIALIZED)
-			continue;
-		sensor->drv->list_activities(sensor,
-				&enabled, &disabled);
-		/* exclude double tap, it is used internally. */
-		enabled &= ~BIT(MOTIONSENSE_ACTIVITY_DOUBLE_TAP);
-		while (enabled) {
-			int activity = get_next_bit(&enabled);
-			sensor->drv->manage_activity(sensor, activity, 0, NULL);
-		}
-		/* Re-enable double tap in case AP disabled it */
-		sensor->drv->manage_activity(sensor,
-				MOTIONSENSE_ACTIVITY_DOUBLE_TAP, 1, NULL);
-	}
-#endif
+	/*
+	 * Run motion_sense_switch_sensor_rate_data in the HOOK task,
+	 * To be sure no 2 rate changes happens in parallel.
+	 */
+	hook_call_deferred(&motion_sense_switch_sensor_rate_data, 0);
 }
 DECLARE_HOOK(HOOK_CHIPSET_SHUTDOWN, motion_sense_shutdown,
 	     MOTION_SENSE_HOOK_PRIO);
@@ -661,6 +685,8 @@ static int motion_sense_process(struct motion_sensor_t *sensor,
 	int ret = EC_SUCCESS;
 	int is_odr_pending = 0;
 
+	ASSERT(task_get_current() == TASK_ID_MOTIONSENSE);
+
 	if (*event & TASK_EVENT_MOTION_ODR_CHANGE) {
 		const int sensor_bit = 1 << (sensor - motion_sensors);
 		int odr_pending = atomic_read_clear(&odr_event_required);
@@ -1137,7 +1163,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);
 		}