Message-ID: <VE1PR04MB6528165DAECBCD18083AEDDE89070@VE1PR04MB6528.eurprd04.prod.outlook.com>
Date:   Mon, 12 Oct 2020 08:58:17 +0000
From:   Jun Li <jun.li@....com>
To:     Guenter Roeck <linux@...ck-us.net>,
        ChiYuan Huang <u0084500@...il.com>
CC:     Jun Li <lijun.kernel@...il.com>,
        Greg KH <gregkh@...uxfoundation.org>,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Linux USB List <linux-usb@...r.kernel.org>,
        lkml <linux-kernel@...r.kernel.org>,
        cy_huang <cy_huang@...htek.com>
Subject: RE: [PATCH] usb: typec: tcpm: Fix if vbus before cc, hard_reset_count
 not reset issue



> -----Original Message-----
> From: Guenter Roeck <groeck7@...il.com> On Behalf Of Guenter Roeck
> Sent: Sunday, October 11, 2020 3:32 AM
> To: Jun Li <jun.li@....com>; ChiYuan Huang <u0084500@...il.com>
> Cc: Jun Li <lijun.kernel@...il.com>; Greg KH <gregkh@...uxfoundation.org>;
> Heikki Krogerus <heikki.krogerus@...ux.intel.com>; Linux USB List
> <linux-usb@...r.kernel.org>; lkml <linux-kernel@...r.kernel.org>;
> cy_huang <cy_huang@...htek.com>
> 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

Powered by blists - more mailing lists