From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751415AbdKUS4k (ORCPT ); Tue, 21 Nov 2017 13:56:40 -0500 Received: from mail-pg0-f66.google.com ([74.125.83.66]:42102 "EHLO mail-pg0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751238AbdKUS4i (ORCPT ); Tue, 21 Nov 2017 13:56:38 -0500 X-Google-Smtp-Source: AGs4zMapswEmWNR54d57hKHD2Zvq/1x+G4gu5Gl77PB4ylj0te8rEYEM10JyKkhTtMYRl/v+oSWkwg== Date: Tue, 21 Nov 2017 10:56:35 -0800 From: Guenter Roeck To: Adam Thomson Cc: Heikki Krogerus , Greg Kroah-Hartman , Hans de Goede , Yueyao Zhu , Rui Miguel Silva , linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, support.opensource@diasemi.com Subject: Re: [PATCH v3] typec: tcpm: fusb302: Resolve out of order messaging events Message-ID: <20171121185635.GA8980@roeck-us.net> References: <20171121141212.481DE3FBAC@swsrvapps-01.diasemi.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171121141212.481DE3FBAC@swsrvapps-01.diasemi.com> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 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 > --- > > 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 >