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

From: ChiYuan Huang <u0084500@gmail.com>
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>
Subject: Re: [PATCH] usb: typec: tcpm: Fix if vbus before cc, hard_reset_count not reset issue
Date: Fri, 4 Sep 2020 00:21:56 +0800	[thread overview]
Message-ID: <CADiBU3_iHk4aoM8o6GcaTmWDZT4ymvb0Ff-XeLLZ0C9dhCnLZQ@mail.gmail.com> (raw)
In-Reply-To: <20200902165713.GG56237@roeck-us.net>

Guenter Roeck <linux@roeck-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@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_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.

> Guenter
>
> >       tcpm_reset_port(port);
> >  }
> > --
> > 2.7.4
> >