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

[Jun Li <jun.li@nxp.com>]

> -----Original Message-----
> From: ChiYuan Huang <u0084500@gmail.com>
> Sent: Monday, October 12, 2020 2:23 PM
> To: Guenter Roeck <linux@roeck-us.net>
> Cc: Jun Li <jun.li@nxp.com>; Jun Li <lijun.kernel@gmail.com>; 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
> 
> Guenter Roeck <linux@roeck-us.net> 於 2020年10月11日 週日 上午3:31寫道：
> >
> > 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);
> > >  }
> > >
> > >
> 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
> >