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