Re: [PATCH 1/2] usb: typec: tcpm: Fix up tcpm set delayed state which may not delay

[Guenter Roeck <linux@roeck-us.net>]

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