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

[PATCH v3] 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 updates the message reading
code to check whether a GoodCRC message was received or not. Based
on this check it will either report that the previous transmission
has completed or it will pass the msg data to TCPM for futher
processing. This way the incorrect ordering of the events no longer
matters.

Signed-off-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
---

Changes in v3:
 - Always read from FIFO on TX_SUCCES and GCRCSENT events, but decision on how
   to report to TCPM is no longer based on these event types but instead on type
   of message read in from FIFO.

Changes in v2:
 - Remove erroneous extended header check

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

 drivers/usb/typec/fusb302/fusb302.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/typec/fusb302/fusb302.c b/drivers/usb/typec/fusb302/fusb302.c
index 72cb060..d200085 100644
--- a/drivers/usb/typec/fusb302/fusb302.c
+++ b/drivers/usb/typec/fusb302/fusb302.c
@@ -1543,6 +1543,21 @@ static int fusb302_pd_read_message(struct fusb302_chip *chip,
 	fusb302_log(chip, "PD message header: %x", msg->header);
 	fusb302_log(chip, "PD message len: %d", len);
 
+	/*
+	 * Check if we've read off a GoodCRC message. If so then indicate to
+	 * TCPM that the previous transmission has completed. Otherwise we pass
+	 * the received message over to TCPM for processing.
+	 *
+	 * We make this check here instead of basing the reporting decision on
+	 * the IRQ event type, as it's possible for the chip to report the
+	 * TX_SUCCESS and GCRCSENT events out of order on occasion, so we need
+	 * to check the message type to ensure correct reporting to TCPM.
+	 */
+	if ((!len) && (pd_header_type_le(msg->header) == PD_CTRL_GOOD_CRC))
+		tcpm_pd_transmit_complete(chip->tcpm_port, TCPC_TX_SUCCESS);
+	else
+		tcpm_pd_receive(chip->tcpm_port, msg);
+
 	return ret;
 }
 
@@ -1650,13 +1665,12 @@ 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);
+			fusb302_log(chip,
+				    "cannot read in PD message, ret=%d", ret);
 			goto done;
 		}
-		tcpm_pd_transmit_complete(chip->tcpm_port, TCPC_TX_SUCCESS);
 	}
 
 	if (interrupta & FUSB_REG_INTERRUPTA_HARDRESET) {
@@ -1677,7 +1691,6 @@ static irqreturn_t fusb302_irq_intn(int irq, void *dev_id)
 				    "cannot read in PD message, ret=%d", ret);
 			goto done;
 		}
-		tcpm_pd_receive(chip->tcpm_port, &pd_msg);
 	}
 done:
 	mutex_unlock(&chip->lock);
-- 
1.9.1

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

[Guenter Roeck]

On Tue, Nov 21, 2017 at 02:12:12PM +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 updates the message reading
> code to check whether a GoodCRC message was received or not. Based
> on this check it will either report that the previous transmission
> has completed or it will pass the msg data to TCPM for futher
> processing. This way the incorrect ordering of the events no longer
> matters.
> 
> Signed-off-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>

I am not really happy about the code duplication (two calls to
fusb302_pd_read_message() and identical error messages), but
everything else I came up with was even more messy, so

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
> 
> Changes in v3:
>  - Always read from FIFO on TX_SUCCES and GCRCSENT events, but decision on how
>    to report to TCPM is no longer based on these event types but instead on type
>    of message read in from FIFO.
> 
> Changes in v2:
>  - Remove erroneous extended header check
> 
> Patch is based on Linux next-20171114 to include move out of staging.
> 
>  drivers/usb/typec/fusb302/fusb302.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/typec/fusb302/fusb302.c b/drivers/usb/typec/fusb302/fusb302.c
> index 72cb060..d200085 100644
> --- a/drivers/usb/typec/fusb302/fusb302.c
> +++ b/drivers/usb/typec/fusb302/fusb302.c
> @@ -1543,6 +1543,21 @@ static int fusb302_pd_read_message(struct fusb302_chip *chip,
>  	fusb302_log(chip, "PD message header: %x", msg->header);
>  	fusb302_log(chip, "PD message len: %d", len);
>  
> +	/*
> +	 * Check if we've read off a GoodCRC message. If so then indicate to
> +	 * TCPM that the previous transmission has completed. Otherwise we pass
> +	 * the received message over to TCPM for processing.
> +	 *
> +	 * We make this check here instead of basing the reporting decision on
> +	 * the IRQ event type, as it's possible for the chip to report the
> +	 * TX_SUCCESS and GCRCSENT events out of order on occasion, so we need
> +	 * to check the message type to ensure correct reporting to TCPM.
> +	 */
> +	if ((!len) && (pd_header_type_le(msg->header) == PD_CTRL_GOOD_CRC))
> +		tcpm_pd_transmit_complete(chip->tcpm_port, TCPC_TX_SUCCESS);
> +	else
> +		tcpm_pd_receive(chip->tcpm_port, msg);
> +
>  	return ret;
>  }
>  
> @@ -1650,13 +1665,12 @@ 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);
> +			fusb302_log(chip,
> +				    "cannot read in PD message, ret=%d", ret);
>  			goto done;
>  		}
> -		tcpm_pd_transmit_complete(chip->tcpm_port, TCPC_TX_SUCCESS);
>  	}
>  
>  	if (interrupta & FUSB_REG_INTERRUPTA_HARDRESET) {
> @@ -1677,7 +1691,6 @@ static irqreturn_t fusb302_irq_intn(int irq, void *dev_id)
>  				    "cannot read in PD message, ret=%d", ret);
>  			goto done;
>  		}
> -		tcpm_pd_receive(chip->tcpm_port, &pd_msg);
>  	}
>  done:
>  	mutex_unlock(&chip->lock);
> -- 
> 1.9.1
> 

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

[Heikki Krogerus]

On Tue, Nov 21, 2017 at 02:12:12PM +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 updates the message reading
> code to check whether a GoodCRC message was received or not. Based
> on this check it will either report that the previous transmission
> has completed or it will pass the msg data to TCPM for futher
> processing. This way the incorrect ordering of the events no longer
> matters.
> 
> Signed-off-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>

FWIW:

Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
> 
> Changes in v3:
>  - Always read from FIFO on TX_SUCCES and GCRCSENT events, but decision on how
>    to report to TCPM is no longer based on these event types but instead on type
>    of message read in from FIFO.
> 
> Changes in v2:
>  - Remove erroneous extended header check
> 
> Patch is based on Linux next-20171114 to include move out of staging.
> 
>  drivers/usb/typec/fusb302/fusb302.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/typec/fusb302/fusb302.c b/drivers/usb/typec/fusb302/fusb302.c
> index 72cb060..d200085 100644
> --- a/drivers/usb/typec/fusb302/fusb302.c
> +++ b/drivers/usb/typec/fusb302/fusb302.c
> @@ -1543,6 +1543,21 @@ static int fusb302_pd_read_message(struct fusb302_chip *chip,
>  	fusb302_log(chip, "PD message header: %x", msg->header);
>  	fusb302_log(chip, "PD message len: %d", len);
>  
> +	/*
> +	 * Check if we've read off a GoodCRC message. If so then indicate to
> +	 * TCPM that the previous transmission has completed. Otherwise we pass
> +	 * the received message over to TCPM for processing.
> +	 *
> +	 * We make this check here instead of basing the reporting decision on
> +	 * the IRQ event type, as it's possible for the chip to report the
> +	 * TX_SUCCESS and GCRCSENT events out of order on occasion, so we need
> +	 * to check the message type to ensure correct reporting to TCPM.
> +	 */
> +	if ((!len) && (pd_header_type_le(msg->header) == PD_CTRL_GOOD_CRC))
> +		tcpm_pd_transmit_complete(chip->tcpm_port, TCPC_TX_SUCCESS);
> +	else
> +		tcpm_pd_receive(chip->tcpm_port, msg);
> +
>  	return ret;
>  }
>  
> @@ -1650,13 +1665,12 @@ 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);
> +			fusb302_log(chip,
> +				    "cannot read in PD message, ret=%d", ret);
>  			goto done;
>  		}
> -		tcpm_pd_transmit_complete(chip->tcpm_port, TCPC_TX_SUCCESS);
>  	}
>  
>  	if (interrupta & FUSB_REG_INTERRUPTA_HARDRESET) {
> @@ -1677,7 +1691,6 @@ static irqreturn_t fusb302_irq_intn(int irq, void *dev_id)
>  				    "cannot read in PD message, ret=%d", ret);
>  			goto done;
>  		}
> -		tcpm_pd_receive(chip->tcpm_port, &pd_msg);
>  	}
>  done:
>  	mutex_unlock(&chip->lock);

Thanks,

-- 
heikki