| Message ID | 20210824113014.1420117-1-xu.yang_2@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] usb: typec: tcpm: Fix up tcpm set delayed state which may not delay | expand |
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@vger.kernel.org> > Signed-off-by: Xu Yang <xu.yang_2@nxp.com> > > 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
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);
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@vger.kernel.org> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>