Re: [PATCH] usb: typec: tcpm: Fix if vbus before cc, hard_reset_count not reset issue

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

On 10/10/20 4:21 AM, Jun Li wrote:
> 
> 
>> -----Original Message-----
>> From: ChiYuan Huang <u0084500@gmail.com>
>> Sent: Saturday, October 10, 2020 12:06 AM
>> To: Jun Li <jun.li@nxp.com>
>> Cc: Jun Li <lijun.kernel@gmail.com>; Guenter Roeck <linux@roeck-us.net>;
>> Greg KH <gregkh@linuxfoundation.org>; Heikki Krogerus
>> <heikki.krogerus@linux.intel.com>; Linux USB List
>> <linux-usb@vger.kernel.org>; lkml <linux-kernel@vger.kernel.org>;
>> cy_huang <cy_huang@richtek.com>
>> Subject: Re: [PATCH] usb: typec: tcpm: Fix if vbus before cc, hard_reset_count
>> not reset issue
>>
>> Jun Li <jun.li@nxp.com> 於 2020年10月9日 週五 下午2:12寫道：
>>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: ChiYuan Huang <u0084500@gmail.com>
>>>> Sent: Wednesday, October 7, 2020 6:13 PM
>>>> To: Jun Li <lijun.kernel@gmail.com>
>>>> Cc: Guenter Roeck <linux@roeck-us.net>; Greg KH
>>>> <gregkh@linuxfoundation.org>; Heikki Krogerus
>>>> <heikki.krogerus@linux.intel.com>; Linux USB List
>>>> <linux-usb@vger.kernel.org>; lkml <linux-kernel@vger.kernel.org>;
>>>> cy_huang <cy_huang@richtek.com>; Jun Li <jun.li@nxp.com>
>>>> Subject: Re: [PATCH] usb: typec: tcpm: Fix if vbus before cc,
>>>> hard_reset_count not reset issue
>>>>
>>>> ChiYuan Huang <u0084500@gmail.com> 於 2020年10月7日 週三 上午1:39寫
>> 道：
>>>>>
>>>>> Jun Li <lijun.kernel@gmail.com> 於 2020年10月7日 週三 上午12:52寫
>> 道：
>>>>>>
>>>>>> ChiYuan Huang <u0084500@gmail.com> 于2020年10月6日周二 下午12:38
>> 写
>>>> 道：
>>>>>>>
>>>>>>> Guenter Roeck <linux@roeck-us.net> 於 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