Message-ID: <CADiBU3-83rVLqhVAqqSGc0qQ66PHsGVVcp_m3sm_4ZS5A+GXKQ@mail.gmail.com>
Date:   Sun, 6 Sep 2020 23:22:40 +0800
From:   ChiYuan Huang <u0084500@...il.com>
To:     Guenter Roeck <linux@...ck-us.net>
Cc:     Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Greg KH <gregkh@...uxfoundation.org>,
        linux-usb@...r.kernel.org, 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

Guenter Roeck <linux@...ck-us.net> 於 2020年9月5日 週六 下午11:51寫道：
>
> On 9/4/20 6:24 PM, ChiYuan Huang wrote:
> > Guenter Roeck <linux@...ck-us.net> 於 2020年9月5日 週六 上午3:41寫道：
> >>
> >> On 9/3/20 9:21 AM, ChiYuan Huang wrote:
> >>> Guenter Roeck <linux@...ck-us.net> 於 2020年9月3日 週四 上午12:57寫道：
> >>>>
> >>>> On Wed, Sep 02, 2020 at 11:35:33PM +0800, cy_huang wrote:
> >>>>> From: ChiYuan Huang <cy_huang@...htek.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@...htek.com>
> >>>>> ---
> >>>>> Below's the flow.
> >>>>>
> >>>>> _tcpm_pd_vbus_off() -> run_state_machine to change state to SNK_UNATTACHED
> >>>>> 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=open and port->cc2=open
> >>>>>
> >>>>> It cause tcpm_port_is_disconnected return false, then hard_reset_count 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/tcpm.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 = 0;
> >>>>> +     port->hard_reset_count = 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 could be a local error (with the local hardware), or with the
> remote partner not accepting the reset. We only know that an 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.
> >
> The operational word is "normal". Error recovery is expected to handle
> situations which are not normal.

Following by the USBPD 3.0 revision 1.2, section 8.3.3.24.1
The ErrorRecovery state is  used to electronically disconnect Port
Partner using the USB Type-C connector.
And there's one sentence to be said "The ErrorRecovery staste shall
map to USB Type-C Error Recovery state operations".
I also read ErrorRecovery state in USB TYPE-C 1.3 spec.
Section 4.5.2.2.2.1   ErrorRecovery state requirement listed the below text.
The port shall not drive VBUS or VCONN, and shall present a
high-impedance to ground (above
zOPEN) on its CC1 and CC2 pins.
Section 4.5.2.2.2.2 Exiting from the error recovery state
I read the description. The roughly meaning is to change the state to
Unattached(Src or Snk) after tErrorRecovery.

Summary the above text.
Reset HardResetCounter is ok in tcpm_detach.
My patch is just to relax the counter reset conditions during tcpm_detach().
If not, it will check tcpm_port_is_disconnected().
And only two scenario, the hard reset count will be cleared to 0 at this case.
1) port not attached and cc1=open and cc2=open
2) port attached and either (polarity=cc1, cc1=open) or (polarity=cc2, cc2=open)

I think this judgement is narrow in tcpm_detach case.

>
> I don't question the need to reset the counter. The only question
> is where and when to reset it.
>
I re-check all tcpm code for hard reset counter about the increment and reset.
They all meets USBPD spec. Only the detach case, I'm wondering why it
need to add the check for tcpm_port_is_disconnected().

> Guenter

Powered by blists - more mailing lists