Message-ID: <20201005110808.GA298743@kroah.com>
Date:   Mon, 5 Oct 2020 13:08:08 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Guenter Roeck <linux@...ck-us.net>
Cc:     ChiYuan Huang <u0084500@...il.com>,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        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

On Fri, Oct 02, 2020 at 07:26:24AM -0700, Guenter Roeck wrote:
> On 10/2/20 6:31 AM, Greg KH wrote:
> > On Tue, Sep 15, 2020 at 11:07:18AM +0800, ChiYuan Huang wrote:
> >> Hi, Guenter:
> >>
> >> ChiYuan Huang <u0084500@...il.com> 於 2020年9月6日 週日 下午11:22寫道：
> >>>
> >>> 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().
> >>>
> >> Below's the real case log.
> >> [ 4848.046358] VBUS off
> >> [ 4848.046384] state change SNK_READY -> SNK_UNATTACHED
> >> [ 4848.050908] Setting voltage/current limit 0 mV 0 mA
> >> [ 4848.050936] polarity 0
> >> [ 4848.052593] Requesting mux state 0, usb-role 0, orientation 0
> >> [ 4848.053222] Start toggling
> >> [ 4848.086500] state change SNK_UNATTACHED -> TOGGLING
> >> [ 4848.089983] CC1: 0 -> 0, CC2: 3 -> 3 [state TOGGLING, polarity 0, connected]
> >> [ 4848.089993] state change TOGGLING -> SNK_ATTACH_WAIT
> >> [ 4848.090031] pending state change SNK_ATTACH_WAIT -> SNK_DEBOUNCED @ 200 ms
> >> [ 4848.141162] CC1: 0 -> 0, CC2: 3 -> 0 [state SNK_ATTACH_WAIT,
> >> polarity 0, disconnected]
> >> [ 4848.141170] state change SNK_ATTACH_WAIT -> SNK_ATTACH_WAIT
> >> [ 4848.141184] pending state change SNK_ATTACH_WAIT -> SNK_UNATTACHED @ 20 ms
> >> [ 4848.163156] state change SNK_ATTACH_WAIT -> SNK_UNATTACHED [delayed 20 ms]
> >> [ 4848.163162] Start toggling
> >> [ 4848.216918] CC1: 0 -> 0, CC2: 0 -> 3 [state TOGGLING, polarity 0, connected]
> >> [ 4848.216954] state change TOGGLING -> SNK_ATTACH_WAIT
> >> [ 4848.217080] pending state change SNK_ATTACH_WAIT -> SNK_DEBOUNCED @ 200 ms
> >> [ 4848.231771] CC1: 0 -> 0, CC2: 3 -> 0 [state SNK_ATTACH_WAIT,
> >> polarity 0, disconnected]
> >> [ 4848.231800] state change SNK_ATTACH_WAIT -> SNK_ATTACH_WAIT
> >> [ 4848.231857] pending state change SNK_ATTACH_WAIT -> SNK_UNATTACHED @ 20 ms
> >> [ 4848.256022] state change SNK_ATTACH_WAIT -> SNK_UNATTACHED [delayed 20 ms]
> >> [ 4848.256049] Start toggling
> >> [ 4848.871148] VBUS on
> >> [ 4848.885324] CC1: 0 -> 0, CC2: 0 -> 3 [state TOGGLING, polarity 0, connected]
> >> [ 4848.885372] state change TOGGLING -> SNK_ATTACH_WAIT
> >> [ 4848.885548] pending state change SNK_ATTACH_WAIT -> SNK_DEBOUNCED @ 200 ms
> >> [ 4849.088240] state change SNK_ATTACH_WAIT -> SNK_DEBOUNCED [delayed 200 ms]
> >> [ 4849.088284] state change SNK_DEBOUNCED -> SNK_ATTACHED
> >> [ 4849.088291] polarity 1
> >> [ 4849.088769] Requesting mux state 1, usb-role 2, orientation 2
> >> [ 4849.088895] state change SNK_ATTACHED -> SNK_STARTUP
> >> [ 4849.088907] state change SNK_STARTUP -> SNK_DISCOVERY
> >> [ 4849.088915] Setting voltage/current limit 5000 mV 0 mA
> >> [ 4849.088927] vbus=0 charge:=1
> >> [ 4849.090505] state change SNK_DISCOVERY -> SNK_WAIT_CAPABILITIES
> >> [ 4849.090828] pending state change SNK_WAIT_CAPABILITIES -> SNK_READY @ 240 ms
> >> [ 4849.335878] state change SNK_WAIT_CAPABILITIES -> SNK_READY [delayed 240 ms]
> >>
> >> You can see the next type c attach log.
> >> It directly change state from SNK_WAIT_CAPABILITIES to SNK_READY due
> >> to not reset hard_reset_count.
> >>
> >> It's easy to reproduce if you plugout USB Adapater w/i AtoC cable connected.
> > 
> > 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?  :)

thanks,

greg k-h

Powered by blists - more mailing lists