On 8/24/21 4:30 AM, Xu Yang wrote:
Setting a delayed state by tcpm_set_state may enter the delayed state instantly without delay for the specified time. [ 65.424458] CC1: 0 -> 0, CC2: 0 -> 2 [state TOGGLING, polarity 0, connected] [ 65.424475] state change TOGGLING -> SRC_ATTACH_WAIT [rev1 NONE_AMS] [ 65.427233] VBUS off [ 65.427238] VBUS VSAFE0V [ 65.427243] pending state change SRC_ATTACH_WAIT -> SNK_TRY @ 200 ms [rev1 NONE_AMS] [ 65.427252] state change SRC_ATTACH_WAIT -> SNK_TRY [delayed 200 ms] [ 65.427258] cc:=2 In this log, tcpm should change to SNK_TRY state after 200 ms. The following sequence may trigger this abnormal result: [tcpm_pd_event_handler] [tcpm_state_machine_work] 1 tcpm_set_state(A, 0) 2 port->state = A 3 port->delayed_state = INVALID_STATE 4 queue work to worker_list 5 tcpm_set_state(B, ms) 6 port->delayed_state = B 7 start timer 8 dequeue work from worker_list 9 tcpm_state_machine_work 10 port->delayed_state != INVALID_STATE 11 port->state = B 12 port->delayed_state = INVALID_STATE 13 handle B state In step 9, tcpm_state_machine_work gets scheduled because it has been queued in step 4. At this point, however, both port->state and port->delayed_state are non INVALID_STATE which causes the pending state to be handled in step 13 without delay. If a non-delayed state and a delayed state are orderly set in some works except tcpm_state_machine_work, this bug will certainly occur. Also, if set in a thread different from tcpm worker thread, this bug may occur. Therefore, when port->delayed_state is a valid state but the state_machine_timer is still running, tcpm_state_machine_work should keep the delayed state pending until the state_machine_timer timeout. Fixes: 4b4e02c83167 ("typec: tcpm: Move out of staging") cc: <stable@xxxxxxxxxxxxxxx> Signed-off-by: Xu Yang <xu.yang_2@xxxxxxx> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c index c40e0513873d..4bdf119b1306 100644 --- a/drivers/usb/typec/tcpm/tcpm.c +++ b/drivers/usb/typec/tcpm/tcpm.c @@ -4815,7 +4815,8 @@ static void tcpm_state_machine_work(struct kthread_work *work) goto done;/* If we were queued due to a delayed state change, update it now */- if (port->delayed_state) { + if (port->delayed_state != INVALID_STATE && + ktime_after(ktime_get(), port->delayed_runtime)) { tcpm_log(port, "state change %s -> %s [delayed %ld ms]", tcpm_states[port->state], tcpm_states[port->delayed_state], port->delay_ms);
Unless I am missing something, this doesn't really match what the description says. It will ignore the pending state change and execute the state change to SRC_ATTACH_WAIT. This will then likely call tcpm_set_state() again. In other words, the state change to A is executed even though it was superseded with a state change to (B, ms). That doesn't look correct to me. I think the problem may be similar to the problem in patch 2: The worker is already running by the time tcpm_set_state(B, ms) is called, because tcpm_set_state(A, 0) triggered it. This means that "dequeue work from worker_list" already happened before tcpm_set_state(B, ms) was called. The only difference to patch 2 is that the multiple state changes are not triggered from tcpm_init() but by some external event outside the state machine. We should try to find a solution which covers both situations and makes sure that the worker only handles the most recent state change. Thanks, Guenter
