linux-kernel - Re: [PATCH 4/4] usb: typec: tcpm: fix source caps may lost after soft reset

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7038030f-1d7a-4c91-a255-bec73847b7c9@roeck-us.net>
Date:   Sun, 19 Mar 2023 20:35:17 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     Frank Wang <frank.wang@...k-chips.com>
Cc:     heikki.krogerus@...ux.intel.com, gregkh@...uxfoundation.org,
        heiko@...ech.de, linux-usb@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-rockchip@...ts.infradead.org,
        huangtao@...k-chips.com, william.wu@...k-chips.com,
        jianwei.zheng@...k-chips.com, yubing.zhang@...k-chips.com,
        wmc@...k-chips.com
Subject: Re: [PATCH 4/4] usb: typec: tcpm: fix source caps may lost after
 soft reset

On Mon, Mar 20, 2023 at 10:38:35AM +0800, Frank Wang wrote:
> Hi Guenter,
> 
> On 2023/3/17 20:58, Guenter Roeck wrote:
> > On 3/12/23 19:58, Frank Wang wrote:
> > > Invoke set_pd_rx() may flush the RX FIFO of PD controller, so do
> > > set_pd_rx() before sending Soft Reset in case Source caps may be flushed
> > > at debounce time between SOFT_RESET_SEND and SNK_WAIT_CAPABILITIES
> > > state.
> > > 
> > 
> > Isn't that a problem of the fusb302 driver that it flushes its buffers
> > unconditionally when its set_pd_rx() callback is called ?
> > 
> > Guenter
> > 
> 
> The story goes like this,  the fusb302 notified SOFT_RESET completion to
> TCPM and began to receive the Source Caps automatically,
> TCPM got completion from fusb302 and changed state to SNK_WAIT_CAPABILITIES
> and invoked set_pd_rx() callback. However, the
> fusb302 or TCPM's worker may be not scheduled in time, set_pd_rx() would be
> performed after the Source Caps packets had received
> into fusb302's FIFO, even after the GoodCRC (Source Caps) had be replied.
> 

Yes, but the fusb302 driver clears its fifo in the set_pd_rx() callback.
I see that as a problem in the fusb302 driver( it could clear its fifo when
it notifies the TCPM that soft reset is complete, for example), and I am
hesitant to work around that problem in the tcpm code.

Guenter

> So make forward set_pd_rx() process before PD_CTRL_SOFT_RESET sent at
> SOFT_RESET_SEND state can cleanup the context in our side
> and ensure the right PD commucation. I am not sure whether it is sensible?
> 
> BR.
> Frank
> 
> > > Without this patch, in PD charger stress test, the FUSB302 driver may
> > > occur the following exceptions in power negotiation stage.
> > > 
> > > [ ...]
> > > [ 4.512252] fusb302_irq_intn
> > > [ 4.512260] AMS SOFT_RESET_AMS finished
> > > [ 4.512269] state change SOFT_RESET_SEND ->SNK_WAIT_CAPABILITIES
> > > [rev3 NONE_AMS]
> > > [ 4.514511] pd := on
> > > [ 4.514516] pending state change SNK_WAIT_CAPABILITIES
> > > ->HARD_RESET_SEND @ 310 ms [rev3 NONE_AMS]
> > > [ 4.515428] IRQ: 0x51, a: 0x00, b: 0x01, status0: 0x93
> > > [ 4.515431] IRQ: BC_LVL, handler pending
> > > [ 4.515435] IRQ: PD sent good CRC
> > > [ 4.516434] PD message header: 0
> > > [ 4.516437] PD message len: 0
> > > [ 4.516444] PD RX, header: 0x0 [1]
> > > 
> > > Signed-off-by: Frank Wang <frank.wang@...k-chips.com>
> > > ---
> > >   drivers/usb/typec/tcpm/tcpm.c | 11 +++++++----
> > >   1 file changed, 7 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/usb/typec/tcpm/tcpm.c
> > > b/drivers/usb/typec/tcpm/tcpm.c
> > > index 9e583060e64fc..ba6bf71838eed 100644
> > > --- a/drivers/usb/typec/tcpm/tcpm.c
> > > +++ b/drivers/usb/typec/tcpm/tcpm.c
> > > @@ -4321,10 +4321,12 @@ static void run_state_machine(struct
> > > tcpm_port *port)
> > >           tcpm_set_state(port, unattached_state(port), 0);
> > >           break;
> > >       case SNK_WAIT_CAPABILITIES:
> > > -        ret = port->tcpc->set_pd_rx(port->tcpc, true);
> > > -        if (ret < 0) {
> > > -            tcpm_set_state(port, SNK_READY, 0);
> > > -            break;
> > > +        if (port->prev_state != SOFT_RESET_SEND) {
> > > +            ret = port->tcpc->set_pd_rx(port->tcpc, true);
> > > +            if (ret < 0) {
> > > +                tcpm_set_state(port, SNK_READY, 0);
> > > +                break;
> > > +            }
> > >           }
> > >           /*
> > >            * If VBUS has never been low, and we time out waiting
> > > @@ -4603,6 +4605,7 @@ static void run_state_machine(struct tcpm_port
> > > *port)
> > >       case SOFT_RESET_SEND:
> > >           port->message_id = 0;
> > >           port->rx_msgid = -1;
> > > +        port->tcpc->set_pd_rx(port->tcpc, true);
> > >           if (tcpm_pd_send_control(port, PD_CTRL_SOFT_RESET))
> > >               tcpm_set_state_cond(port, hard_reset_state(port), 0);
> > >           else
> >