[PATCH v2] typec: tcpm: fusb302: Resolve out of order messaging events

[PATCH v2] typec: tcpm: fusb302: Resolve out of order messaging events

[Adam Thomson]

The expectation in the FUSB302 driver is that a TX_SUCCESS event
should occur after a message has been sent, but before a GCRCSENT
event is raised to indicate successful receipt of a message from
the partner. However in some circumstances it is possible to see
the hardware raise a GCRCSENT event before a TX_SUCCESS event
is raised. The upshot of this is that the GCRCSENT handling portion
of code ends up reporting the GoodCRC message to TCPM because the
TX_SUCCESS event hasn't yet arrived to trigger a consumption of it.
When TX_SUCCESS is then raised by the chip it ends up consuming the
actual message that was meant for TCPM, and this incorrect sequence
results in a hard reset from TCPM.

To avoid this problem, this commit moves all FIFO reading to be
done based on a GCRCSENT event, and when reading from the FIFO
any GoodCRC messages read in are discarded so only valid messages
are reported to TCPM.

Changes in v2:
 - Remove erroneous extended header check

Patch is based on Linux next-20171114 to include move out of staging.

Signed-off-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
---
 drivers/usb/typec/fusb302/fusb302.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/typec/fusb302/fusb302.c b/drivers/usb/typec/fusb302/fusb302.c
index 72cb060..ddf88f0 100644
--- a/drivers/usb/typec/fusb302/fusb302.c
+++ b/drivers/usb/typec/fusb302/fusb302.c
@@ -1650,12 +1650,6 @@ static irqreturn_t fusb302_irq_intn(int irq, void *dev_id)
 
 	if (interrupta & FUSB_REG_INTERRUPTA_TX_SUCCESS) {
 		fusb302_log(chip, "IRQ: PD tx success");
-		/* read out the received good CRC */
-		ret = fusb302_pd_read_message(chip, &pd_msg);
-		if (ret < 0) {
-			fusb302_log(chip, "cannot read in GCRC, ret=%d", ret);
-			goto done;
-		}
 		tcpm_pd_transmit_complete(chip->tcpm_port, TCPC_TX_SUCCESS);
 	}
 
@@ -1671,12 +1665,22 @@ static irqreturn_t fusb302_irq_intn(int irq, void *dev_id)
 
 	if (interruptb & FUSB_REG_INTERRUPTB_GCRCSENT) {
 		fusb302_log(chip, "IRQ: PD sent good CRC");
+retry:
 		ret = fusb302_pd_read_message(chip, &pd_msg);
 		if (ret < 0) {
 			fusb302_log(chip,
 				    "cannot read in PD message, ret=%d", ret);
 			goto done;
 		}
+
+		/*
+		 * Check to make sure we've not read off a GoodCRC message.
+		 * If so then read again to retrieve expected message
+		 */
+		if ((!pd_header_cnt_le(pd_msg.header)) &&
+		    (pd_header_type_le(pd_msg.header) == PD_CTRL_GOOD_CRC))
+			goto retry;
+
 		tcpm_pd_receive(chip->tcpm_port, &pd_msg);
 	}
 done:
-- 
1.9.1

Re: [PATCH v2] typec: tcpm: fusb302: Resolve out of order messaging events

[Guenter Roeck]

On Thu, Nov 16, 2017 at 04:28:11PM +0000, Adam Thomson wrote:
> The expectation in the FUSB302 driver is that a TX_SUCCESS event
> should occur after a message has been sent, but before a GCRCSENT
> event is raised to indicate successful receipt of a message from
> the partner. However in some circumstances it is possible to see
> the hardware raise a GCRCSENT event before a TX_SUCCESS event
> is raised. The upshot of this is that the GCRCSENT handling portion
> of code ends up reporting the GoodCRC message to TCPM because the
> TX_SUCCESS event hasn't yet arrived to trigger a consumption of it.
> When TX_SUCCESS is then raised by the chip it ends up consuming the
> actual message that was meant for TCPM, and this incorrect sequence
> results in a hard reset from TCPM.
> 
> To avoid this problem, this commit moves all FIFO reading to be
> done based on a GCRCSENT event, and when reading from the FIFO
> any GoodCRC messages read in are discarded so only valid messages
> are reported to TCPM.
> 
> Changes in v2:
>  - Remove erroneous extended header check
> 
> Patch is based on Linux next-20171114 to include move out of staging.
> 
> Signed-off-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
> ---
>  drivers/usb/typec/fusb302/fusb302.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/typec/fusb302/fusb302.c b/drivers/usb/typec/fusb302/fusb302.c
> index 72cb060..ddf88f0 100644
> --- a/drivers/usb/typec/fusb302/fusb302.c
> +++ b/drivers/usb/typec/fusb302/fusb302.c
> @@ -1650,12 +1650,6 @@ static irqreturn_t fusb302_irq_intn(int irq, void *dev_id)
>  
>  	if (interrupta & FUSB_REG_INTERRUPTA_TX_SUCCESS) {
>  		fusb302_log(chip, "IRQ: PD tx success");
> -		/* read out the received good CRC */
> -		ret = fusb302_pd_read_message(chip, &pd_msg);
> -		if (ret < 0) {
> -			fusb302_log(chip, "cannot read in GCRC, ret=%d", ret);
> -			goto done;
> -		}

If multiple "Good CRC" messages are received in a row, they won't be read from
the chip, which might result in a buffer overflow.

It might be better to always read all pending messages and handle it depending
on the message type. Something along the line of

	while (interrupts & (FUSB_REG_INTERRUPTA_TX_SUCCESS |
			     FUSB_REG_INTERRUPTB_GCRCSENT)) {
		ret = fusb302_pd_read_message(chip, &pd_msg);
		if (ret < 0)
			goto done;
		if (msg_type == good CRC) {
			tcpm_pd_transmit_complete(chip->tcpm_port, TCPC_TX_SUCCESS);
			interrupts &= ~FUSB_REG_INTERRUPTA_TX_SUCCESS;
		} else {
			tcpm_pd_receive(chip->tcpm_port, &pd_msg);
			interrupts &= ~FUSB_REG_INTERRUPTB_GCRCSENT;
		}
	}

Guenter

>  		tcpm_pd_transmit_complete(chip->tcpm_port, TCPC_TX_SUCCESS);
>  	}
>  
> @@ -1671,12 +1665,22 @@ static irqreturn_t fusb302_irq_intn(int irq, void *dev_id)
>  
>  	if (interruptb & FUSB_REG_INTERRUPTB_GCRCSENT) {
>  		fusb302_log(chip, "IRQ: PD sent good CRC");
> +retry:
>  		ret = fusb302_pd_read_message(chip, &pd_msg);
>  		if (ret < 0) {
>  			fusb302_log(chip,
>  				    "cannot read in PD message, ret=%d", ret);
>  			goto done;
>  		}
> +
> +		/*
> +		 * Check to make sure we've not read off a GoodCRC message.
> +		 * If so then read again to retrieve expected message
> +		 */
> +		if ((!pd_header_cnt_le(pd_msg.header)) &&
> +		    (pd_header_type_le(pd_msg.header) == PD_CTRL_GOOD_CRC))
> +			goto retry;
> +
>  		tcpm_pd_receive(chip->tcpm_port, &pd_msg);
>  	}
>  done:
> -- 
> 1.9.1
> 

RE: [PATCH v2] typec: tcpm: fusb302: Resolve out of order messaging events

[Adam Thomson]

On 17 November 2017 17:20, Guenter Roeck wrote:

> On Thu, Nov 16, 2017 at 04:28:11PM +0000, Adam Thomson wrote:
> > The expectation in the FUSB302 driver is that a TX_SUCCESS event
> > should occur after a message has been sent, but before a GCRCSENT
> > event is raised to indicate successful receipt of a message from
> > the partner. However in some circumstances it is possible to see
> > the hardware raise a GCRCSENT event before a TX_SUCCESS event
> > is raised. The upshot of this is that the GCRCSENT handling portion
> > of code ends up reporting the GoodCRC message to TCPM because the
> > TX_SUCCESS event hasn't yet arrived to trigger a consumption of it.
> > When TX_SUCCESS is then raised by the chip it ends up consuming the
> > actual message that was meant for TCPM, and this incorrect sequence
> > results in a hard reset from TCPM.
> >
> > To avoid this problem, this commit moves all FIFO reading to be
> > done based on a GCRCSENT event, and when reading from the FIFO
> > any GoodCRC messages read in are discarded so only valid messages
> > are reported to TCPM.
> >
> > Changes in v2:
> >  - Remove erroneous extended header check
> >
> > Patch is based on Linux next-20171114 to include move out of staging.
> >
> > Signed-off-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
> > ---
> >  drivers/usb/typec/fusb302/fusb302.c | 16 ++++++++++------
> >  1 file changed, 10 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/usb/typec/fusb302/fusb302.c
> b/drivers/usb/typec/fusb302/fusb302.c
> > index 72cb060..ddf88f0 100644
> > --- a/drivers/usb/typec/fusb302/fusb302.c
> > +++ b/drivers/usb/typec/fusb302/fusb302.c
> > @@ -1650,12 +1650,6 @@ static irqreturn_t fusb302_irq_intn(int irq, void
> *dev_id)
> >
> >  	if (interrupta & FUSB_REG_INTERRUPTA_TX_SUCCESS) {
> >  		fusb302_log(chip, "IRQ: PD tx success");
> > -		/* read out the received good CRC */
> > -		ret = fusb302_pd_read_message(chip, &pd_msg);
> > -		if (ret < 0) {
> > -			fusb302_log(chip, "cannot read in GCRC, ret=%d", ret);
> > -			goto done;
> > -		}
> 
> If multiple "Good CRC" messages are received in a row, they won't be read from
> the chip, which might result in a buffer overflow.

Thanks for your comments. I had considered this since sending the patch. In 
reality I think we'd need to send 11 consecutive messages to which we receive
11 GoodCRCs without receiving a real response message from the partner to clear
the FIFO. Having looked again at the PD spec that seemed really unlikely, but...

> It might be better to always read all pending messages and handle it depending
> on the message type. Something along the line of
>
> 	while (interrupts & (FUSB_REG_INTERRUPTA_TX_SUCCESS |
> 			     FUSB_REG_INTERRUPTB_GCRCSENT)) {
> 		ret = fusb302_pd_read_message(chip, &pd_msg);
> 		if (ret < 0)
> 			goto done;
> 		if (msg_type == good CRC) {
> 			tcpm_pd_transmit_complete(chip->tcpm_port,
> TCPC_TX_SUCCESS);
> 			interrupts &= ~FUSB_REG_INTERRUPTA_TX_SUCCESS;
> 		} else {
> 			tcpm_pd_receive(chip->tcpm_port, &pd_msg);
> 			interrupts &= ~FUSB_REG_INTERRUPTB_GCRCSENT;
> 		}
> 	}
> 
> Guenter

...I do prefer your approach as it doesn't leave anything to chance. I'll
respin accordingly.

> 
> >  		tcpm_pd_transmit_complete(chip->tcpm_port, TCPC_TX_SUCCESS);
> >  	}
> >
> > @@ -1671,12 +1665,22 @@ static irqreturn_t fusb302_irq_intn(int irq, void
> *dev_id)
> >
> >  	if (interruptb & FUSB_REG_INTERRUPTB_GCRCSENT) {
> >  		fusb302_log(chip, "IRQ: PD sent good CRC");
> > +retry:
> >  		ret = fusb302_pd_read_message(chip, &pd_msg);
> >  		if (ret < 0) {
> >  			fusb302_log(chip,
> >  				    "cannot read in PD message, ret=%d", ret);
> >  			goto done;
> >  		}
> > +
> > +		/*
> > +		 * Check to make sure we've not read off a GoodCRC message.
> > +		 * If so then read again to retrieve expected message
> > +		 */
> > +		if ((!pd_header_cnt_le(pd_msg.header)) &&
> > +		    (pd_header_type_le(pd_msg.header) == PD_CTRL_GOOD_CRC))
> > +			goto retry;
> > +
> >  		tcpm_pd_receive(chip->tcpm_port, &pd_msg);
> >  	}
> >  done:
> > --
> > 1.9.1
> >