usb: typec: tcpm: Fix if vbus before cc, hard_reset_count not reset issue

Commit Message

ChiYuan Huang Sept. 2, 2020, 3:35 p.m. UTC

From: ChiYuan Huang <[e-mail]>

Fix: If vbus event is before cc_event trigger, hard_reset_count
won't bt reset for some case.

Signed-off-by: ChiYuan Huang <[e-mail]>
---
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(-)

Comments

Guenter Roeck Sept. 2, 2020, 4:57 p.m. UTC | #1

On Wed, Sep 02, 2020 at 11:35:33PM +0800, cy_huang wrote:
> From: ChiYuan Huang <[e-mail]>
> 
> Fix: If vbus event is before cc_event trigger, hard_reset_count
> won't bt reset for some case.
> 
> Signed-off-by: ChiYuan Huang <[e-mail]>
> ---
> 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 ?

Guenter

>  	tcpm_reset_port(port);
>  }
> -- 
> 2.7.4
>

ChiYuan Huang Sept. 3, 2020, 4:21 p.m. UTC | #2

Guenter Roeck <[e-mail]> 於 2020年9月3日 週四 上午12:57寫道：
>
> On Wed, Sep 02, 2020 at 11:35:33PM +0800, cy_huang wrote:
> > From: ChiYuan Huang <[e-mail]>
> >
> > Fix: If vbus event is before cc_event trigger, hard_reset_count
> > won't bt reset for some case.
> >
> > Signed-off-by: ChiYuan Huang <[e-mail]>
> > ---
> > 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.

> Guenter
>
> >       tcpm_reset_port(port);
> >  }
> > --
> > 2.7.4
> >

Guenter Roeck Sept. 4, 2020, 7:41 p.m. UTC | #3

On 9/3/20 9:21 AM, ChiYuan Huang wrote:
> Guenter Roeck <[e-mail]> 於 2020年9月3日 週四 上午12:57寫道：
>>
>> On Wed, Sep 02, 2020 at 11:35:33PM +0800, cy_huang wrote:
>>> From: ChiYuan Huang <[e-mail]>
>>>
>>> Fix: If vbus event is before cc_event trigger, hard_reset_count
>>> won't bt reset for some case.
>>>
>>> Signed-off-by: ChiYuan Huang <[e-mail]>
>>> ---
>>> 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.

Guenter

>> Guenter
>>
>>>       tcpm_reset_port(port);
>>>  }
>>> --
>>> 2.7.4
>>>

ChiYuan Huang Sept. 5, 2020, 1:24 a.m. UTC | #4

Guenter Roeck <[e-mail]> 於 2020年9月5日 週六 上午3:41寫道：
>
> On 9/3/20 9:21 AM, ChiYuan Huang wrote:
> > Guenter Roeck <[e-mail]> 於 2020年9月3日 週四 上午12:57寫道：
> >>
> >> On Wed, Sep 02, 2020 at 11:35:33PM +0800, cy_huang wrote:
> >>> From: ChiYuan Huang <[e-mail]>
> >>>
> >>> Fix: If vbus event is before cc_event trigger, hard_reset_count
> >>> won't bt reset for some case.
> >>>
> >>> Signed-off-by: ChiYuan Huang <[e-mail]>
> >>> ---
> >>> 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 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.

>
> Guenter
>
> >> Guenter
> >>
> >>>       tcpm_reset_port(port);
> >>>  }
> >>> --
> >>> 2.7.4
> >>>
>

Guenter Roeck Sept. 5, 2020, 3:51 p.m. UTC | #5

On 9/4/20 6:24 PM, ChiYuan Huang wrote:
> Guenter Roeck <[e-mail]> 於 2020年9月5日 週六 上午3:41寫道：
>>
>> On 9/3/20 9:21 AM, ChiYuan Huang wrote:
>>> Guenter Roeck <[e-mail]> 於 2020年9月3日 週四 上午12:57寫道：
>>>>
>>>> On Wed, Sep 02, 2020 at 11:35:33PM +0800, cy_huang wrote:
>>>>> From: ChiYuan Huang <[e-mail]>
>>>>>
>>>>> Fix: If vbus event is before cc_event trigger, hard_reset_count
>>>>> won't bt reset for some case.
>>>>>
>>>>> Signed-off-by: ChiYuan Huang <[e-mail]>
>>>>> ---
>>>>> 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

ChiYuan Huang Sept. 6, 2020, 3:22 p.m. UTC | #6

Guenter Roeck <[e-mail]> 於 2020年9月5日 週六 下午11:51寫道：
>
> On 9/4/20 6:24 PM, ChiYuan Huang wrote:
> > Guenter Roeck <[e-mail]> 於 2020年9月5日 週六 上午3:41寫道：
> >>
> >> On 9/3/20 9:21 AM, ChiYuan Huang wrote:
> >>> Guenter Roeck <[e-mail]> 於 2020年9月3日 週四 上午12:57寫道：
> >>>>
> >>>> On Wed, Sep 02, 2020 at 11:35:33PM +0800, cy_huang wrote:
> >>>>> From: ChiYuan Huang <[e-mail]>
> >>>>>
> >>>>> Fix: If vbus event is before cc_event trigger, hard_reset_count
> >>>>> won't bt reset for some case.
> >>>>>
> >>>>> Signed-off-by: ChiYuan Huang <[e-mail]>
> >>>>> ---
> >>>>> 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.

Following by the USBPD 3.0 revision 1.2, section 8.3.3.24.1
The ErrorRecovery state is  used to electronically disconnect Port
Partner using the USB Type-C connector.
And there's one sentence to be said "The ErrorRecovery staste shall
map to USB Type-C Error Recovery state operations".
I also read ErrorRecovery state in USB TYPE-C 1.3 spec.
Section 4.5.2.2.2.1   ErrorRecovery state requirement listed the below text.
The port shall not drive VBUS or VCONN, and shall present a
high-impedance to ground (above
zOPEN) on its CC1 and CC2 pins.
Section 4.5.2.2.2.2 Exiting from the error recovery state
I read the description. The roughly meaning is to change the state to
Unattached(Src or Snk) after tErrorRecovery.

Summary the above text.
Reset HardResetCounter is ok in tcpm_detach.
My patch is just to relax the counter reset conditions during tcpm_detach().
If not, it will check tcpm_port_is_disconnected().
And only two scenario, the hard reset count will be cleared to 0 at this case.
1) port not attached and cc1=open and cc2=open
2) port attached and either (polarity=cc1, cc1=open) or (polarity=cc2, cc2=open)

I think this judgement is narrow in tcpm_detach case.

>
> I don't question the need to reset the counter. The only question
> is where and when to reset it.
>
I re-check all tcpm code for hard reset counter about the increment and reset.
They all meets USBPD spec. Only the detach case, I'm wondering why it
need to add the check for tcpm_port_is_disconnected().

> Guenter

ChiYuan Huang Sept. 15, 2020, 3:07 a.m. UTC | #7

Hi, Guenter:

ChiYuan Huang <[e-mail]> 於 2020年9月6日 週日 下午11:22寫道：
>
> Guenter Roeck <[e-mail]> 於 2020年9月5日 週六 下午11:51寫道：
> >
> > On 9/4/20 6:24 PM, ChiYuan Huang wrote:
> > > Guenter Roeck <[e-mail]> 於 2020年9月5日 週六 上午3:41寫道：
> > >>
> > >> On 9/3/20 9:21 AM, ChiYuan Huang wrote:
> > >>> Guenter Roeck <[e-mail]> 於 2020年9月3日 週四 上午12:57寫道：
> > >>>>
> > >>>> On Wed, Sep 02, 2020 at 11:35:33PM +0800, cy_huang wrote:
> > >>>>> From: ChiYuan Huang <[e-mail]>
> > >>>>>
> > >>>>> Fix: If vbus event is before cc_event trigger, hard_reset_count
> > >>>>> won't bt reset for some case.
> > >>>>>
> > >>>>> Signed-off-by: ChiYuan Huang <[e-mail]>
> > >>>>> ---
> > >>>>> 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.
>
> Following by the USBPD 3.0 revision 1.2, section 8.3.3.24.1
> The ErrorRecovery state is  used to electronically disconnect Port
> Partner using the USB Type-C connector.
> And there's one sentence to be said "The ErrorRecovery staste shall
> map to USB Type-C Error Recovery state operations".
> I also read ErrorRecovery state in USB TYPE-C 1.3 spec.
> Section 4.5.2.2.2.1   ErrorRecovery state requirement listed the below text.
> The port shall not drive VBUS or VCONN, and shall present a
> high-impedance to ground (above
> zOPEN) on its CC1 and CC2 pins.
> Section 4.5.2.2.2.2 Exiting from the error recovery state
> I read the description. The roughly meaning is to change the state to
> Unattached(Src or Snk) after tErrorRecovery.
>
> Summary the above text.
> Reset HardResetCounter is ok in tcpm_detach.
> My patch is just to relax the counter reset conditions during tcpm_detach().
> If not, it will check tcpm_port_is_disconnected().
> And only two scenario, the hard reset count will be cleared to 0 at this case.
> 1) port not attached and cc1=open and cc2=open
> 2) port attached and either (polarity=cc1, cc1=open) or (polarity=cc2, cc2=open)
>
> I think this judgement is narrow in tcpm_detach case.
>
> >
> > I don't question the need to reset the counter. The only question
> > is where and when to reset it.
> >
> I re-check all tcpm code for hard reset counter about the increment and reset.
> They all meets USBPD spec. Only the detach case, I'm wondering why it
> need to add the check for tcpm_port_is_disconnected().
>
Below's the real case log.
[ 4848.046358] VBUS off
[ 4848.046384] state change SNK_READY -> SNK_UNATTACHED
[ 4848.050908] Setting voltage/current limit 0 mV 0 mA
[ 4848.050936] polarity 0
[ 4848.052593] Requesting mux state 0, usb-role 0, orientation 0
[ 4848.053222] Start toggling
[ 4848.086500] state change SNK_UNATTACHED -> TOGGLING
[ 4848.089983] CC1: 0 -> 0, CC2: 3 -> 3 [state TOGGLING, polarity 0, connected]
[ 4848.089993] state change TOGGLING -> SNK_ATTACH_WAIT
[ 4848.090031] pending state change SNK_ATTACH_WAIT -> SNK_DEBOUNCED @ 200 ms
[ 4848.141162] CC1: 0 -> 0, CC2: 3 -> 0 [state SNK_ATTACH_WAIT,
polarity 0, disconnected]
[ 4848.141170] state change SNK_ATTACH_WAIT -> SNK_ATTACH_WAIT
[ 4848.141184] pending state change SNK_ATTACH_WAIT -> SNK_UNATTACHED @ 20 ms
[ 4848.163156] state change SNK_ATTACH_WAIT -> SNK_UNATTACHED [delayed 20 ms]
[ 4848.163162] Start toggling
[ 4848.216918] CC1: 0 -> 0, CC2: 0 -> 3 [state TOGGLING, polarity 0, connected]
[ 4848.216954] state change TOGGLING -> SNK_ATTACH_WAIT
[ 4848.217080] pending state change SNK_ATTACH_WAIT -> SNK_DEBOUNCED @ 200 ms
[ 4848.231771] CC1: 0 -> 0, CC2: 3 -> 0 [state SNK_ATTACH_WAIT,
polarity 0, disconnected]
[ 4848.231800] state change SNK_ATTACH_WAIT -> SNK_ATTACH_WAIT
[ 4848.231857] pending state change SNK_ATTACH_WAIT -> SNK_UNATTACHED @ 20 ms
[ 4848.256022] state change SNK_ATTACH_WAIT -> SNK_UNATTACHED [delayed 20 ms]
[ 4848.256049] Start toggling
[ 4848.871148] VBUS on
[ 4848.885324] CC1: 0 -> 0, CC2: 0 -> 3 [state TOGGLING, polarity 0, connected]
[ 4848.885372] state change TOGGLING -> SNK_ATTACH_WAIT
[ 4848.885548] pending state change SNK_ATTACH_WAIT -> SNK_DEBOUNCED @ 200 ms
[ 4849.088240] state change SNK_ATTACH_WAIT -> SNK_DEBOUNCED [delayed 200 ms]
[ 4849.088284] state change SNK_DEBOUNCED -> SNK_ATTACHED
[ 4849.088291] polarity 1
[ 4849.088769] Requesting mux state 1, usb-role 2, orientation 2
[ 4849.088895] state change SNK_ATTACHED -> SNK_STARTUP
[ 4849.088907] state change SNK_STARTUP -> SNK_DISCOVERY
[ 4849.088915] Setting voltage/current limit 5000 mV 0 mA
[ 4849.088927] vbus=0 charge:=1
[ 4849.090505] state change SNK_DISCOVERY -> SNK_WAIT_CAPABILITIES
[ 4849.090828] pending state change SNK_WAIT_CAPABILITIES -> SNK_READY @ 240 ms
[ 4849.335878] state change SNK_WAIT_CAPABILITIES -> SNK_READY [delayed 240 ms]

You can see the next type c attach log.
It directly change state from SNK_WAIT_CAPABILITIES to SNK_READY due
to not reset hard_reset_count.

It's easy to reproduce if you plugout USB Adapater w/i AtoC cable connected.

> > Guenter

Greg KH Oct. 2, 2020, 1:31 p.m. UTC | #8

On Tue, Sep 15, 2020 at 11:07:18AM +0800, ChiYuan Huang wrote:
> Hi, Guenter:
> 
> ChiYuan Huang <[e-mail]> 於 2020年9月6日 週日 下午11:22寫道：
> >
> > Guenter Roeck <[e-mail]> 於 2020年9月5日 週六 下午11:51寫道：
> > >
> > > On 9/4/20 6:24 PM, ChiYuan Huang wrote:
> > > > Guenter Roeck <[e-mail]> 於 2020年9月5日 週六 上午3:41寫道：
> > > >>
> > > >> On 9/3/20 9:21 AM, ChiYuan Huang wrote:
> > > >>> Guenter Roeck <[e-mail]> 於 2020年9月3日 週四 上午12:57寫道：
> > > >>>>
> > > >>>> On Wed, Sep 02, 2020 at 11:35:33PM +0800, cy_huang wrote:
> > > >>>>> From: ChiYuan Huang <[e-mail]>
> > > >>>>>
> > > >>>>> Fix: If vbus event is before cc_event trigger, hard_reset_count
> > > >>>>> won't bt reset for some case.
> > > >>>>>
> > > >>>>> Signed-off-by: ChiYuan Huang <[e-mail]>
> > > >>>>> ---
> > > >>>>> 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.
> >
> > Following by the USBPD 3.0 revision 1.2, section 8.3.3.24.1
> > The ErrorRecovery state is  used to electronically disconnect Port
> > Partner using the USB Type-C connector.
> > And there's one sentence to be said "The ErrorRecovery staste shall
> > map to USB Type-C Error Recovery state operations".
> > I also read ErrorRecovery state in USB TYPE-C 1.3 spec.
> > Section 4.5.2.2.2.1   ErrorRecovery state requirement listed the below text.
> > The port shall not drive VBUS or VCONN, and shall present a
> > high-impedance to ground (above
> > zOPEN) on its CC1 and CC2 pins.
> > Section 4.5.2.2.2.2 Exiting from the error recovery state
> > I read the description. The roughly meaning is to change the state to
> > Unattached(Src or Snk) after tErrorRecovery.
> >
> > Summary the above text.
> > Reset HardResetCounter is ok in tcpm_detach.
> > My patch is just to relax the counter reset conditions during tcpm_detach().
> > If not, it will check tcpm_port_is_disconnected().
> > And only two scenario, the hard reset count will be cleared to 0 at this case.
> > 1) port not attached and cc1=open and cc2=open
> > 2) port attached and either (polarity=cc1, cc1=open) or (polarity=cc2, cc2=open)
> >
> > I think this judgement is narrow in tcpm_detach case.
> >
> > >
> > > I don't question the need to reset the counter. The only question
> > > is where and when to reset it.
> > >
> > I re-check all tcpm code for hard reset counter about the increment and reset.
> > They all meets USBPD spec. Only the detach case, I'm wondering why it
> > need to add the check for tcpm_port_is_disconnected().
> >
> Below's the real case log.
> [ 4848.046358] VBUS off
> [ 4848.046384] state change SNK_READY -> SNK_UNATTACHED
> [ 4848.050908] Setting voltage/current limit 0 mV 0 mA
> [ 4848.050936] polarity 0
> [ 4848.052593] Requesting mux state 0, usb-role 0, orientation 0
> [ 4848.053222] Start toggling
> [ 4848.086500] state change SNK_UNATTACHED -> TOGGLING
> [ 4848.089983] CC1: 0 -> 0, CC2: 3 -> 3 [state TOGGLING, polarity 0, connected]
> [ 4848.089993] state change TOGGLING -> SNK_ATTACH_WAIT
> [ 4848.090031] pending state change SNK_ATTACH_WAIT -> SNK_DEBOUNCED @ 200 ms
> [ 4848.141162] CC1: 0 -> 0, CC2: 3 -> 0 [state SNK_ATTACH_WAIT,
> polarity 0, disconnected]
> [ 4848.141170] state change SNK_ATTACH_WAIT -> SNK_ATTACH_WAIT
> [ 4848.141184] pending state change SNK_ATTACH_WAIT -> SNK_UNATTACHED @ 20 ms
> [ 4848.163156] state change SNK_ATTACH_WAIT -> SNK_UNATTACHED [delayed 20 ms]
> [ 4848.163162] Start toggling
> [ 4848.216918] CC1: 0 -> 0, CC2: 0 -> 3 [state TOGGLING, polarity 0, connected]
> [ 4848.216954] state change TOGGLING -> SNK_ATTACH_WAIT
> [ 4848.217080] pending state change SNK_ATTACH_WAIT -> SNK_DEBOUNCED @ 200 ms
> [ 4848.231771] CC1: 0 -> 0, CC2: 3 -> 0 [state SNK_ATTACH_WAIT,
> polarity 0, disconnected]
> [ 4848.231800] state change SNK_ATTACH_WAIT -> SNK_ATTACH_WAIT
> [ 4848.231857] pending state change SNK_ATTACH_WAIT -> SNK_UNATTACHED @ 20 ms
> [ 4848.256022] state change SNK_ATTACH_WAIT -> SNK_UNATTACHED [delayed 20 ms]
> [ 4848.256049] Start toggling
> [ 4848.871148] VBUS on
> [ 4848.885324] CC1: 0 -> 0, CC2: 0 -> 3 [state TOGGLING, polarity 0, connected]
> [ 4848.885372] state change TOGGLING -> SNK_ATTACH_WAIT
> [ 4848.885548] pending state change SNK_ATTACH_WAIT -> SNK_DEBOUNCED @ 200 ms
> [ 4849.088240] state change SNK_ATTACH_WAIT -> SNK_DEBOUNCED [delayed 200 ms]
> [ 4849.088284] state change SNK_DEBOUNCED -> SNK_ATTACHED
> [ 4849.088291] polarity 1
> [ 4849.088769] Requesting mux state 1, usb-role 2, orientation 2
> [ 4849.088895] state change SNK_ATTACHED -> SNK_STARTUP
> [ 4849.088907] state change SNK_STARTUP -> SNK_DISCOVERY
> [ 4849.088915] Setting voltage/current limit 5000 mV 0 mA
> [ 4849.088927] vbus=0 charge:=1
> [ 4849.090505] state change SNK_DISCOVERY -> SNK_WAIT_CAPABILITIES
> [ 4849.090828] pending state change SNK_WAIT_CAPABILITIES -> SNK_READY @ 240 ms
> [ 4849.335878] state change SNK_WAIT_CAPABILITIES -> SNK_READY [delayed 240 ms]
> 
> You can see the next type c attach log.
> It directly change state from SNK_WAIT_CAPABILITIES to SNK_READY due
> to not reset hard_reset_count.
> 
> It's easy to reproduce if you plugout USB Adapater w/i AtoC cable connected.

What ever happened with this patch, is there still disagreement?

thanks,

greg k-h

Guenter Roeck Oct. 2, 2020, 2:26 p.m. UTC | #9

On 10/2/20 6:31 AM, Greg KH wrote:
> On Tue, Sep 15, 2020 at 11:07:18AM +0800, ChiYuan Huang wrote:
>> Hi, Guenter:
>>
>> ChiYuan Huang <[e-mail]> 於 2020年9月6日 週日 下午11:22寫道：
>>>
>>> Guenter Roeck <[e-mail]> 於 2020年9月5日 週六 下午11:51寫道：
>>>>
>>>> On 9/4/20 6:24 PM, ChiYuan Huang wrote:
>>>>> Guenter Roeck <[e-mail]> 於 2020年9月5日 週六 上午3:41寫道：
>>>>>>
>>>>>> On 9/3/20 9:21 AM, ChiYuan Huang wrote:
>>>>>>> Guenter Roeck <[e-mail]> 於 2020年9月3日 週四 上午12:57寫道：
>>>>>>>>
>>>>>>>> On Wed, Sep 02, 2020 at 11:35:33PM +0800, cy_huang wrote:
>>>>>>>>> From: ChiYuan Huang <[e-mail]>
>>>>>>>>>
>>>>>>>>> Fix: If vbus event is before cc_event trigger, hard_reset_count
>>>>>>>>> won't bt reset for some case.
>>>>>>>>>
>>>>>>>>> Signed-off-by: ChiYuan Huang <[e-mail]>
>>>>>>>>> ---
>>>>>>>>> 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.
>>>
>>> Following by the USBPD 3.0 revision 1.2, section 8.3.3.24.1
>>> The ErrorRecovery state is  used to electronically disconnect Port
>>> Partner using the USB Type-C connector.
>>> And there's one sentence to be said "The ErrorRecovery staste shall
>>> map to USB Type-C Error Recovery state operations".
>>> I also read ErrorRecovery state in USB TYPE-C 1.3 spec.
>>> Section 4.5.2.2.2.1   ErrorRecovery state requirement listed the below text.
>>> The port shall not drive VBUS or VCONN, and shall present a
>>> high-impedance to ground (above
>>> zOPEN) on its CC1 and CC2 pins.
>>> Section 4.5.2.2.2.2 Exiting from the error recovery state
>>> I read the description. The roughly meaning is to change the state to
>>> Unattached(Src or Snk) after tErrorRecovery.
>>>
>>> Summary the above text.
>>> Reset HardResetCounter is ok in tcpm_detach.
>>> My patch is just to relax the counter reset conditions during tcpm_detach().
>>> If not, it will check tcpm_port_is_disconnected().
>>> And only two scenario, the hard reset count will be cleared to 0 at this case.
>>> 1) port not attached and cc1=open and cc2=open
>>> 2) port attached and either (polarity=cc1, cc1=open) or (polarity=cc2, cc2=open)
>>>
>>> I think this judgement is narrow in tcpm_detach case.
>>>
>>>>
>>>> I don't question the need to reset the counter. The only question
>>>> is where and when to reset it.
>>>>
>>> I re-check all tcpm code for hard reset counter about the increment and reset.
>>> They all meets USBPD spec. Only the detach case, I'm wondering why it
>>> need to add the check for tcpm_port_is_disconnected().
>>>
>> Below's the real case log.
>> [ 4848.046358] VBUS off
>> [ 4848.046384] state change SNK_READY -> SNK_UNATTACHED
>> [ 4848.050908] Setting voltage/current limit 0 mV 0 mA
>> [ 4848.050936] polarity 0
>> [ 4848.052593] Requesting mux state 0, usb-role 0, orientation 0
>> [ 4848.053222] Start toggling
>> [ 4848.086500] state change SNK_UNATTACHED -> TOGGLING
>> [ 4848.089983] CC1: 0 -> 0, CC2: 3 -> 3 [state TOGGLING, polarity 0, connected]
>> [ 4848.089993] state change TOGGLING -> SNK_ATTACH_WAIT
>> [ 4848.090031] pending state change SNK_ATTACH_WAIT -> SNK_DEBOUNCED @ 200 ms
>> [ 4848.141162] CC1: 0 -> 0, CC2: 3 -> 0 [state SNK_ATTACH_WAIT,
>> polarity 0, disconnected]
>> [ 4848.141170] state change SNK_ATTACH_WAIT -> SNK_ATTACH_WAIT
>> [ 4848.141184] pending state change SNK_ATTACH_WAIT -> SNK_UNATTACHED @ 20 ms
>> [ 4848.163156] state change SNK_ATTACH_WAIT -> SNK_UNATTACHED [delayed 20 ms]
>> [ 4848.163162] Start toggling
>> [ 4848.216918] CC1: 0 -> 0, CC2: 0 -> 3 [state TOGGLING, polarity 0, connected]
>> [ 4848.216954] state change TOGGLING -> SNK_ATTACH_WAIT
>> [ 4848.217080] pending state change SNK_ATTACH_WAIT -> SNK_DEBOUNCED @ 200 ms
>> [ 4848.231771] CC1: 0 -> 0, CC2: 3 -> 0 [state SNK_ATTACH_WAIT,
>> polarity 0, disconnected]
>> [ 4848.231800] state change SNK_ATTACH_WAIT -> SNK_ATTACH_WAIT
>> [ 4848.231857] pending state change SNK_ATTACH_WAIT -> SNK_UNATTACHED @ 20 ms
>> [ 4848.256022] state change SNK_ATTACH_WAIT -> SNK_UNATTACHED [delayed 20 ms]
>> [ 4848.256049] Start toggling
>> [ 4848.871148] VBUS on
>> [ 4848.885324] CC1: 0 -> 0, CC2: 0 -> 3 [state TOGGLING, polarity 0, connected]
>> [ 4848.885372] state change TOGGLING -> SNK_ATTACH_WAIT
>> [ 4848.885548] pending state change SNK_ATTACH_WAIT -> SNK_DEBOUNCED @ 200 ms
>> [ 4849.088240] state change SNK_ATTACH_WAIT -> SNK_DEBOUNCED [delayed 200 ms]
>> [ 4849.088284] state change SNK_DEBOUNCED -> SNK_ATTACHED
>> [ 4849.088291] polarity 1
>> [ 4849.088769] Requesting mux state 1, usb-role 2, orientation 2
>> [ 4849.088895] state change SNK_ATTACHED -> SNK_STARTUP
>> [ 4849.088907] state change SNK_STARTUP -> SNK_DISCOVERY
>> [ 4849.088915] Setting voltage/current limit 5000 mV 0 mA
>> [ 4849.088927] vbus=0 charge:=1
>> [ 4849.090505] state change SNK_DISCOVERY -> SNK_WAIT_CAPABILITIES
>> [ 4849.090828] pending state change SNK_WAIT_CAPABILITIES -> SNK_READY @ 240 ms
>> [ 4849.335878] state change SNK_WAIT_CAPABILITIES -> SNK_READY [delayed 240 ms]
>>
>> You can see the next type c attach log.
>> It directly change state from SNK_WAIT_CAPABILITIES to SNK_READY due
>> to not reset hard_reset_count.
>>
>> It's easy to reproduce if you plugout USB Adapater w/i AtoC cable connected.
> 
> What ever happened with this patch, is there still disagreement?
> 

Yes, there is. I wouldn't have added the conditional without reason,
and I am concerned that removing it entirely will open another problem.
Feel free to apply, though - I can't prove that my concern is valid,
and after all we'll get reports from the field later if it is.

Guenter

Greg KH Oct. 5, 2020, 11:08 a.m. UTC | #10

On Fri, Oct 02, 2020 at 07:26:24AM -0700, Guenter Roeck wrote:
> On 10/2/20 6:31 AM, Greg KH wrote:
> > On Tue, Sep 15, 2020 at 11:07:18AM +0800, ChiYuan Huang wrote:
> >> Hi, Guenter:
> >>
> >> ChiYuan Huang <[e-mail]> 於 2020年9月6日 週日 下午11:22寫道：
> >>>
> >>> Guenter Roeck <[e-mail]> 於 2020年9月5日 週六 下午11:51寫道：
> >>>>
> >>>> On 9/4/20 6:24 PM, ChiYuan Huang wrote:
> >>>>> Guenter Roeck <[e-mail]> 於 2020年9月5日 週六 上午3:41寫道：
> >>>>>>
> >>>>>> On 9/3/20 9:21 AM, ChiYuan Huang wrote:
> >>>>>>> Guenter Roeck <[e-mail]> 於 2020年9月3日 週四 上午12:57寫道：
> >>>>>>>>
> >>>>>>>> On Wed, Sep 02, 2020 at 11:35:33PM +0800, cy_huang wrote:
> >>>>>>>>> From: ChiYuan Huang <[e-mail]>
> >>>>>>>>>
> >>>>>>>>> Fix: If vbus event is before cc_event trigger, hard_reset_count
> >>>>>>>>> won't bt reset for some case.
> >>>>>>>>>
> >>>>>>>>> Signed-off-by: ChiYuan Huang <[e-mail]>
> >>>>>>>>> ---
> >>>>>>>>> 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.
> >>>
> >>> Following by the USBPD 3.0 revision 1.2, section 8.3.3.24.1
> >>> The ErrorRecovery state is  used to electronically disconnect Port
> >>> Partner using the USB Type-C connector.
> >>> And there's one sentence to be said "The ErrorRecovery staste shall
> >>> map to USB Type-C Error Recovery state operations".
> >>> I also read ErrorRecovery state in USB TYPE-C 1.3 spec.
> >>> Section 4.5.2.2.2.1   ErrorRecovery state requirement listed the below text.
> >>> The port shall not drive VBUS or VCONN, and shall present a
> >>> high-impedance to ground (above
> >>> zOPEN) on its CC1 and CC2 pins.
> >>> Section 4.5.2.2.2.2 Exiting from the error recovery state
> >>> I read the description. The roughly meaning is to change the state to
> >>> Unattached(Src or Snk) after tErrorRecovery.
> >>>
> >>> Summary the above text.
> >>> Reset HardResetCounter is ok in tcpm_detach.
> >>> My patch is just to relax the counter reset conditions during tcpm_detach().
> >>> If not, it will check tcpm_port_is_disconnected().
> >>> And only two scenario, the hard reset count will be cleared to 0 at this case.
> >>> 1) port not attached and cc1=open and cc2=open
> >>> 2) port attached and either (polarity=cc1, cc1=open) or (polarity=cc2, cc2=open)
> >>>
> >>> I think this judgement is narrow in tcpm_detach case.
> >>>
> >>>>
> >>>> I don't question the need to reset the counter. The only question
> >>>> is where and when to reset it.
> >>>>
> >>> I re-check all tcpm code for hard reset counter about the increment and reset.
> >>> They all meets USBPD spec. Only the detach case, I'm wondering why it
> >>> need to add the check for tcpm_port_is_disconnected().
> >>>
> >> Below's the real case log.
> >> [ 4848.046358] VBUS off
> >> [ 4848.046384] state change SNK_READY -> SNK_UNATTACHED
> >> [ 4848.050908] Setting voltage/current limit 0 mV 0 mA
> >> [ 4848.050936] polarity 0
> >> [ 4848.052593] Requesting mux state 0, usb-role 0, orientation 0
> >> [ 4848.053222] Start toggling
> >> [ 4848.086500] state change SNK_UNATTACHED -> TOGGLING
> >> [ 4848.089983] CC1: 0 -> 0, CC2: 3 -> 3 [state TOGGLING, polarity 0, connected]
> >> [ 4848.089993] state change TOGGLING -> SNK_ATTACH_WAIT
> >> [ 4848.090031] pending state change SNK_ATTACH_WAIT -> SNK_DEBOUNCED @ 200 ms
> >> [ 4848.141162] CC1: 0 -> 0, CC2: 3 -> 0 [state SNK_ATTACH_WAIT,
> >> polarity 0, disconnected]
> >> [ 4848.141170] state change SNK_ATTACH_WAIT -> SNK_ATTACH_WAIT
> >> [ 4848.141184] pending state change SNK_ATTACH_WAIT -> SNK_UNATTACHED @ 20 ms
> >> [ 4848.163156] state change SNK_ATTACH_WAIT -> SNK_UNATTACHED [delayed 20 ms]
> >> [ 4848.163162] Start toggling
> >> [ 4848.216918] CC1: 0 -> 0, CC2: 0 -> 3 [state TOGGLING, polarity 0, connected]
> >> [ 4848.216954] state change TOGGLING -> SNK_ATTACH_WAIT
> >> [ 4848.217080] pending state change SNK_ATTACH_WAIT -> SNK_DEBOUNCED @ 200 ms
> >> [ 4848.231771] CC1: 0 -> 0, CC2: 3 -> 0 [state SNK_ATTACH_WAIT,
> >> polarity 0, disconnected]
> >> [ 4848.231800] state change SNK_ATTACH_WAIT -> SNK_ATTACH_WAIT
> >> [ 4848.231857] pending state change SNK_ATTACH_WAIT -> SNK_UNATTACHED @ 20 ms
> >> [ 4848.256022] state change SNK_ATTACH_WAIT -> SNK_UNATTACHED [delayed 20 ms]
> >> [ 4848.256049] Start toggling
> >> [ 4848.871148] VBUS on
> >> [ 4848.885324] CC1: 0 -> 0, CC2: 0 -> 3 [state TOGGLING, polarity 0, connected]
> >> [ 4848.885372] state change TOGGLING -> SNK_ATTACH_WAIT
> >> [ 4848.885548] pending state change SNK_ATTACH_WAIT -> SNK_DEBOUNCED @ 200 ms
> >> [ 4849.088240] state change SNK_ATTACH_WAIT -> SNK_DEBOUNCED [delayed 200 ms]
> >> [ 4849.088284] state change SNK_DEBOUNCED -> SNK_ATTACHED
> >> [ 4849.088291] polarity 1
> >> [ 4849.088769] Requesting mux state 1, usb-role 2, orientation 2
> >> [ 4849.088895] state change SNK_ATTACHED -> SNK_STARTUP
> >> [ 4849.088907] state change SNK_STARTUP -> SNK_DISCOVERY
> >> [ 4849.088915] Setting voltage/current limit 5000 mV 0 mA
> >> [ 4849.088927] vbus=0 charge:=1
> >> [ 4849.090505] state change SNK_DISCOVERY -> SNK_WAIT_CAPABILITIES
> >> [ 4849.090828] pending state change SNK_WAIT_CAPABILITIES -> SNK_READY @ 240 ms
> >> [ 4849.335878] state change SNK_WAIT_CAPABILITIES -> SNK_READY [delayed 240 ms]
> >>
> >> You can see the next type c attach log.
> >> It directly change state from SNK_WAIT_CAPABILITIES to SNK_READY due
> >> to not reset hard_reset_count.
> >>
> >> It's easy to reproduce if you plugout USB Adapater w/i AtoC cable connected.
> > 
> > What ever happened with this patch, is there still disagreement?
> > 
> 
> Yes, there is. I wouldn't have added the conditional without reason,
> and I am concerned that removing it entirely will open another problem.
> Feel free to apply, though - I can't prove that my concern is valid,
> and after all we'll get reports from the field later if it is.

Ok, can I get an ack so I know who to come back to in the future if
there are issues?  :)

thanks,

greg k-h

Guenter Roeck Oct. 5, 2020, 3:30 p.m. UTC | #11

On 10/5/20 4:08 AM, Greg KH wrote:
[ ... ]
>>> What ever happened with this patch, is there still disagreement?
>>>
>>
>> Yes, there is. I wouldn't have added the conditional without reason,
>> and I am concerned that removing it entirely will open another problem.
>> Feel free to apply, though - I can't prove that my concern is valid,
>> and after all we'll get reports from the field later if it is.
> 
> Ok, can I get an ack so I know who to come back to in the future if
> there are issues?  :)
> 

Not from me, for the reasons I stated. I would be ok with something like:

-	if (tcpm_port_is_disconnected(port))
+	if (tcpm_port_is_disconnected(port) ||
+           (tcpm_cc_is_open(port->cc1) && tcpm_cc_is_open(port->cc2)))

to narrow down the condition.

Guenter

ChiYuan Huang Oct. 6, 2020, 4:37 a.m. UTC | #12

Guenter Roeck <[e-mail]> 於 2020年10月5日 週一 下午11:30寫道：
>
> On 10/5/20 4:08 AM, Greg KH wrote:
> [ ... ]
> >>> What ever happened with this patch, is there still disagreement?
> >>>
> >>
> >> Yes, there is. I wouldn't have added the conditional without reason,
> >> and I am concerned that removing it entirely will open another problem.
> >> Feel free to apply, though - I can't prove that my concern is valid,
> >> and after all we'll get reports from the field later if it is.
> >
> > Ok, can I get an ack so I know who to come back to in the future if
> > there are issues?  :)
> >
>
> Not from me, for the reasons I stated. I would be ok with something like:
>
> -       if (tcpm_port_is_disconnected(port))
> +       if (tcpm_port_is_disconnected(port) ||
> +           (tcpm_cc_is_open(port->cc1) && tcpm_cc_is_open(port->cc2)))
>
> to narrow down the condition.

I have tried the above comment and It doesn't work.
How about to change the judgement like as below

-       if (tcpm_port_is_disconnected(port))
+       if (tcpm_port_is_disconnected(port) || !port->vbus_present)

The hard_reset_count not reset issue is following by the below order
1. VBUS off ( at the same time, cc is still detected as attached)
port->attached become false and cc is not open
2. After that, cc detached.
due to port->attached is false, tcpm_detach() directly return.

And that's why hard_reset_count is not reset to 0.
>
> Guenter

Jun Li Oct. 6, 2020, 4:52 p.m. UTC | #13

ChiYuan Huang <[e-mail]> 于2020年10月6日周二 下午12:38写道：
>
> Guenter Roeck <[e-mail]> 於 2020年10月5日 週一 下午11:30寫道：
> >
> > On 10/5/20 4:08 AM, Greg KH wrote:
> > [ ... ]
> > >>> What ever happened with this patch, is there still disagreement?
> > >>>
> > >>
> > >> Yes, there is. I wouldn't have added the conditional without reason,
> > >> and I am concerned that removing it entirely will open another problem.
> > >> Feel free to apply, though - I can't prove that my concern is valid,
> > >> and after all we'll get reports from the field later if it is.
> > >
> > > Ok, can I get an ack so I know who to come back to in the future if
> > > there are issues?  :)
> > >
> >
> > Not from me, for the reasons I stated. I would be ok with something like:
> >
> > -       if (tcpm_port_is_disconnected(port))
> > +       if (tcpm_port_is_disconnected(port) ||
> > +           (tcpm_cc_is_open(port->cc1) && tcpm_cc_is_open(port->cc2)))
> >
> > to narrow down the condition.
>
> I have tried the above comment and It doesn't work.
> How about to change the judgement like as below
>
> -       if (tcpm_port_is_disconnected(port))
> +       if (tcpm_port_is_disconnected(port) || !port->vbus_present)
>
> The hard_reset_count not reset issue is following by the below order
> 1. VBUS off ( at the same time, cc is still detected as attached)
> port->attached become false and cc is not open
> 2. After that, cc detached.
> due to port->attached is false, tcpm_detach() directly return.

If tcpm_detach() return directly, then how your patch can reset
hard_reset_count?

I am seeing the same issue on my platform, the proposed change:
-       if (tcpm_port_is_disconnected(port))
-               port->hard_reset_count = 0;
+       port->hard_reset_count = 0;
can't resolve it on my platform.

How about reset hard_reset_count in SNK_READY?
@@ -3325,6 +3329,7 @@ static void run_state_machine(struct tcpm_port *port)
        case SNK_READY:
                port->try_snk_count = 0;
                port->update_sink_caps = false;
+               port->hard_reset_count = 0;
                if (port->explicit_contract) {
                        typec_set_pwr_opmode(port->typec_port,
                                             TYPEC_PWR_MODE_PD);

can this resolve your problem?

Li Jun
>
> And that's why hard_reset_count is not reset to 0.
> >
> > Guenter

ChiYuan Huang Oct. 6, 2020, 5:39 p.m. UTC | #14

Jun Li <[e-mail]> 於 2020年10月7日 週三 上午12:52寫道：
>
> ChiYuan Huang <[e-mail]> 于2020年10月6日周二 下午12:38写道：
> >
> > Guenter Roeck <[e-mail]> 於 2020年10月5日 週一 下午11:30寫道：
> > >
> > > On 10/5/20 4:08 AM, Greg KH wrote:
> > > [ ... ]
> > > >>> What ever happened with this patch, is there still disagreement?
> > > >>>
> > > >>
> > > >> Yes, there is. I wouldn't have added the conditional without reason,
> > > >> and I am concerned that removing it entirely will open another problem.
> > > >> Feel free to apply, though - I can't prove that my concern is valid,
> > > >> and after all we'll get reports from the field later if it is.
> > > >
> > > > Ok, can I get an ack so I know who to come back to in the future if
> > > > there are issues?  :)
> > > >
> > >
> > > Not from me, for the reasons I stated. I would be ok with something like:
> > >
> > > -       if (tcpm_port_is_disconnected(port))
> > > +       if (tcpm_port_is_disconnected(port) ||
> > > +           (tcpm_cc_is_open(port->cc1) && tcpm_cc_is_open(port->cc2)))
> > >
> > > to narrow down the condition.
> >
> > I have tried the above comment and It doesn't work.
> > How about to change the judgement like as below
> >
> > -       if (tcpm_port_is_disconnected(port))
> > +       if (tcpm_port_is_disconnected(port) || !port->vbus_present)
> >
> > The hard_reset_count not reset issue is following by the below order
> > 1. VBUS off ( at the same time, cc is still detected as attached)
> > port->attached become false and cc is not open
> > 2. After that, cc detached.
> > due to port->attached is false, tcpm_detach() directly return.
>
> If tcpm_detach() return directly, then how your patch can reset
> hard_reset_count?
>
Yes, it can. We know vbus_present change from true to false and cc
detach both trigger tcpm_detach.
My change is whenever tcpm_detach to be called, hard_reset_count will
be reset to zero.

> I am seeing the same issue on my platform, the proposed change:
> -       if (tcpm_port_is_disconnected(port))
> -               port->hard_reset_count = 0;
> +       port->hard_reset_count = 0;
> can't resolve it on my platform.
>
I'm not sure what's your condition. Could you directly paste the tcpm
log for the check?
> How about reset hard_reset_count in SNK_READY?
> @@ -3325,6 +3329,7 @@ static void run_state_machine(struct tcpm_port *port)
>         case SNK_READY:
>                 port->try_snk_count = 0;
>                 port->update_sink_caps = false;
> +               port->hard_reset_count = 0;
>                 if (port->explicit_contract) {
>                         typec_set_pwr_opmode(port->typec_port,
>                                              TYPEC_PWR_MODE_PD);
>
> can this resolve your problem?
I'm not sure. It need to have a try, then I can answer you.
But from USBPD spec, the hard_reset_count need to reset zero only when
1. At src state, pe_src_send_cap and receive GoodCRC
2. At snk state, pe_snk_evaluate_cap need to reset hard_reset_count
>
> Li Jun
> >
> > And that's why hard_reset_count is not reset to 0.
> > >
> > > Guenter

ChiYuan Huang Oct. 7, 2020, 10:13 a.m. UTC | #15

ChiYuan Huang <[e-mail]> 於 2020年10月7日 週三 上午1:39寫道：
>
> Jun Li <[e-mail]> 於 2020年10月7日 週三 上午12:52寫道：
> >
> > ChiYuan Huang <[e-mail]> 于2020年10月6日周二 下午12:38写道：
> > >
> > > Guenter Roeck <[e-mail]> 於 2020年10月5日 週一 下午11:30寫道：
> > > >
> > > > On 10/5/20 4:08 AM, Greg KH wrote:
> > > > [ ... ]
> > > > >>> What ever happened with this patch, is there still disagreement?
> > > > >>>
> > > > >>
> > > > >> Yes, there is. I wouldn't have added the conditional without reason,
> > > > >> and I am concerned that removing it entirely will open another problem.
> > > > >> Feel free to apply, though - I can't prove that my concern is valid,
> > > > >> and after all we'll get reports from the field later if it is.
> > > > >
> > > > > Ok, can I get an ack so I know who to come back to in the future if
> > > > > there are issues?  :)
> > > > >
> > > >
> > > > Not from me, for the reasons I stated. I would be ok with something like:
> > > >
> > > > -       if (tcpm_port_is_disconnected(port))
> > > > +       if (tcpm_port_is_disconnected(port) ||
> > > > +           (tcpm_cc_is_open(port->cc1) && tcpm_cc_is_open(port->cc2)))
> > > >
> > > > to narrow down the condition.
> > >
> > > I have tried the above comment and It doesn't work.
> > > How about to change the judgement like as below
> > >
> > > -       if (tcpm_port_is_disconnected(port))
> > > +       if (tcpm_port_is_disconnected(port) || !port->vbus_present)
> > >
> > > The hard_reset_count not reset issue is following by the below order
> > > 1. VBUS off ( at the same time, cc is still detected as attached)
> > > port->attached become false and cc is not open
> > > 2. After that, cc detached.
> > > due to port->attached is false, tcpm_detach() directly return.
> >
> > If tcpm_detach() return directly, then how your patch can reset
> > hard_reset_count?
> >
> Yes, it can. We know vbus_present change from true to false and cc
> detach both trigger tcpm_detach.
> My change is whenever tcpm_detach to be called, hard_reset_count will
> be reset to zero.
>
> > I am seeing the same issue on my platform, the proposed change:
> > -       if (tcpm_port_is_disconnected(port))
> > -               port->hard_reset_count = 0;
> > +       port->hard_reset_count = 0;
> > can't resolve it on my platform.
> >
> I'm not sure what's your condition. Could you directly paste the tcpm
> log for the check?
> > How about reset hard_reset_count in SNK_READY?
> > @@ -3325,6 +3329,7 @@ static void run_state_machine(struct tcpm_port *port)
> >         case SNK_READY:
> >                 port->try_snk_count = 0;
> >                 port->update_sink_caps = false;
> > +               port->hard_reset_count = 0;
> >                 if (port->explicit_contract) {
> >                         typec_set_pwr_opmode(port->typec_port,
> >                                              TYPEC_PWR_MODE_PD);
> >
> > can this resolve your problem?
> I'm not sure. It need to have a try, then I can answer you.
> But from USBPD spec, the hard_reset_count need to reset zero only when
> 1. At src state, pe_src_send_cap and receive GoodCRC
> 2. At snk state, pe_snk_evaluate_cap need to reset hard_reset_count
> >
> > Li Jun
> > >
> > > And that's why hard_reset_count is not reset to 0.

I tried in snk_ready to reset hard_reset_count.
At normal case, it works.
But it seems still the possible fail case like as below.
200ms -> cc debounce max time
240ms -> snk_waitcap max time
If the plugin/out period is between (200+240) and (200+ 2* 240)ms ,
and the src side plug out like as the described scenario.
From this case, hard_reset_count may still 1.
And we expect the next plugin hard_reset_count is 0. But not, actually
it never reset.
So at next plugin, only one hard_reset will be sent and reach
hard_reset_count max (2).

This case is hard to reproduce. But actually it's possible.

> > > >
> > > > Guenter

Jun Li Oct. 9, 2020, 2:58 a.m. UTC | #16

> -----Original Message-----
> From: ChiYuan Huang <[e-mail]>
> Sent: Wednesday, October 7, 2020 1:39 AM
> To: Jun Li <[e-mail]>
> Cc: Guenter Roeck <[e-mail]>; Greg KH
> <[e-mail]>; Heikki Krogerus
> <[e-mail]>; Linux USB List
> <[e-mail]>; lkml <[e-mail]>;
> cy_huang <[e-mail]>; Jun Li <[e-mail]>
> Subject: Re: [PATCH] usb: typec: tcpm: Fix if vbus before cc, hard_reset_count
> not reset issue
> 
> Jun Li <[e-mail]> 於 2020年10月7日 週三 上午12:52寫道：
> >
> > ChiYuan Huang <[e-mail]> 于2020年10月6日周二 下午12:38写道：
> > >
> > > Guenter Roeck <[e-mail]> 於 2020年10月5日 週一 下午11:30寫
> 道：
> > > >
> > > > On 10/5/20 4:08 AM, Greg KH wrote:
> > > > [ ... ]
> > > > >>> What ever happened with this patch, is there still disagreement?
> > > > >>>
> > > > >>
> > > > >> Yes, there is. I wouldn't have added the conditional without
> > > > >> reason, and I am concerned that removing it entirely will open another
> problem.
> > > > >> Feel free to apply, though - I can't prove that my concern is
> > > > >> valid, and after all we'll get reports from the field later if it
> is.
> > > > >
> > > > > Ok, can I get an ack so I know who to come back to in the future
> > > > > if there are issues?  :)
> > > > >
> > > >
> > > > Not from me, for the reasons I stated. I would be ok with something
> like:
> > > >
> > > > -       if (tcpm_port_is_disconnected(port))
> > > > +       if (tcpm_port_is_disconnected(port) ||
> > > > +           (tcpm_cc_is_open(port->cc1) &&
> > > > + tcpm_cc_is_open(port->cc2)))
> > > >
> > > > to narrow down the condition.
> > >
> > > I have tried the above comment and It doesn't work.
> > > How about to change the judgement like as below
> > >
> > > -       if (tcpm_port_is_disconnected(port))
> > > +       if (tcpm_port_is_disconnected(port) || !port->vbus_present)
> > >
> > > The hard_reset_count not reset issue is following by the below order
> > > 1. VBUS off ( at the same time, cc is still detected as attached)
> > > port->attached become false and cc is not open
> > > 2. After that, cc detached.
> > > due to port->attached is false, tcpm_detach() directly return.
> >
> > If tcpm_detach() return directly, then how your patch can reset
> > hard_reset_count?
> >
> Yes, it can. We know vbus_present change from true to false and cc detach
> both trigger tcpm_detach.
> My change is whenever tcpm_detach to be called, hard_reset_count will be
> reset to zero.

Your patch is based on the assumption that tcpm_detach() is called with
port->attached is true, but tcpm_reset_port() may happen before that,
in that case, tcpm_reset_port() clear port->attached flag so tcpm_detach
will return directly.

> 
> > I am seeing the same issue on my platform, the proposed change:
> > -       if (tcpm_port_is_disconnected(port))
> > -               port->hard_reset_count = 0;
> > +       port->hard_reset_count = 0;
> > can't resolve it on my platform.
> >
> I'm not sure what's your condition. Could you directly paste the tcpm log
> for the check?

[   47.127729] Setting voltage/current limit 0 mV 0 mA
[   47.127739] polarity 0
[   47.129333] Requesting mux state 0, usb-role 0, orientation 0
[   47.130516] state change SNK_READY -> SNK_UNATTACHED
[   47.131181] CC1: 0 -> 0, CC2: 3 -> 0 [state SNK_UNATTACHED, polarity 0, disconnected]
[   47.131194] state change SNK_UNATTACHED -> PORT_RESET
[   47.134701] Setting voltage/current limit 0 mV 0 mA
[   47.134709] polarity 0
[   47.136291] Requesting mux state 0, usb-role 0, orientation 0
[   47.136873] cc:=0
[   47.137446] pending state change PORT_RESET -> PORT_RESET_WAIT_OFF @ 100 ms
[   47.138084] CC1: 0 -> 0, CC2: 0 -> 0 [state PORT_RESET, polarity 0, disconnected]
[   47.239143] state change PORT_RESET -> PORT_RESET_WAIT_OFF [delayed 100 ms]
[   47.239151] state change PORT_RESET_WAIT_OFF -> SNK_UNATTACHED
[   47.239154] Entering tcpm_detach directly return here <------------
[   47.239157] Start toggling
[   47.240150] state change SNK_UNATTACHED -> TOGGLING

Li Jun
> > How about reset hard_reset_count in SNK_READY?
> > @@ -3325,6 +3329,7 @@ static void run_state_machine(struct tcpm_port
> *port)
> >         case SNK_READY:
> >                 port->try_snk_count = 0;
> >                 port->update_sink_caps = false;
> > +               port->hard_reset_count = 0;
> >                 if (port->explicit_contract) {
> >                         typec_set_pwr_opmode(port->typec_port,
> >                                              TYPEC_PWR_MODE_PD);
> >
> > can this resolve your problem?
> I'm not sure. It need to have a try, then I can answer you.
> But from USBPD spec, the hard_reset_count need to reset zero only when 1.
> At src state, pe_src_send_cap and receive GoodCRC 2. At snk state,
> pe_snk_evaluate_cap need to reset hard_reset_count
> >
> > Li Jun
> > >
> > > And that's why hard_reset_count is not reset to 0.
> > > >
> > > > Guenter

Jun Li Oct. 9, 2020, 6:12 a.m. UTC | #17

> -----Original Message-----
> From: ChiYuan Huang <[e-mail]>
> Sent: Wednesday, October 7, 2020 6:13 PM
> To: Jun Li <[e-mail]>
> Cc: Guenter Roeck <[e-mail]>; Greg KH
> <[e-mail]>; Heikki Krogerus
> <[e-mail]>; Linux USB List
> <[e-mail]>; lkml <[e-mail]>;
> cy_huang <[e-mail]>; Jun Li <[e-mail]>
> Subject: Re: [PATCH] usb: typec: tcpm: Fix if vbus before cc, hard_reset_count
> not reset issue
> 
> ChiYuan Huang <[e-mail]> 於 2020年10月7日 週三 上午1:39寫道：
> >
> > Jun Li <[e-mail]> 於 2020年10月7日 週三 上午12:52寫道：
> > >
> > > ChiYuan Huang <[e-mail]> 于2020年10月6日周二 下午12:38写
> 道：
> > > >
> > > > Guenter Roeck <[e-mail]> 於 2020年10月5日 週一 下午11:30
> 寫道：
> > > > >
> > > > > On 10/5/20 4:08 AM, Greg KH wrote:
> > > > > [ ... ]
> > > > > >>> What ever happened with this patch, is there still disagreement?
> > > > > >>>
> > > > > >>
> > > > > >> Yes, there is. I wouldn't have added the conditional without
> > > > > >> reason, and I am concerned that removing it entirely will open
> another problem.
> > > > > >> Feel free to apply, though - I can't prove that my concern is
> > > > > >> valid, and after all we'll get reports from the field later if
> it is.
> > > > > >
> > > > > > Ok, can I get an ack so I know who to come back to in the
> > > > > > future if there are issues?  :)
> > > > > >
> > > > >
> > > > > Not from me, for the reasons I stated. I would be ok with something
> like:
> > > > >
> > > > > -       if (tcpm_port_is_disconnected(port))
> > > > > +       if (tcpm_port_is_disconnected(port) ||
> > > > > +           (tcpm_cc_is_open(port->cc1) &&
> > > > > + tcpm_cc_is_open(port->cc2)))
> > > > >
> > > > > to narrow down the condition.
> > > >
> > > > I have tried the above comment and It doesn't work.
> > > > How about to change the judgement like as below
> > > >
> > > > -       if (tcpm_port_is_disconnected(port))
> > > > +       if (tcpm_port_is_disconnected(port) ||
> > > > + !port->vbus_present)
> > > >
> > > > The hard_reset_count not reset issue is following by the below
> > > > order 1. VBUS off ( at the same time, cc is still detected as
> > > > attached)
> > > > port->attached become false and cc is not open
> > > > 2. After that, cc detached.
> > > > due to port->attached is false, tcpm_detach() directly return.
> > >
> > > If tcpm_detach() return directly, then how your patch can reset
> > > hard_reset_count?
> > >
> > Yes, it can. We know vbus_present change from true to false and cc
> > detach both trigger tcpm_detach.
> > My change is whenever tcpm_detach to be called, hard_reset_count will
> > be reset to zero.
> >
> > > I am seeing the same issue on my platform, the proposed change:
> > > -       if (tcpm_port_is_disconnected(port))
> > > -               port->hard_reset_count = 0;
> > > +       port->hard_reset_count = 0;
> > > can't resolve it on my platform.
> > >
> > I'm not sure what's your condition. Could you directly paste the tcpm
> > log for the check?
> > > How about reset hard_reset_count in SNK_READY?
> > > @@ -3325,6 +3329,7 @@ static void run_state_machine(struct tcpm_port
> *port)
> > >         case SNK_READY:
> > >                 port->try_snk_count = 0;
> > >                 port->update_sink_caps = false;
> > > +               port->hard_reset_count = 0;
> > >                 if (port->explicit_contract) {
> > >                         typec_set_pwr_opmode(port->typec_port,
> > >                                              TYPEC_PWR_MODE_PD);
> > >
> > > can this resolve your problem?
> > I'm not sure. It need to have a try, then I can answer you.
> > But from USBPD spec, the hard_reset_count need to reset zero only when
> > 1. At src state, pe_src_send_cap and receive GoodCRC 2. At snk state,
> > pe_snk_evaluate_cap need to reset hard_reset_count

3. 
8.3.3.3.8 PE_SNK_Hard_Reset state
"Note: The HardResetCounter is reset on a power cycle or Detach."

> > >
> > > Li Jun
> > > >
> > > > And that's why hard_reset_count is not reset to 0.
> 
> I tried in snk_ready to reset hard_reset_count.
> At normal case, it works.
> But it seems still the possible fail case like as below.
> 200ms -> cc debounce max time
> 240ms -> snk_waitcap max time
> If the plugin/out period is between (200+240) and (200+ 2* 240)ms , and the
> src side plug out like as the described scenario.
> From this case, hard_reset_count may still 1.
> And we expect the next plugin hard_reset_count is 0. But not, actually it
> never reset.
> So at next plugin, only one hard_reset will be sent and reach hard_reset_count
> max (2).
> 
> This case is hard to reproduce. But actually it's possible.

Make sense.

Then I propose doing this at SNK_UNATTACHED
@@ -3156,6 +3156,7 @@ static void run_state_machine(struct tcpm_port *port)
                if (!port->non_pd_role_swap)
                        tcpm_swap_complete(port, -ENOTCONN);
                tcpm_pps_complete(port, -ENOTCONN);
+               port->hard_reset_count = 0;
                tcpm_snk_detach(port);
                if (tcpm_start_toggling(port, TYPEC_CC_RD)) {
                        tcpm_set_state(port, TOGGLING, 0);
Li Jun

> 
> > > > >
> > > > > Guenter

ChiYuan Huang Oct. 9, 2020, 4:06 p.m. UTC | #18

Jun Li <[e-mail]> 於 2020年10月9日 週五 下午2:12寫道：
>
>
>
> > -----Original Message-----
> > From: ChiYuan Huang <[e-mail]>
> > Sent: Wednesday, October 7, 2020 6:13 PM
> > To: Jun Li <[e-mail]>
> > Cc: Guenter Roeck <[e-mail]>; Greg KH
> > <[e-mail]>; Heikki Krogerus
> > <[e-mail]>; Linux USB List
> > <[e-mail]>; lkml <[e-mail]>;
> > cy_huang <[e-mail]>; Jun Li <[e-mail]>
> > Subject: Re: [PATCH] usb: typec: tcpm: Fix if vbus before cc, hard_reset_count
> > not reset issue
> >
> > ChiYuan Huang <[e-mail]> 於 2020年10月7日 週三 上午1:39寫道：
> > >
> > > Jun Li <[e-mail]> 於 2020年10月7日 週三 上午12:52寫道：
> > > >
> > > > ChiYuan Huang <[e-mail]> 于2020年10月6日周二 下午12:38写
> > 道：
> > > > >
> > > > > Guenter Roeck <[e-mail]> 於 2020年10月5日 週一 下午11:30
> > 寫道：
> > > > > >
> > > > > > On 10/5/20 4:08 AM, Greg KH wrote:
> > > > > > [ ... ]
> > > > > > >>> What ever happened with this patch, is there still disagreement?
> > > > > > >>>
> > > > > > >>
> > > > > > >> Yes, there is. I wouldn't have added the conditional without
> > > > > > >> reason, and I am concerned that removing it entirely will open
> > another problem.
> > > > > > >> Feel free to apply, though - I can't prove that my concern is
> > > > > > >> valid, and after all we'll get reports from the field later if
> > it is.
> > > > > > >
> > > > > > > Ok, can I get an ack so I know who to come back to in the
> > > > > > > future if there are issues?  :)
> > > > > > >
> > > > > >
> > > > > > Not from me, for the reasons I stated. I would be ok with something
> > like:
> > > > > >
> > > > > > -       if (tcpm_port_is_disconnected(port))
> > > > > > +       if (tcpm_port_is_disconnected(port) ||
> > > > > > +           (tcpm_cc_is_open(port->cc1) &&
> > > > > > + tcpm_cc_is_open(port->cc2)))
> > > > > >
> > > > > > to narrow down the condition.
> > > > >
> > > > > I have tried the above comment and It doesn't work.
> > > > > How about to change the judgement like as below
> > > > >
> > > > > -       if (tcpm_port_is_disconnected(port))
> > > > > +       if (tcpm_port_is_disconnected(port) ||
> > > > > + !port->vbus_present)
> > > > >
> > > > > The hard_reset_count not reset issue is following by the below
> > > > > order 1. VBUS off ( at the same time, cc is still detected as
> > > > > attached)
> > > > > port->attached become false and cc is not open
> > > > > 2. After that, cc detached.
> > > > > due to port->attached is false, tcpm_detach() directly return.
> > > >
> > > > If tcpm_detach() return directly, then how your patch can reset
> > > > hard_reset_count?
> > > >
> > > Yes, it can. We know vbus_present change from true to false and cc
> > > detach both trigger tcpm_detach.
> > > My change is whenever tcpm_detach to be called, hard_reset_count will
> > > be reset to zero.
> > >
> > > > I am seeing the same issue on my platform, the proposed change:
> > > > -       if (tcpm_port_is_disconnected(port))
> > > > -               port->hard_reset_count = 0;
> > > > +       port->hard_reset_count = 0;
> > > > can't resolve it on my platform.
> > > >
> > > I'm not sure what's your condition. Could you directly paste the tcpm
> > > log for the check?
> > > > How about reset hard_reset_count in SNK_READY?
> > > > @@ -3325,6 +3329,7 @@ static void run_state_machine(struct tcpm_port
> > *port)
> > > >         case SNK_READY:
> > > >                 port->try_snk_count = 0;
> > > >                 port->update_sink_caps = false;
> > > > +               port->hard_reset_count = 0;
> > > >                 if (port->explicit_contract) {
> > > >                         typec_set_pwr_opmode(port->typec_port,
> > > >                                              TYPEC_PWR_MODE_PD);
> > > >
> > > > can this resolve your problem?
> > > I'm not sure. It need to have a try, then I can answer you.
> > > But from USBPD spec, the hard_reset_count need to reset zero only when
> > > 1. At src state, pe_src_send_cap and receive GoodCRC 2. At snk state,
> > > pe_snk_evaluate_cap need to reset hard_reset_count
>
> 3.
> 8.3.3.3.8 PE_SNK_Hard_Reset state
> "Note: The HardResetCounter is reset on a power cycle or Detach."
>
> > > >
> > > > Li Jun
> > > > >
> > > > > And that's why hard_reset_count is not reset to 0.
> >
> > I tried in snk_ready to reset hard_reset_count.
> > At normal case, it works.
> > But it seems still the possible fail case like as below.
> > 200ms -> cc debounce max time
> > 240ms -> snk_waitcap max time
> > If the plugin/out period is between (200+240) and (200+ 2* 240)ms , and the
> > src side plug out like as the described scenario.
> > From this case, hard_reset_count may still 1.
> > And we expect the next plugin hard_reset_count is 0. But not, actually it
> > never reset.
> > So at next plugin, only one hard_reset will be sent and reach hard_reset_count
> > max (2).
> >
> > This case is hard to reproduce. But actually it's possible.
>
> Make sense.
>
> Then I propose doing this at SNK_UNATTACHED
> @@ -3156,6 +3156,7 @@ static void run_state_machine(struct tcpm_port *port)
>                 if (!port->non_pd_role_swap)
>                         tcpm_swap_complete(port, -ENOTCONN);
>                 tcpm_pps_complete(port, -ENOTCONN);
> +               port->hard_reset_count = 0;
>                 tcpm_snk_detach(port);
>                 if (tcpm_start_toggling(port, TYPEC_CC_RD)) {
>                         tcpm_set_state(port, TOGGLING, 0);
> Li Jun

For the current power role is snk, I think it may work.
How about the src role? src role only reset the hard_reset_count in
tcpm_detach and src_ready state?

I check the flow that  you mentioned in the previous mail. It's really
a special case from any state to port_reset.
If the case is considered, how about to reset  the hard_reset_count
and don't care the port is attached or not in tcpm_detach function
like as below.

@@ -2789,6 +2789,8 @@ static void tcpm_reset_port(struct tcpm_port *port)

 static void tcpm_detach(struct tcpm_port *port)
 {
+       port->hard_reset_count = 0;
+
        if (!port->attached)
                return;

@@ -2797,9 +2799,6 @@ 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;
-
        tcpm_reset_port(port);
 }

Like I mentioned before, whatever the condition is, hard_reset_count
must be reset to zero during tcpm_detach.

But refer to Guenter's mail,  he prefer to narrow down the condition
to reset this counter.

I think the original thought is important why to put this line there.

Hi, Guenter:
   From the discussion, we really need to know why you put the reset
line below port attached is false and also make some judgement.
I think there may be ome condition that we don't considered.

>
> >
> > > > > >
> > > > > > Guenter

Jun Li Oct. 10, 2020, 11:21 a.m. UTC | #19

> -----Original Message-----
> From: ChiYuan Huang <[e-mail]>
> Sent: Saturday, October 10, 2020 12:06 AM
> To: Jun Li <[e-mail]>
> Cc: Jun Li <[e-mail]>; Guenter Roeck <[e-mail]>;
> Greg KH <[e-mail]>; Heikki Krogerus
> <[e-mail]>; Linux USB List
> <[e-mail]>; lkml <[e-mail]>;
> cy_huang <[e-mail]>
> Subject: Re: [PATCH] usb: typec: tcpm: Fix if vbus before cc, hard_reset_count
> not reset issue
> 
> Jun Li <[e-mail]> 於 2020年10月9日 週五 下午2:12寫道：
> >
> >
> >
> > > -----Original Message-----
> > > From: ChiYuan Huang <[e-mail]>
> > > Sent: Wednesday, October 7, 2020 6:13 PM
> > > To: Jun Li <[e-mail]>
> > > Cc: Guenter Roeck <[e-mail]>; Greg KH
> > > <[e-mail]>; Heikki Krogerus
> > > <[e-mail]>; Linux USB List
> > > <[e-mail]>; lkml <[e-mail]>;
> > > cy_huang <[e-mail]>; Jun Li <[e-mail]>
> > > Subject: Re: [PATCH] usb: typec: tcpm: Fix if vbus before cc,
> > > hard_reset_count not reset issue
> > >
> > > ChiYuan Huang <[e-mail]> 於 2020年10月7日 週三 上午1:39寫
> 道：
> > > >
> > > > Jun Li <[e-mail]> 於 2020年10月7日 週三 上午12:52寫
> 道：
> > > > >
> > > > > ChiYuan Huang <[e-mail]> 于2020年10月6日周二 下午12:38
> 写
> > > 道：
> > > > > >
> > > > > > Guenter Roeck <[e-mail]> 於 2020年10月5日 週一 下午
> 11:30
> > > 寫道：
> > > > > > >
> > > > > > > On 10/5/20 4:08 AM, Greg KH wrote:
> > > > > > > [ ... ]
> > > > > > > >>> What ever happened with this patch, is there still disagreement?
> > > > > > > >>>
> > > > > > > >>
> > > > > > > >> Yes, there is. I wouldn't have added the conditional
> > > > > > > >> without reason, and I am concerned that removing it
> > > > > > > >> entirely will open
> > > another problem.
> > > > > > > >> Feel free to apply, though - I can't prove that my
> > > > > > > >> concern is valid, and after all we'll get reports from
> > > > > > > >> the field later if
> > > it is.
> > > > > > > >
> > > > > > > > Ok, can I get an ack so I know who to come back to in the
> > > > > > > > future if there are issues?  :)
> > > > > > > >
> > > > > > >
> > > > > > > Not from me, for the reasons I stated. I would be ok with
> > > > > > > something
> > > like:
> > > > > > >
> > > > > > > -       if (tcpm_port_is_disconnected(port))
> > > > > > > +       if (tcpm_port_is_disconnected(port) ||
> > > > > > > +           (tcpm_cc_is_open(port->cc1) &&
> > > > > > > + tcpm_cc_is_open(port->cc2)))
> > > > > > >
> > > > > > > to narrow down the condition.
> > > > > >
> > > > > > I have tried the above comment and It doesn't work.
> > > > > > How about to change the judgement like as below
> > > > > >
> > > > > > -       if (tcpm_port_is_disconnected(port))
> > > > > > +       if (tcpm_port_is_disconnected(port) ||
> > > > > > + !port->vbus_present)
> > > > > >
> > > > > > The hard_reset_count not reset issue is following by the below
> > > > > > order 1. VBUS off ( at the same time, cc is still detected as
> > > > > > attached)
> > > > > > port->attached become false and cc is not open
> > > > > > 2. After that, cc detached.
> > > > > > due to port->attached is false, tcpm_detach() directly return.
> > > > >
> > > > > If tcpm_detach() return directly, then how your patch can reset
> > > > > hard_reset_count?
> > > > >
> > > > Yes, it can. We know vbus_present change from true to false and cc
> > > > detach both trigger tcpm_detach.
> > > > My change is whenever tcpm_detach to be called, hard_reset_count
> > > > will be reset to zero.
> > > >
> > > > > I am seeing the same issue on my platform, the proposed change:
> > > > > -       if (tcpm_port_is_disconnected(port))
> > > > > -               port->hard_reset_count = 0;
> > > > > +       port->hard_reset_count = 0;
> > > > > can't resolve it on my platform.
> > > > >
> > > > I'm not sure what's your condition. Could you directly paste the
> > > > tcpm log for the check?
> > > > > How about reset hard_reset_count in SNK_READY?
> > > > > @@ -3325,6 +3329,7 @@ static void run_state_machine(struct
> > > > > tcpm_port
> > > *port)
> > > > >         case SNK_READY:
> > > > >                 port->try_snk_count = 0;
> > > > >                 port->update_sink_caps = false;
> > > > > +               port->hard_reset_count = 0;
> > > > >                 if (port->explicit_contract) {
> > > > >                         typec_set_pwr_opmode(port->typec_port,
> > > > >                                              TYPEC_PWR_MODE_PD);
> > > > >
> > > > > can this resolve your problem?
> > > > I'm not sure. It need to have a try, then I can answer you.
> > > > But from USBPD spec, the hard_reset_count need to reset zero only
> > > > when 1. At src state, pe_src_send_cap and receive GoodCRC 2. At
> > > > snk state, pe_snk_evaluate_cap need to reset hard_reset_count
> >
> > 3.
> > 8.3.3.3.8 PE_SNK_Hard_Reset state
> > "Note: The HardResetCounter is reset on a power cycle or Detach."
> >
> > > > >
> > > > > Li Jun
> > > > > >
> > > > > > And that's why hard_reset_count is not reset to 0.
> > >
> > > I tried in snk_ready to reset hard_reset_count.
> > > At normal case, it works.
> > > But it seems still the possible fail case like as below.
> > > 200ms -> cc debounce max time
> > > 240ms -> snk_waitcap max time
> > > If the plugin/out period is between (200+240) and (200+ 2* 240)ms ,
> > > and the src side plug out like as the described scenario.
> > > From this case, hard_reset_count may still 1.
> > > And we expect the next plugin hard_reset_count is 0. But not,
> > > actually it never reset.
> > > So at next plugin, only one hard_reset will be sent and reach
> > > hard_reset_count max (2).
> > >
> > > This case is hard to reproduce. But actually it's possible.
> >
> > Make sense.
> >
> > Then I propose doing this at SNK_UNATTACHED @@ -3156,6 +3156,7 @@
> > static void run_state_machine(struct tcpm_port *port)
> >                 if (!port->non_pd_role_swap)
> >                         tcpm_swap_complete(port, -ENOTCONN);
> >                 tcpm_pps_complete(port, -ENOTCONN);
> > +               port->hard_reset_count = 0;
> >                 tcpm_snk_detach(port);
> >                 if (tcpm_start_toggling(port, TYPEC_CC_RD)) {
> >                         tcpm_set_state(port, TOGGLING, 0); Li Jun
> 
> For the current power role is snk, I think it may work.
> How about the src role? src role only reset the hard_reset_count in
> tcpm_detach and src_ready state?

Sorry, after gave more check on PD sped, this isn't a right solution.
See below

> 
> I check the flow that  you mentioned in the previous mail. It's really a
> special case from any state to port_reset.
> If the case is considered, how about to reset  the hard_reset_count and don't
> care the port is attached or not in tcpm_detach function like as below.
> 
> @@ -2789,6 +2789,8 @@ static void tcpm_reset_port(struct tcpm_port *port)
> 
>  static void tcpm_detach(struct tcpm_port *port)  {
> +       port->hard_reset_count = 0;
> +
>         if (!port->attached)
>                 return;
> 
> @@ -2797,9 +2799,6 @@ 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;
> -
>         tcpm_reset_port(port);
>  }
> 
> Like I mentioned before, whatever the condition is, hard_reset_count must
> be reset to zero during tcpm_detach.

This may not be so correct as you said, I think Guenter's concern is valid.

tcpm_detach() is not *only* be called in cases of *physical* detach
like the function name suggests.

The current proposals may *wrongly* reset this counter.

Let me give an example:

HARD_RESET_SEND -> HARD_RESET_START ->
SNK_HARD_RESET_SINK_OFF -> SNK_HARD_RESET_WAIT_VBUS ->
SNK_UNATTACHED(in case of VBUS doesn't come back in expected duration)
-> call to tcpm_detach()

In this sequence, does the sink need keep the counter? From the PD spec,
I think the answer is yes, sink need this counter to judge if need
do hard reset again or error recovery on later try, see:

Figure 8-67 Sink Port State Diagram

The difference between your case and my example, is your case never turn
off vbus, so will not go to SNK_UNATTACHED, so will not call to tcpm_detach()
after first hard reset.

So
	if (tcpm_port_is_disconnected(port))
		port->hard_reset_count = 0;

should keep as it is, the counter can only be cleared if there is really
physical disconnect by *partner*.

But current tcpm code may have some problem on keeping local CC state if
there is hard reset on-going(port->hard_reset_count > 0), because the
current handling of SNK_UNATTACHED may cause disconnection generated
by *local*(partner actually keeps its CC), e.g. reset polarity and do
drp_toggling, this should be fixed, but this is another patch, I can
try to do this later.

Back to your problem, I think the issue is, at the first tcpm_detach()
the cc disconnect event hasn't happen, so the reset counter is left there
but the port->attached is cleared, then the following(real disconnect)
tcpm_detach() will just return directly due to port->attached is false.

So I guess this will resolve your problem:

@@ -2885,6 +2885,9 @@ static void tcpm_reset_port(struct tcpm_port *port)

 static void tcpm_detach(struct tcpm_port *port)
 {
+       if (tcpm_port_is_disconnected(port))
+               port->hard_reset_count = 0;
+
        if (!port->attached)
                return;

@@ -2893,9 +2896,6 @@ 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;
-
        tcpm_reset_port(port);
 }


Compared with current code's condition:
   3 static bool tcpm_port_is_disconnected(struct tcpm_port *port)
   4 {
   5         return (!port->attached && port->cc1 == TYPEC_CC_OPEN &&
   6                 port->cc2 == TYPEC_CC_OPEN) ||
   7                (port->attached && ((port->polarity == TYPEC_POLARITY_CC1 &&
   8                                     port->cc1 == TYPEC_CC_OPEN) ||
   9                                    (port->polarity == TYPEC_POLARITY_CC2 &&
  10                                     port->cc2 == TYPEC_CC_OPEN)));
  11 }

My above change is only adding another condition to clear the reset counter:
(!port->attached && port->cc1 == TYPEC_CC_OPEN && port->cc2 == TYPEC_CC_OPEN)

This condition is close to Guenter's suggestion in this thread:

-       if (tcpm_port_is_disconnected(port))
+       if (tcpm_port_is_disconnected(port) ||
+           (tcpm_cc_is_open(port->cc1) && tcpm_cc_is_open(port->cc2)))

Hi Guenter, any comments on this?

Thanks
Li Jun

> 
> But refer to Guenter's mail,  he prefer to narrow down the condition to reset
> this counter.
> 
> I think the original thought is important why to put this line there.
> 
> Hi, Guenter:
>    From the discussion, we really need to know why you put the reset line
> below port attached is false and also make some judgement.
> I think there may be ome condition that we don't considered.

This condition was added at first place, I think my above 
> 
> >
> > >
> > > > > > >
> > > > > > > Guenter

Guenter Roeck Oct. 10, 2020, 3:46 p.m. UTC | #20

On Sat, Oct 10, 2020 at 12:06:13AM +0800, ChiYuan Huang wrote:
[ ... ]
> 
> Like I mentioned before, whatever the condition is, hard_reset_count
> must be reset to zero during tcpm_detach.
> 
> But refer to Guenter's mail,  he prefer to narrow down the condition
> to reset this counter.
> 
> I think the original thought is important why to put this line there.
> 
> Hi, Guenter:
>    From the discussion, we really need to know why you put the reset
> line below port attached is false and also make some judgement.
> I think there may be ome condition that we don't considered.
> 
As I am sure I have mentioned before, it was to handle misbehaving
partners, to enforce that the system goes into error recovery state
and (hopefully) recover the partner enough to be able to reconnect.
This is the same reason why resetting the counter is commented out
in SRC_SEND_CAPABILITIES and reset in SRC_READY instead. The typical
sequence was that the state machine would process from SRC_UNATTACHED
to some point and then stall / time out, but never be in disconnected
state.

Always resetting the hard reset counter in tcpm_detach() would disable
error recovery in that situation, and affected partners would never
recover. Effectively it would disable error recovery in any state machine
cycle which involves an unattached state, which makes me really question
if it is indeed mandated by the specification to reset the hard reset
counter at that point.

Guenter

Guenter Roeck Oct. 10, 2020, 7:31 p.m. UTC | #21

On 10/10/20 4:21 AM, Jun Li wrote:
> 
> 
>> -----Original Message-----
>> From: ChiYuan Huang <[e-mail]>
>> Sent: Saturday, October 10, 2020 12:06 AM
>> To: Jun Li <[e-mail]>
>> Cc: Jun Li <[e-mail]>; Guenter Roeck <[e-mail]>;
>> Greg KH <[e-mail]>; Heikki Krogerus
>> <[e-mail]>; Linux USB List
>> <[e-mail]>; lkml <[e-mail]>;
>> cy_huang <[e-mail]>
>> Subject: Re: [PATCH] usb: typec: tcpm: Fix if vbus before cc, hard_reset_count
>> not reset issue
>>
>> Jun Li <[e-mail]> 於 2020年10月9日 週五 下午2:12寫道：
>>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: ChiYuan Huang <[e-mail]>
>>>> Sent: Wednesday, October 7, 2020 6:13 PM
>>>> To: Jun Li <[e-mail]>
>>>> Cc: Guenter Roeck <[e-mail]>; Greg KH
>>>> <[e-mail]>; Heikki Krogerus
>>>> <[e-mail]>; Linux USB List
>>>> <[e-mail]>; lkml <[e-mail]>;
>>>> cy_huang <[e-mail]>; Jun Li <[e-mail]>
>>>> Subject: Re: [PATCH] usb: typec: tcpm: Fix if vbus before cc,
>>>> hard_reset_count not reset issue
>>>>
>>>> ChiYuan Huang <[e-mail]> 於 2020年10月7日 週三 上午1:39寫
>> 道：
>>>>>
>>>>> Jun Li <[e-mail]> 於 2020年10月7日 週三 上午12:52寫
>> 道：
>>>>>>
>>>>>> ChiYuan Huang <[e-mail]> 于2020年10月6日周二 下午12:38
>> 写
>>>> 道：
>>>>>>>
>>>>>>> Guenter Roeck <[e-mail]> 於 2020年10月5日 週一 下午
>> 11:30
>>>> 寫道：
>>>>>>>>
>>>>>>>> On 10/5/20 4:08 AM, Greg KH wrote:
>>>>>>>> [ ... ]
>>>>>>>>>>> What ever happened with this patch, is there still disagreement?
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Yes, there is. I wouldn't have added the conditional
>>>>>>>>>> without reason, and I am concerned that removing it
>>>>>>>>>> entirely will open
>>>> another problem.
>>>>>>>>>> Feel free to apply, though - I can't prove that my
>>>>>>>>>> concern is valid, and after all we'll get reports from
>>>>>>>>>> the field later if
>>>> it is.
>>>>>>>>>
>>>>>>>>> Ok, can I get an ack so I know who to come back to in the
>>>>>>>>> future if there are issues?  :)
>>>>>>>>>
>>>>>>>>
>>>>>>>> Not from me, for the reasons I stated. I would be ok with
>>>>>>>> something
>>>> like:
>>>>>>>>
>>>>>>>> -       if (tcpm_port_is_disconnected(port))
>>>>>>>> +       if (tcpm_port_is_disconnected(port) ||
>>>>>>>> +           (tcpm_cc_is_open(port->cc1) &&
>>>>>>>> + tcpm_cc_is_open(port->cc2)))
>>>>>>>>
>>>>>>>> to narrow down the condition.
>>>>>>>
>>>>>>> I have tried the above comment and It doesn't work.
>>>>>>> How about to change the judgement like as below
>>>>>>>
>>>>>>> -       if (tcpm_port_is_disconnected(port))
>>>>>>> +       if (tcpm_port_is_disconnected(port) ||
>>>>>>> + !port->vbus_present)
>>>>>>>
>>>>>>> The hard_reset_count not reset issue is following by the below
>>>>>>> order 1. VBUS off ( at the same time, cc is still detected as
>>>>>>> attached)
>>>>>>> port->attached become false and cc is not open
>>>>>>> 2. After that, cc detached.
>>>>>>> due to port->attached is false, tcpm_detach() directly return.
>>>>>>
>>>>>> If tcpm_detach() return directly, then how your patch can reset
>>>>>> hard_reset_count?
>>>>>>
>>>>> Yes, it can. We know vbus_present change from true to false and cc
>>>>> detach both trigger tcpm_detach.
>>>>> My change is whenever tcpm_detach to be called, hard_reset_count
>>>>> will be reset to zero.
>>>>>
>>>>>> I am seeing the same issue on my platform, the proposed change:
>>>>>> -       if (tcpm_port_is_disconnected(port))
>>>>>> -               port->hard_reset_count = 0;
>>>>>> +       port->hard_reset_count = 0;
>>>>>> can't resolve it on my platform.
>>>>>>
>>>>> I'm not sure what's your condition. Could you directly paste the
>>>>> tcpm log for the check?
>>>>>> How about reset hard_reset_count in SNK_READY?
>>>>>> @@ -3325,6 +3329,7 @@ static void run_state_machine(struct
>>>>>> tcpm_port
>>>> *port)
>>>>>>         case SNK_READY:
>>>>>>                 port->try_snk_count = 0;
>>>>>>                 port->update_sink_caps = false;
>>>>>> +               port->hard_reset_count = 0;
>>>>>>                 if (port->explicit_contract) {
>>>>>>                         typec_set_pwr_opmode(port->typec_port,
>>>>>>                                              TYPEC_PWR_MODE_PD);
>>>>>>
>>>>>> can this resolve your problem?
>>>>> I'm not sure. It need to have a try, then I can answer you.
>>>>> But from USBPD spec, the hard_reset_count need to reset zero only
>>>>> when 1. At src state, pe_src_send_cap and receive GoodCRC 2. At
>>>>> snk state, pe_snk_evaluate_cap need to reset hard_reset_count
>>>
>>> 3.
>>> 8.3.3.3.8 PE_SNK_Hard_Reset state
>>> "Note: The HardResetCounter is reset on a power cycle or Detach."
>>>
>>>>>>
>>>>>> Li Jun
>>>>>>>
>>>>>>> And that's why hard_reset_count is not reset to 0.
>>>>
>>>> I tried in snk_ready to reset hard_reset_count.
>>>> At normal case, it works.
>>>> But it seems still the possible fail case like as below.
>>>> 200ms -> cc debounce max time
>>>> 240ms -> snk_waitcap max time
>>>> If the plugin/out period is between (200+240) and (200+ 2* 240)ms ,
>>>> and the src side plug out like as the described scenario.
>>>> From this case, hard_reset_count may still 1.
>>>> And we expect the next plugin hard_reset_count is 0. But not,
>>>> actually it never reset.
>>>> So at next plugin, only one hard_reset will be sent and reach
>>>> hard_reset_count max (2).
>>>>
>>>> This case is hard to reproduce. But actually it's possible.
>>>
>>> Make sense.
>>>
>>> Then I propose doing this at SNK_UNATTACHED @@ -3156,6 +3156,7 @@
>>> static void run_state_machine(struct tcpm_port *port)
>>>                 if (!port->non_pd_role_swap)
>>>                         tcpm_swap_complete(port, -ENOTCONN);
>>>                 tcpm_pps_complete(port, -ENOTCONN);
>>> +               port->hard_reset_count = 0;
>>>                 tcpm_snk_detach(port);
>>>                 if (tcpm_start_toggling(port, TYPEC_CC_RD)) {
>>>                         tcpm_set_state(port, TOGGLING, 0); Li Jun
>>
>> For the current power role is snk, I think it may work.
>> How about the src role? src role only reset the hard_reset_count in
>> tcpm_detach and src_ready state?
> 
> Sorry, after gave more check on PD sped, this isn't a right solution.
> See below
> 
>>
>> I check the flow that  you mentioned in the previous mail. It's really a
>> special case from any state to port_reset.
>> If the case is considered, how about to reset  the hard_reset_count and don't
>> care the port is attached or not in tcpm_detach function like as below.
>>
>> @@ -2789,6 +2789,8 @@ static void tcpm_reset_port(struct tcpm_port *port)
>>
>>  static void tcpm_detach(struct tcpm_port *port)  {
>> +       port->hard_reset_count = 0;
>> +
>>         if (!port->attached)
>>                 return;
>>
>> @@ -2797,9 +2799,6 @@ 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;
>> -
>>         tcpm_reset_port(port);
>>  }
>>
>> Like I mentioned before, whatever the condition is, hard_reset_count must
>> be reset to zero during tcpm_detach.
> 
> This may not be so correct as you said, I think Guenter's concern is valid.
> 
> tcpm_detach() is not *only* be called in cases of *physical* detach
> like the function name suggests.
> 
> The current proposals may *wrongly* reset this counter.
> 
> Let me give an example:
> 
> HARD_RESET_SEND -> HARD_RESET_START ->
> SNK_HARD_RESET_SINK_OFF -> SNK_HARD_RESET_WAIT_VBUS ->
> SNK_UNATTACHED(in case of VBUS doesn't come back in expected duration)
> -> call to tcpm_detach()
> 
> In this sequence, does the sink need keep the counter? From the PD spec,
> I think the answer is yes, sink need this counter to judge if need
> do hard reset again or error recovery on later try, see:
> 
> Figure 8-67 Sink Port State Diagram
> 
> The difference between your case and my example, is your case never turn
> off vbus, so will not go to SNK_UNATTACHED, so will not call to tcpm_detach()
> after first hard reset.
> 
> So
> 	if (tcpm_port_is_disconnected(port))
> 		port->hard_reset_count = 0;
> 
> should keep as it is, the counter can only be cleared if there is really
> physical disconnect by *partner*.
> 
> But current tcpm code may have some problem on keeping local CC state if
> there is hard reset on-going(port->hard_reset_count > 0), because the
> current handling of SNK_UNATTACHED may cause disconnection generated
> by *local*(partner actually keeps its CC), e.g. reset polarity and do
> drp_toggling, this should be fixed, but this is another patch, I can
> try to do this later.
> 
> Back to your problem, I think the issue is, at the first tcpm_detach()
> the cc disconnect event hasn't happen, so the reset counter is left there
> but the port->attached is cleared, then the following(real disconnect)
> tcpm_detach() will just return directly due to port->attached is false.
> 
> So I guess this will resolve your problem:
> 
> @@ -2885,6 +2885,9 @@ static void tcpm_reset_port(struct tcpm_port *port)
> 
>  static void tcpm_detach(struct tcpm_port *port)
>  {
> +       if (tcpm_port_is_disconnected(port))
> +               port->hard_reset_count = 0;
> +
>         if (!port->attached)
>                 return;
> 
> @@ -2893,9 +2896,6 @@ 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;
> -
>         tcpm_reset_port(port);
>  }
> 
> 
> Compared with current code's condition:
>    3 static bool tcpm_port_is_disconnected(struct tcpm_port *port)
>    4 {
>    5         return (!port->attached && port->cc1 == TYPEC_CC_OPEN &&
>    6                 port->cc2 == TYPEC_CC_OPEN) ||
>    7                (port->attached && ((port->polarity == TYPEC_POLARITY_CC1 &&
>    8                                     port->cc1 == TYPEC_CC_OPEN) ||
>    9                                    (port->polarity == TYPEC_POLARITY_CC2 &&
>   10                                     port->cc2 == TYPEC_CC_OPEN)));
>   11 }
> 
> My above change is only adding another condition to clear the reset counter:
> (!port->attached && port->cc1 == TYPEC_CC_OPEN && port->cc2 == TYPEC_CC_OPEN)
> 
> This condition is close to Guenter's suggestion in this thread:
> 
> -       if (tcpm_port_is_disconnected(port))
> +       if (tcpm_port_is_disconnected(port) ||
> +           (tcpm_cc_is_open(port->cc1) && tcpm_cc_is_open(port->cc2)))
> 
> Hi Guenter, any comments on this?
> 

That makes sense, and I would agree to the change you suggest above.

Guenter

> Thanks
> Li Jun
> 
>>
>> But refer to Guenter's mail,  he prefer to narrow down the condition to reset
>> this counter.
>>
>> I think the original thought is important why to put this line there.
>>
>> Hi, Guenter:
>>    From the discussion, we really need to know why you put the reset line
>> below port attached is false and also make some judgement.
>> I think there may be ome condition that we don't considered.
> 
> This condition was added at first place, I think my above 
>>
>>>
>>>>
>>>>>>>>
>>>>>>>> Guenter

ChiYuan Huang Oct. 12, 2020, 6:22 a.m. UTC | #22

Guenter Roeck <[e-mail]> 於 2020年10月11日 週日 上午3:31寫道：
>
> On 10/10/20 4:21 AM, Jun Li wrote:
> >
> >
> >> -----Original Message-----
> >> From: ChiYuan Huang <[e-mail]>
> >> Sent: Saturday, October 10, 2020 12:06 AM
> >> To: Jun Li <[e-mail]>
> >> Cc: Jun Li <[e-mail]>; Guenter Roeck <[e-mail]>;
> >> Greg KH <[e-mail]>; Heikki Krogerus
> >> <[e-mail]>; Linux USB List
> >> <[e-mail]>; lkml <[e-mail]>;
> >> cy_huang <[e-mail]>
> >> Subject: Re: [PATCH] usb: typec: tcpm: Fix if vbus before cc, hard_reset_count
> >> not reset issue
> >>
> >> Jun Li <[e-mail]> 於 2020年10月9日 週五 下午2:12寫道：
> >>>
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: ChiYuan Huang <[e-mail]>
> >>>> Sent: Wednesday, October 7, 2020 6:13 PM
> >>>> To: Jun Li <[e-mail]>
> >>>> Cc: Guenter Roeck <[e-mail]>; Greg KH
> >>>> <[e-mail]>; Heikki Krogerus
> >>>> <[e-mail]>; Linux USB List
> >>>> <[e-mail]>; lkml <[e-mail]>;
> >>>> cy_huang <[e-mail]>; Jun Li <[e-mail]>
> >>>> Subject: Re: [PATCH] usb: typec: tcpm: Fix if vbus before cc,
> >>>> hard_reset_count not reset issue
> >>>>
> >>>> ChiYuan Huang <[e-mail]> 於 2020年10月7日 週三 上午1:39寫
> >> 道：
> >>>>>
> >>>>> Jun Li <[e-mail]> 於 2020年10月7日 週三 上午12:52寫
> >> 道：
> >>>>>>
> >>>>>> ChiYuan Huang <[e-mail]> 于2020年10月6日周二 下午12:38
> >> 写
> >>>> 道：
> >>>>>>>
> >>>>>>> Guenter Roeck <[e-mail]> 於 2020年10月5日 週一 下午
> >> 11:30
> >>>> 寫道：
> >>>>>>>>
> >>>>>>>> On 10/5/20 4:08 AM, Greg KH wrote:
> >>>>>>>> [ ... ]
> >>>>>>>>>>> What ever happened with this patch, is there still disagreement?
> >>>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> Yes, there is. I wouldn't have added the conditional
> >>>>>>>>>> without reason, and I am concerned that removing it
> >>>>>>>>>> entirely will open
> >>>> another problem.
> >>>>>>>>>> Feel free to apply, though - I can't prove that my
> >>>>>>>>>> concern is valid, and after all we'll get reports from
> >>>>>>>>>> the field later if
> >>>> it is.
> >>>>>>>>>
> >>>>>>>>> Ok, can I get an ack so I know who to come back to in the
> >>>>>>>>> future if there are issues?  :)
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> Not from me, for the reasons I stated. I would be ok with
> >>>>>>>> something
> >>>> like:
> >>>>>>>>
> >>>>>>>> -       if (tcpm_port_is_disconnected(port))
> >>>>>>>> +       if (tcpm_port_is_disconnected(port) ||
> >>>>>>>> +           (tcpm_cc_is_open(port->cc1) &&
> >>>>>>>> + tcpm_cc_is_open(port->cc2)))
> >>>>>>>>
> >>>>>>>> to narrow down the condition.
> >>>>>>>
> >>>>>>> I have tried the above comment and It doesn't work.
> >>>>>>> How about to change the judgement like as below
> >>>>>>>
> >>>>>>> -       if (tcpm_port_is_disconnected(port))
> >>>>>>> +       if (tcpm_port_is_disconnected(port) ||
> >>>>>>> + !port->vbus_present)
> >>>>>>>
> >>>>>>> The hard_reset_count not reset issue is following by the below
> >>>>>>> order 1. VBUS off ( at the same time, cc is still detected as
> >>>>>>> attached)
> >>>>>>> port->attached become false and cc is not open
> >>>>>>> 2. After that, cc detached.
> >>>>>>> due to port->attached is false, tcpm_detach() directly return.
> >>>>>>
> >>>>>> If tcpm_detach() return directly, then how your patch can reset
> >>>>>> hard_reset_count?
> >>>>>>
> >>>>> Yes, it can. We know vbus_present change from true to false and cc
> >>>>> detach both trigger tcpm_detach.
> >>>>> My change is whenever tcpm_detach to be called, hard_reset_count
> >>>>> will be reset to zero.
> >>>>>
> >>>>>> I am seeing the same issue on my platform, the proposed change:
> >>>>>> -       if (tcpm_port_is_disconnected(port))
> >>>>>> -               port->hard_reset_count = 0;
> >>>>>> +       port->hard_reset_count = 0;
> >>>>>> can't resolve it on my platform.
> >>>>>>
> >>>>> I'm not sure what's your condition. Could you directly paste the
> >>>>> tcpm log for the check?
> >>>>>> How about reset hard_reset_count in SNK_READY?
> >>>>>> @@ -3325,6 +3329,7 @@ static void run_state_machine(struct
> >>>>>> tcpm_port
> >>>> *port)
> >>>>>>         case SNK_READY:
> >>>>>>                 port->try_snk_count = 0;
> >>>>>>                 port->update_sink_caps = false;
> >>>>>> +               port->hard_reset_count = 0;
> >>>>>>                 if (port->explicit_contract) {
> >>>>>>                         typec_set_pwr_opmode(port->typec_port,
> >>>>>>                                              TYPEC_PWR_MODE_PD);
> >>>>>>
> >>>>>> can this resolve your problem?
> >>>>> I'm not sure. It need to have a try, then I can answer you.
> >>>>> But from USBPD spec, the hard_reset_count need to reset zero only
> >>>>> when 1. At src state, pe_src_send_cap and receive GoodCRC 2. At
> >>>>> snk state, pe_snk_evaluate_cap need to reset hard_reset_count
> >>>
> >>> 3.
> >>> 8.3.3.3.8 PE_SNK_Hard_Reset state
> >>> "Note: The HardResetCounter is reset on a power cycle or Detach."
> >>>
> >>>>>>
> >>>>>> Li Jun
> >>>>>>>
> >>>>>>> And that's why hard_reset_count is not reset to 0.
> >>>>
> >>>> I tried in snk_ready to reset hard_reset_count.
> >>>> At normal case, it works.
> >>>> But it seems still the possible fail case like as below.
> >>>> 200ms -> cc debounce max time
> >>>> 240ms -> snk_waitcap max time
> >>>> If the plugin/out period is between (200+240) and (200+ 2* 240)ms ,
> >>>> and the src side plug out like as the described scenario.
> >>>> From this case, hard_reset_count may still 1.
> >>>> And we expect the next plugin hard_reset_count is 0. But not,
> >>>> actually it never reset.
> >>>> So at next plugin, only one hard_reset will be sent and reach
> >>>> hard_reset_count max (2).
> >>>>
> >>>> This case is hard to reproduce. But actually it's possible.
> >>>
> >>> Make sense.
> >>>
> >>> Then I propose doing this at SNK_UNATTACHED @@ -3156,6 +3156,7 @@
> >>> static void run_state_machine(struct tcpm_port *port)
> >>>                 if (!port->non_pd_role_swap)
> >>>                         tcpm_swap_complete(port, -ENOTCONN);
> >>>                 tcpm_pps_complete(port, -ENOTCONN);
> >>> +               port->hard_reset_count = 0;
> >>>                 tcpm_snk_detach(port);
> >>>                 if (tcpm_start_toggling(port, TYPEC_CC_RD)) {
> >>>                         tcpm_set_state(port, TOGGLING, 0); Li Jun
> >>
> >> For the current power role is snk, I think it may work.
> >> How about the src role? src role only reset the hard_reset_count in
> >> tcpm_detach and src_ready state?
> >
> > Sorry, after gave more check on PD sped, this isn't a right solution.
> > See below
> >
> >>
> >> I check the flow that  you mentioned in the previous mail. It's really a
> >> special case from any state to port_reset.
> >> If the case is considered, how about to reset  the hard_reset_count and don't
> >> care the port is attached or not in tcpm_detach function like as below.
> >>
> >> @@ -2789,6 +2789,8 @@ static void tcpm_reset_port(struct tcpm_port *port)
> >>
> >>  static void tcpm_detach(struct tcpm_port *port)  {
> >> +       port->hard_reset_count = 0;
> >> +
> >>         if (!port->attached)
> >>                 return;
> >>
> >> @@ -2797,9 +2799,6 @@ 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;
> >> -
> >>         tcpm_reset_port(port);
> >>  }
> >>
> >> Like I mentioned before, whatever the condition is, hard_reset_count must
> >> be reset to zero during tcpm_detach.
> >
> > This may not be so correct as you said, I think Guenter's concern is valid.
> >
> > tcpm_detach() is not *only* be called in cases of *physical* detach
> > like the function name suggests.
> >
> > The current proposals may *wrongly* reset this counter.
> >
> > Let me give an example:
> >
> > HARD_RESET_SEND -> HARD_RESET_START ->
> > SNK_HARD_RESET_SINK_OFF -> SNK_HARD_RESET_WAIT_VBUS ->
> > SNK_UNATTACHED(in case of VBUS doesn't come back in expected duration)
> > -> call to tcpm_detach()
> >
> > In this sequence, does the sink need keep the counter? From the PD spec,
> > I think the answer is yes, sink need this counter to judge if need
> > do hard reset again or error recovery on later try, see:
> >
> > Figure 8-67 Sink Port State Diagram
> >
> > The difference between your case and my example, is your case never turn
> > off vbus, so will not go to SNK_UNATTACHED, so will not call to tcpm_detach()
> > after first hard reset.
> >
> > So
> >       if (tcpm_port_is_disconnected(port))
> >               port->hard_reset_count = 0;
> >
> > should keep as it is, the counter can only be cleared if there is really
> > physical disconnect by *partner*.
> >
> > But current tcpm code may have some problem on keeping local CC state if
> > there is hard reset on-going(port->hard_reset_count > 0), because the
> > current handling of SNK_UNATTACHED may cause disconnection generated
> > by *local*(partner actually keeps its CC), e.g. reset polarity and do
> > drp_toggling, this should be fixed, but this is another patch, I can
> > try to do this later.
> >
> > Back to your problem, I think the issue is, at the first tcpm_detach()
> > the cc disconnect event hasn't happen, so the reset counter is left there
> > but the port->attached is cleared, then the following(real disconnect)
> > tcpm_detach() will just return directly due to port->attached is false.
> >
> > So I guess this will resolve your problem:
> >
> > @@ -2885,6 +2885,9 @@ static void tcpm_reset_port(struct tcpm_port *port)
> >
> >  static void tcpm_detach(struct tcpm_port *port)
> >  {
> > +       if (tcpm_port_is_disconnected(port))
> > +               port->hard_reset_count = 0;
> > +
> >         if (!port->attached)
> >                 return;
> >
> > @@ -2893,9 +2896,6 @@ 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;
> > -
> >         tcpm_reset_port(port);
> >  }
> >
> >
I have checked this patch. It can solve the problem.

> > Compared with current code's condition:
> >    3 static bool tcpm_port_is_disconnected(struct tcpm_port *port)
> >    4 {
> >    5         return (!port->attached && port->cc1 == TYPEC_CC_OPEN &&
> >    6                 port->cc2 == TYPEC_CC_OPEN) ||
> >    7                (port->attached && ((port->polarity == TYPEC_POLARITY_CC1 &&
> >    8                                     port->cc1 == TYPEC_CC_OPEN) ||
> >    9                                    (port->polarity == TYPEC_POLARITY_CC2 &&
> >   10                                     port->cc2 == TYPEC_CC_OPEN)));
> >   11 }
> >
> > My above change is only adding another condition to clear the reset counter:
> > (!port->attached && port->cc1 == TYPEC_CC_OPEN && port->cc2 == TYPEC_CC_OPEN)
> >
> > This condition is close to Guenter's suggestion in this thread:
> >
> > -       if (tcpm_port_is_disconnected(port))
> > +       if (tcpm_port_is_disconnected(port) ||
> > +           (tcpm_cc_is_open(port->cc1) && tcpm_cc_is_open(port->cc2)))
> >
> > Hi Guenter, any comments on this?
> >
>
> That makes sense, and I would agree to the change you suggest above.
Jun:
   will you send the patch following the final discussion? Or I also can help.
Thx.
>
> Guenter
>
> > Thanks
> > Li Jun
> >
> >>
> >> But refer to Guenter's mail,  he prefer to narrow down the condition to reset
> >> this counter.
> >>
> >> I think the original thought is important why to put this line there.
> >>
> >> Hi, Guenter:
> >>    From the discussion, we really need to know why you put the reset line
> >> below port attached is false and also make some judgement.
> >> I think there may be ome condition that we don't considered.
> >
> > This condition was added at first place, I think my above
> >>
> >>>
> >>>>
> >>>>>>>>
> >>>>>>>> Guenter
>

Jun Li Oct. 12, 2020, 8:58 a.m. UTC | #23

> -----Original Message-----
> From: Guenter Roeck <[e-mail]> On Behalf Of Guenter Roeck
> Sent: Sunday, October 11, 2020 3:32 AM
> To: Jun Li <[e-mail]>; ChiYuan Huang <[e-mail]>
> Cc: Jun Li <[e-mail]>; Greg KH <[e-mail]>;
> Heikki Krogerus <[e-mail]>; Linux USB List
> <[e-mail]>; lkml <[e-mail]>;
> cy_huang <[e-mail]>
> Subject: Re: [PATCH] usb: typec: tcpm: Fix if vbus before cc, hard_reset_count
> not reset issue
> 
> On 10/10/20 4:21 AM, Jun Li wrote:

...

> >>
> >> Like I mentioned before, whatever the condition is, hard_reset_count
> >> must be reset to zero during tcpm_detach.
> >
> > This may not be so correct as you said, I think Guenter's concern is valid.
> >
> > tcpm_detach() is not *only* be called in cases of *physical* detach
> > like the function name suggests.
> >
> > The current proposals may *wrongly* reset this counter.
> >
> > Let me give an example:
> >
> > HARD_RESET_SEND -> HARD_RESET_START -> SNK_HARD_RESET_SINK_OFF ->
> > SNK_HARD_RESET_WAIT_VBUS -> SNK_UNATTACHED(in case of VBUS doesn't
> > come back in expected duration)
> > -> call to tcpm_detach()
> >
> > In this sequence, does the sink need keep the counter? From the PD
> > spec, I think the answer is yes, sink need this counter to judge if
> > need do hard reset again or error recovery on later try, see:
> >
> > Figure 8-67 Sink Port State Diagram
> >
> > The difference between your case and my example, is your case never
> > turn off vbus, so will not go to SNK_UNATTACHED, so will not call to
> > tcpm_detach() after first hard reset.
> >
> > So
> > 	if (tcpm_port_is_disconnected(port))
> > 		port->hard_reset_count = 0;
> >
> > should keep as it is, the counter can only be cleared if there is
> > really physical disconnect by *partner*.
> >
> > But current tcpm code may have some problem on keeping local CC state
> > if there is hard reset on-going(port->hard_reset_count > 0), because
> > the current handling of SNK_UNATTACHED may cause disconnection
> > generated by *local*(partner actually keeps its CC), e.g. reset
> > polarity and do drp_toggling, this should be fixed, but this is
> > another patch, I can try to do this later.
> >
> > Back to your problem, I think the issue is, at the first tcpm_detach()
> > the cc disconnect event hasn't happen, so the reset counter is left
> > there but the port->attached is cleared, then the following(real
> > disconnect)
> > tcpm_detach() will just return directly due to port->attached is false.
> >
> > So I guess this will resolve your problem:
> >
> > @@ -2885,6 +2885,9 @@ static void tcpm_reset_port(struct tcpm_port
> > *port)
> >
> >  static void tcpm_detach(struct tcpm_port *port)  {
> > +       if (tcpm_port_is_disconnected(port))
> > +               port->hard_reset_count = 0;
> > +
> >         if (!port->attached)
> >                 return;
> >
> > @@ -2893,9 +2896,6 @@ 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;
> > -
> >         tcpm_reset_port(port);
> >  }
> >
> >
> > Compared with current code's condition:
> >    3 static bool tcpm_port_is_disconnected(struct tcpm_port *port)
> >    4 {
> >    5         return (!port->attached && port->cc1 == TYPEC_CC_OPEN &&
> >    6                 port->cc2 == TYPEC_CC_OPEN) ||
> >    7                (port->attached && ((port->polarity ==
> TYPEC_POLARITY_CC1 &&
> >    8                                     port->cc1 == TYPEC_CC_OPEN) ||
> >    9                                    (port->polarity ==
> TYPEC_POLARITY_CC2 &&
> >   10                                     port->cc2 == TYPEC_CC_OPEN)));
> >   11 }
> >
> > My above change is only adding another condition to clear the reset counter:
> > (!port->attached && port->cc1 == TYPEC_CC_OPEN && port->cc2 ==
> > TYPEC_CC_OPEN)
> >
> > This condition is close to Guenter's suggestion in this thread:
> >
> > -       if (tcpm_port_is_disconnected(port))
> > +       if (tcpm_port_is_disconnected(port) ||
> > +           (tcpm_cc_is_open(port->cc1) &&
> > + tcpm_cc_is_open(port->cc2)))
> >
> > Hi Guenter, any comments on this?
> >
> 
> That makes sense, and I would agree to the change you suggest above.

Thanks.

Li Jun
> 
> Guenter
> 
> > Thanks
> > Li Jun
> >
> >>
> >> But refer to Guenter's mail,  he prefer to narrow down the condition
> >> to reset this counter.
> >>
> >> I think the original thought is important why to put this line there.
> >>
> >> Hi, Guenter:
> >>    From the discussion, we really need to know why you put the reset
> >> line below port attached is false and also make some judgement.
> >> I think there may be ome condition that we don't considered.
> >
> > This condition was added at first place, I think my above
> >>
> >>>
> >>>>
> >>>>>>>>
> >>>>>>>> Guenter

Jun Li Oct. 12, 2020, 9:25 a.m. UTC | #24

> -----Original Message-----
> From: ChiYuan Huang <[e-mail]>
> Sent: Monday, October 12, 2020 2:23 PM
> To: Guenter Roeck <[e-mail]>
> Cc: Jun Li <[e-mail]>; Jun Li <[e-mail]>; Greg KH
> <[e-mail]>; Heikki Krogerus
> <[e-mail]>; Linux USB List
> <[e-mail]>; lkml <[e-mail]>;
> cy_huang <[e-mail]>
> Subject: Re: [PATCH] usb: typec: tcpm: Fix if vbus before cc, hard_reset_count
> not reset issue
> 
> Guenter Roeck <[e-mail]> 於 2020年10月11日 週日 上午3:31寫道：
> >
> > On 10/10/20 4:21 AM, Jun Li wrote:
> > >
> > >
> > >> -----Original Message-----
> > >> From: ChiYuan Huang <[e-mail]>
> > >> Sent: Saturday, October 10, 2020 12:06 AM
> > >> To: Jun Li <[e-mail]>
> > >> Cc: Jun Li <[e-mail]>; Guenter Roeck
> > >> <[e-mail]>; Greg KH <[e-mail]>; Heikki
> > >> Krogerus <[e-mail]>; Linux USB List
> > >> <[e-mail]>; lkml <[e-mail]>;
> > >> cy_huang <[e-mail]>
> > >> Subject: Re: [PATCH] usb: typec: tcpm: Fix if vbus before cc,
> > >> hard_reset_count not reset issue
> > >>
> > >> Jun Li <[e-mail]> 於 2020年10月9日 週五 下午2:12寫道：
> > >>>
> > >>>
> > >>>
> > >>>> -----Original Message-----
> > >>>> From: ChiYuan Huang <[e-mail]>
> > >>>> Sent: Wednesday, October 7, 2020 6:13 PM
> > >>>> To: Jun Li <[e-mail]>
> > >>>> Cc: Guenter Roeck <[e-mail]>; Greg KH
> > >>>> <[e-mail]>; Heikki Krogerus
> > >>>> <[e-mail]>; Linux USB List
> > >>>> <[e-mail]>; lkml <[e-mail]>;
> > >>>> cy_huang <[e-mail]>; Jun Li <[e-mail]>
> > >>>> Subject: Re: [PATCH] usb: typec: tcpm: Fix if vbus before cc,
> > >>>> hard_reset_count not reset issue
> > >>>>
> > >>>> ChiYuan Huang <[e-mail]> 於 2020年10月7日 週三 上午1:39
> 寫
> > >> 道：
> > >>>>>
> > >>>>> Jun Li <[e-mail]> 於 2020年10月7日 週三 上午12:52
> 寫
> > >> 道：
> > >>>>>>
> > >>>>>> ChiYuan Huang <[e-mail]> 于2020年10月6日周二 下午12:38
> > >> 写
> > >>>> 道：
> > >>>>>>>
> > >>>>>>> Guenter Roeck <[e-mail]> 於 2020年10月5日 週一 下午
> > >> 11:30
> > >>>> 寫道：
> > >>>>>>>>
> > >>>>>>>> On 10/5/20 4:08 AM, Greg KH wrote:
> > >>>>>>>> [ ... ]
> > >>>>>>>>>>> What ever happened with this patch, is there still disagreement?
> > >>>>>>>>>>>
> > >>>>>>>>>>
> > >>>>>>>>>> Yes, there is. I wouldn't have added the conditional
> > >>>>>>>>>> without reason, and I am concerned that removing it
> > >>>>>>>>>> entirely will open
> > >>>> another problem.
> > >>>>>>>>>> Feel free to apply, though - I can't prove that my concern
> > >>>>>>>>>> is valid, and after all we'll get reports from the field
> > >>>>>>>>>> later if
> > >>>> it is.
> > >>>>>>>>>
> > >>>>>>>>> Ok, can I get an ack so I know who to come back to in the
> > >>>>>>>>> future if there are issues?  :)
> > >>>>>>>>>
> > >>>>>>>>
> > >>>>>>>> Not from me, for the reasons I stated. I would be ok with
> > >>>>>>>> something
> > >>>> like:
> > >>>>>>>>
> > >>>>>>>> -       if (tcpm_port_is_disconnected(port))
> > >>>>>>>> +       if (tcpm_port_is_disconnected(port) ||
> > >>>>>>>> +           (tcpm_cc_is_open(port->cc1) &&
> > >>>>>>>> + tcpm_cc_is_open(port->cc2)))
> > >>>>>>>>
> > >>>>>>>> to narrow down the condition.
> > >>>>>>>
> > >>>>>>> I have tried the above comment and It doesn't work.
> > >>>>>>> How about to change the judgement like as below
> > >>>>>>>
> > >>>>>>> -       if (tcpm_port_is_disconnected(port))
> > >>>>>>> +       if (tcpm_port_is_disconnected(port) ||
> > >>>>>>> + !port->vbus_present)
> > >>>>>>>
> > >>>>>>> The hard_reset_count not reset issue is following by the below
> > >>>>>>> order 1. VBUS off ( at the same time, cc is still detected as
> > >>>>>>> attached)
> > >>>>>>> port->attached become false and cc is not open
> > >>>>>>> 2. After that, cc detached.
> > >>>>>>> due to port->attached is false, tcpm_detach() directly return.
> > >>>>>>
> > >>>>>> If tcpm_detach() return directly, then how your patch can reset
> > >>>>>> hard_reset_count?
> > >>>>>>
> > >>>>> Yes, it can. We know vbus_present change from true to false and
> > >>>>> cc detach both trigger tcpm_detach.
> > >>>>> My change is whenever tcpm_detach to be called, hard_reset_count
> > >>>>> will be reset to zero.
> > >>>>>
> > >>>>>> I am seeing the same issue on my platform, the proposed change:
> > >>>>>> -       if (tcpm_port_is_disconnected(port))
> > >>>>>> -               port->hard_reset_count = 0;
> > >>>>>> +       port->hard_reset_count = 0;
> > >>>>>> can't resolve it on my platform.
> > >>>>>>
> > >>>>> I'm not sure what's your condition. Could you directly paste the
> > >>>>> tcpm log for the check?
> > >>>>>> How about reset hard_reset_count in SNK_READY?
> > >>>>>> @@ -3325,6 +3329,7 @@ static void run_state_machine(struct
> > >>>>>> tcpm_port
> > >>>> *port)
> > >>>>>>         case SNK_READY:
> > >>>>>>                 port->try_snk_count = 0;
> > >>>>>>                 port->update_sink_caps = false;
> > >>>>>> +               port->hard_reset_count = 0;
> > >>>>>>                 if (port->explicit_contract) {
> > >>>>>>                         typec_set_pwr_opmode(port->typec_port,
> > >>>>>>
> > >>>>>> TYPEC_PWR_MODE_PD);
> > >>>>>>
> > >>>>>> can this resolve your problem?
> > >>>>> I'm not sure. It need to have a try, then I can answer you.
> > >>>>> But from USBPD spec, the hard_reset_count need to reset zero
> > >>>>> only when 1. At src state, pe_src_send_cap and receive GoodCRC
> > >>>>> 2. At snk state, pe_snk_evaluate_cap need to reset
> > >>>>> hard_reset_count
> > >>>
> > >>> 3.
> > >>> 8.3.3.3.8 PE_SNK_Hard_Reset state
> > >>> "Note: The HardResetCounter is reset on a power cycle or Detach."
> > >>>
> > >>>>>>
> > >>>>>> Li Jun
> > >>>>>>>
> > >>>>>>> And that's why hard_reset_count is not reset to 0.
> > >>>>
> > >>>> I tried in snk_ready to reset hard_reset_count.
> > >>>> At normal case, it works.
> > >>>> But it seems still the possible fail case like as below.
> > >>>> 200ms -> cc debounce max time
> > >>>> 240ms -> snk_waitcap max time
> > >>>> If the plugin/out period is between (200+240) and (200+ 2* 240)ms
> > >>>> , and the src side plug out like as the described scenario.
> > >>>> From this case, hard_reset_count may still 1.
> > >>>> And we expect the next plugin hard_reset_count is 0. But not,
> > >>>> actually it never reset.
> > >>>> So at next plugin, only one hard_reset will be sent and reach
> > >>>> hard_reset_count max (2).
> > >>>>
> > >>>> This case is hard to reproduce. But actually it's possible.
> > >>>
> > >>> Make sense.
> > >>>
> > >>> Then I propose doing this at SNK_UNATTACHED @@ -3156,6 +3156,7 @@
> > >>> static void run_state_machine(struct tcpm_port *port)
> > >>>                 if (!port->non_pd_role_swap)
> > >>>                         tcpm_swap_complete(port, -ENOTCONN);
> > >>>                 tcpm_pps_complete(port, -ENOTCONN);
> > >>> +               port->hard_reset_count = 0;
> > >>>                 tcpm_snk_detach(port);
> > >>>                 if (tcpm_start_toggling(port, TYPEC_CC_RD)) {
> > >>>                         tcpm_set_state(port, TOGGLING, 0); Li Jun
> > >>
> > >> For the current power role is snk, I think it may work.
> > >> How about the src role? src role only reset the hard_reset_count in
> > >> tcpm_detach and src_ready state?
> > >
> > > Sorry, after gave more check on PD sped, this isn't a right solution.
> > > See below
> > >
> > >>
> > >> I check the flow that  you mentioned in the previous mail. It's
> > >> really a special case from any state to port_reset.
> > >> If the case is considered, how about to reset  the hard_reset_count
> > >> and don't care the port is attached or not in tcpm_detach function like
> as below.
> > >>
> > >> @@ -2789,6 +2789,8 @@ static void tcpm_reset_port(struct tcpm_port
> > >> *port)
> > >>
> > >>  static void tcpm_detach(struct tcpm_port *port)  {
> > >> +       port->hard_reset_count = 0;
> > >> +
> > >>         if (!port->attached)
> > >>                 return;
> > >>
> > >> @@ -2797,9 +2799,6 @@ 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;
> > >> -
> > >>         tcpm_reset_port(port);
> > >>  }
> > >>
> > >> Like I mentioned before, whatever the condition is,
> > >> hard_reset_count must be reset to zero during tcpm_detach.
> > >
> > > This may not be so correct as you said, I think Guenter's concern is
> valid.
> > >
> > > tcpm_detach() is not *only* be called in cases of *physical* detach
> > > like the function name suggests.
> > >
> > > The current proposals may *wrongly* reset this counter.
> > >
> > > Let me give an example:
> > >
> > > HARD_RESET_SEND -> HARD_RESET_START -> SNK_HARD_RESET_SINK_OFF ->
> > > SNK_HARD_RESET_WAIT_VBUS -> SNK_UNATTACHED(in case of VBUS doesn't
> > > come back in expected duration)
> > > -> call to tcpm_detach()
> > >
> > > In this sequence, does the sink need keep the counter? From the PD
> > > spec, I think the answer is yes, sink need this counter to judge if
> > > need do hard reset again or error recovery on later try, see:
> > >
> > > Figure 8-67 Sink Port State Diagram
> > >
> > > The difference between your case and my example, is your case never
> > > turn off vbus, so will not go to SNK_UNATTACHED, so will not call to
> > > tcpm_detach() after first hard reset.
> > >
> > > So
> > >       if (tcpm_port_is_disconnected(port))
> > >               port->hard_reset_count = 0;
> > >
> > > should keep as it is, the counter can only be cleared if there is
> > > really physical disconnect by *partner*.
> > >
> > > But current tcpm code may have some problem on keeping local CC
> > > state if there is hard reset on-going(port->hard_reset_count > 0),
> > > because the current handling of SNK_UNATTACHED may cause
> > > disconnection generated by *local*(partner actually keeps its CC),
> > > e.g. reset polarity and do drp_toggling, this should be fixed, but
> > > this is another patch, I can try to do this later.
> > >
> > > Back to your problem, I think the issue is, at the first
> > > tcpm_detach() the cc disconnect event hasn't happen, so the reset
> > > counter is left there but the port->attached is cleared, then the
> > > following(real disconnect)
> > > tcpm_detach() will just return directly due to port->attached is false.
> > >
> > > So I guess this will resolve your problem:
> > >
> > > @@ -2885,6 +2885,9 @@ static void tcpm_reset_port(struct tcpm_port
> > > *port)
> > >
> > >  static void tcpm_detach(struct tcpm_port *port)  {
> > > +       if (tcpm_port_is_disconnected(port))
> > > +               port->hard_reset_count = 0;
> > > +
> > >         if (!port->attached)
> > >                 return;
> > >
> > > @@ -2893,9 +2896,6 @@ 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;
> > > -
> > >         tcpm_reset_port(port);
> > >  }
> > >
> > >
> I have checked this patch. It can solve the problem.

Good, so this can resolve both your and my problems.

> 
> > > Compared with current code's condition:
> > >    3 static bool tcpm_port_is_disconnected(struct tcpm_port *port)
> > >    4 {
> > >    5         return (!port->attached && port->cc1 == TYPEC_CC_OPEN &&
> > >    6                 port->cc2 == TYPEC_CC_OPEN) ||
> > >    7                (port->attached && ((port->polarity ==
> TYPEC_POLARITY_CC1 &&
> > >    8                                     port->cc1 == TYPEC_CC_OPEN) ||
> > >    9                                    (port->polarity ==
> TYPEC_POLARITY_CC2 &&
> > >   10                                     port->cc2 == TYPEC_CC_OPEN)));
> > >   11 }
> > >
> > > My above change is only adding another condition to clear the reset counter:
> > > (!port->attached && port->cc1 == TYPEC_CC_OPEN && port->cc2 ==
> > > TYPEC_CC_OPEN)
> > >
> > > This condition is close to Guenter's suggestion in this thread:
> > >
> > > -       if (tcpm_port_is_disconnected(port))
> > > +       if (tcpm_port_is_disconnected(port) ||
> > > +           (tcpm_cc_is_open(port->cc1) &&
> > > + tcpm_cc_is_open(port->cc2)))
> > >
> > > Hi Guenter, any comments on this?
> > >
> >
> > That makes sense, and I would agree to the change you suggest above.
> Jun:
>    will you send the patch following the final discussion? Or I also can
> help.
> Thx.

Ok, I will send a formal patch for this.

thanks
Li Jun 

> >
> > Guenter
> >
> > > Thanks
> > > Li Jun
> > >
> > >>
> > >> But refer to Guenter's mail,  he prefer to narrow down the
> > >> condition to reset this counter.
> > >>
> > >> I think the original thought is important why to put this line there.
> > >>
> > >> Hi, Guenter:
> > >>    From the discussion, we really need to know why you put the
> > >> reset line below port attached is false and also make some judgement.
> > >> I think there may be ome condition that we don't considered.
> > >
> > > This condition was added at first place, I think my above
> > >>
> > >>>
> > >>>>
> > >>>>>>>>
> > >>>>>>>> Guenter
> >

Patch

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;
 
 	tcpm_reset_port(port);
 }