Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <1599060933-8092-1-git-send-email-u0084500@gmail.com>
 <20200902165713.GG56237@roeck-us.net> <CADiBU3_iHk4aoM8o6GcaTmWDZT4ymvb0Ff-XeLLZ0C9dhCnLZQ@mail.gmail.com>
 <fd2a33fc-2383-66cb-0fd7-d5aa0cc9111f@roeck-us.net>
In-Reply-To: <fd2a33fc-2383-66cb-0fd7-d5aa0cc9111f@roeck-us.net>
From:   ChiYuan Huang <u0084500@gmail.com>
Date:   Sat, 5 Sep 2020 09:24:50 +0800
Message-ID: <CADiBU3_vYAmHDCONrExzyM+1CTfqJx_eS1hYG8aHkNWFzTcwfg@mail.gmail.com>
Subject: Re: [PATCH] usb: typec: tcpm: Fix if vbus before cc, hard_reset_count
 not reset issue
To:     Guenter Roeck <linux@roeck-us.net>
Cc:     Heikki Krogerus <heikki.krogerus@linux.intel.com>,
        Greg KH <gregkh@linuxfoundation.org>,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
        cy_huang <cy_huang@richtek.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Guenter Roeck <linux@roeck-us.net> =E6=96=BC 2020=E5=B9=B49=E6=9C=885=E6=97=
=A5 =E9=80=B1=E5=85=AD =E4=B8=8A=E5=8D=883:41=E5=AF=AB=E9=81=93=EF=BC=9A
>
> On 9/3/20 9:21 AM, ChiYuan Huang wrote:
> > Guenter Roeck <linux@roeck-us.net> =E6=96=BC 2020=E5=B9=B49=E6=9C=883=
=E6=97=A5 =E9=80=B1=E5=9B=9B =E4=B8=8A=E5=8D=8812:57=E5=AF=AB=E9=81=93=EF=
=BC=9A
> >>
> >> On Wed, Sep 02, 2020 at 11:35:33PM +0800, cy_huang wrote:
> >>> From: ChiYuan Huang <cy_huang@richtek.com>
> >>>
> >>> Fix: If vbus event is before cc_event trigger, hard_reset_count
> >>> won't bt reset for some case.
> >>>
> >>> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> >>> ---
> >>> Below's the flow.
> >>>
> >>> _tcpm_pd_vbus_off() -> run_state_machine to change state to SNK_UNATT=
ACHED
> >>> 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=3Dopen and port->cc2=
=3Dopen
> >>>
> >>> It cause tcpm_port_is_disconnected return false, then hard_reset_coun=
t 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/t=
cpm.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 =3D 0;
> >>> +     port->hard_reset_count =3D 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
> >>>
>