From ccbbd386e55348864394713cbacc550c40b7f7fc Mon Sep 17 00:00:00 2001 From: Gwendal Grignou Date: Fri, 20 Nov 2020 13:54:03 -0800 Subject: 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 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/2553347 Reviewed-by: Aseda Aboagye (cherry picked from commit 104f5257a6b5527574904ec0bb94061f4790f423) Change-Id: Ic36b62ce48c54b7e3c9f1d34fcf5d6a42195abcd Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/2567457 --- common/motion_sense.c | 98 ++++++++++++++++++++++++++++++++------------------- 1 file 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); } -- cgit v1.2.1