On 9/4/20 6:24 PM, ChiYuan Huang wrote: > Guenter Roeck <linux@xxxxxxxxxxxx> 於 2020年9月5日 週六 上午3:41寫道： >> >> On 9/3/20 9:21 AM, ChiYuan Huang wrote: >>> Guenter Roeck <linux@xxxxxxxxxxxx> 於 2020年9月3日 週四 上午12:57寫道： >>>> >>>> On Wed, Sep 02, 2020 at 11:35:33PM +0800, cy_huang wrote: >>>>> From: ChiYuan Huang <cy_huang@xxxxxxxxxxx> >>>>> >>>>> Fix: If vbus event is before cc_event trigger, hard_reset_count >>>>> won't bt reset for some case. >>>>> >>>>> Signed-off-by: ChiYuan Huang <cy_huang@xxxxxxxxxxx> >>>>> --- >>>>> Below's the flow. >>>>> >>>>> _tcpm_pd_vbus_off() -> run_state_machine to change state to SNK_UNATTACHED >>>>> call tcpm_snk_detach() -> tcpm_snk_detach() -> tcpm_detach() >>>>> tcpm_port_is_disconnected() will be called. >>>>> But port->attached is still true and port->cc1=open and port->cc2=open >>>>> >>>>> It cause tcpm_port_is_disconnected return false, then hard_reset_count won't be reset. >>>>> After that, tcpm_reset_port() is called. >>>>> port->attached become false. >>>>> >>>>> After that, cc now trigger cc_change event, the hard_reset_count will be kept. >>>>> Even tcpm_detach will be called, due to port->attached is false, tcpm_detach() >>>>> will directly return. >>>>> >>>>> CC_EVENT will only trigger drp toggling again. >>>>> --- >>>>> drivers/usb/typec/tcpm/tcpm.c | 3 +-- >>>>> 1 file changed, 1 insertion(+), 2 deletions(-) >>>>> >>>>> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c >>>>> index a48e3f90..5c73e1d 100644 >>>>> --- a/drivers/usb/typec/tcpm/tcpm.c >>>>> +++ b/drivers/usb/typec/tcpm/tcpm.c >>>>> @@ -2797,8 +2797,7 @@ static void tcpm_detach(struct tcpm_port *port) >>>>> port->tcpc->set_bist_data(port->tcpc, false); >>>>> } >>>>> >>>>> - if (tcpm_port_is_disconnected(port)) >>>>> - port->hard_reset_count = 0; >>>>> + port->hard_reset_count = 0; >>>>> >>>> >>>> Doesn't that mean that the state machine will never enter >>>> error recovery ? >>>> >>> I think it does't affect the error recovery. >>> All error recovery seems to check pd_capable flag. >>> >>> >From my below case, it's A to C cable only. There is no USBPD contract >>> will be estabilished. >>> >>> This case occurred following by the below test condition >>> Cable -> A to C (default Rp bind to vbus) connected to PC. >>> 1. first time plugged in the cable with PC >>> It will make HARD_RESET_COUNT to be equal 2 >>> 2. And then plug out. At that time HARD_RESET_COUNT is till 2. >>> 3. next time plugged in again. >>> Due to hard_reset_count is still 2 , after wait_cap_timeout, the state >>> eventually changed to SNK_READY. >>> But during the state transition, no hard_reset be sent. >>> >>> Defined in the USBPD policy engine, typec transition to USBPD, all >>> variables must be reset included hard_reset_count. >>> So it expected SNK must send hard_reset again. >>> >>> The original code defined hard_reset_count must be reset only when >>> tcpm_port_is_disconnected. >>> >>> It doesn't make sense that it only occurred in some scenario. >>> If tcpm_detach is called, hard_reset count must be reset also. >>> >> >> If a hard reset fails, the state machine may cycle through states >> HARD_RESET_SEND, HARD_RESET_START, SRC_HARD_RESET_VBUS_OFF, >> SRC_HARD_RESET_VBUS_ON back to SRC_UNATTACHED. In this state, >> tcpm_src_detach() and with it tcpm_detach() is called. The hard >> reset counter is incremented in HARD_RESET_SEND. If tcpm_detach() >> resets the counter, the state machine will keep cycling through hard >> resets without ever entering the error recovery state. I am not >> entirely sure where the counter should be reset, but tcpm_detach() >> seems to be the wrong place. > > This case you specified means locally error occurred. It could be a local error (with the local hardware), or with the remote partner not accepting the reset. We only know that an error occurred. > It intended to re-run the state machine from typec to USBPD. >>From my understanding, hard_reset_count to be reset is reasonable. > > The normal stare from the state transition you specified is > HARD_RESET_SEND, HARD_RESET_START -> SRC_HARD_RESET_VBUS_OFF, > SRC_HARD_RESET_VBUS_ON -> received VBUS_EVENT then go to SRC_STARTUP. > The operational word is "normal". Error recovery is expected to handle situations which are not normal. I don't question the need to reset the counter. The only question is where and when to reset it. Guenter
