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

[Adam Thomson]

On 21 November 2017 14:08, Adam Thomson wrote:

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

Sorry. Finger trouble. Will resend with correct 'v3' title

> 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 
> ---
> 
> 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, _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, _msg);
>   }
>  done:
>   mutex_unlock(>lock);
> --
> 1.9.1

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

[Adam Thomson]

On 21 November 2017 14:08, Adam Thomson wrote:

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

Sorry. Finger trouble. Will resend with correct 'v3' title

> 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 
> ---
> 
> 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, _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, _msg);
>   }
>  done:
>   mutex_unlock(>lock);
> --
> 1.9.1