On Wed, Jun 12, 2024 at 02:46:56PM +0200, Fabrice Gasnier wrote: > Sometimes errors are seen, when doing DR swap, like: > [ 24.672481] ucsi-stm32g0-i2c 0-0035: UCSI_GET_PDOS failed (-5) > [ 24.720188] ucsi-stm32g0-i2c 0-0035: ucsi_handle_connector_change: > GET_CONNECTOR_STATUS failed (-5) > > There may be some race, which lead to read CCI, before the command complete > flag is set, hence returning -EIO. Similar fix has been done also in > ucsi_acpi [1]. > > In case of a spurious or otherwise delayed notification it is > possible that CCI still reports the previous completion. The > UCSI spec is aware of this and provides two completion bits in > CCI, one for normal commands and one for acks. As acks and commands > alternate the notification handler can determine if the completion > bit is from the current command. > > To fix this add the ACK_PENDING bit for ucsi_stm32g0 and only complete > commands if the completion bit matches. > > [1] https://lore.kernel.org/lkml/20240121204123.275441-3-lk@xxxxxxx/ > > Fixes: 72849d4fcee7 ("usb: typec: ucsi: stm32g0: add support for stm32g0 controller") > Signed-off-by: Fabrice Gasnier <fabrice.gasnier@xxxxxxxxxxx> Reviewed-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx> > --- > Changes in v2: rebase and define ACK_PENDING as commented by Dmitry. > --- > drivers/usb/typec/ucsi/ucsi_stm32g0.c | 19 +++++++++++++++---- > 1 file changed, 15 insertions(+), 4 deletions(-) > > diff --git a/drivers/usb/typec/ucsi/ucsi_stm32g0.c b/drivers/usb/typec/ucsi/ucsi_stm32g0.c > index ac48b7763114..ac69288e8bb0 100644 > --- a/drivers/usb/typec/ucsi/ucsi_stm32g0.c > +++ b/drivers/usb/typec/ucsi/ucsi_stm32g0.c > @@ -65,6 +65,7 @@ struct ucsi_stm32g0 { > struct device *dev; > unsigned long flags; > #define COMMAND_PENDING 1 > +#define ACK_PENDING 2 > const char *fw_name; > struct ucsi *ucsi; > bool suspended; > @@ -396,9 +397,13 @@ static int ucsi_stm32g0_sync_write(struct ucsi *ucsi, unsigned int offset, const > size_t len) > { > struct ucsi_stm32g0 *g0 = ucsi_get_drvdata(ucsi); > + bool ack = UCSI_COMMAND(*(u64 *)val) == UCSI_ACK_CC_CI; > int ret; > > - set_bit(COMMAND_PENDING, &g0->flags); > + if (ack) > + set_bit(ACK_PENDING, &g0->flags); > + else > + set_bit(COMMAND_PENDING, &g0->flags); > > ret = ucsi_stm32g0_async_write(ucsi, offset, val, len); > if (ret) > @@ -406,9 +411,14 @@ static int ucsi_stm32g0_sync_write(struct ucsi *ucsi, unsigned int offset, const > > if (!wait_for_completion_timeout(&g0->complete, msecs_to_jiffies(5000))) > ret = -ETIMEDOUT; > + else > + return 0; > > out_clear_bit: > - clear_bit(COMMAND_PENDING, &g0->flags); > + if (ack) > + clear_bit(ACK_PENDING, &g0->flags); > + else > + clear_bit(COMMAND_PENDING, &g0->flags); > > return ret; > } > @@ -429,8 +439,9 @@ static irqreturn_t ucsi_stm32g0_irq_handler(int irq, void *data) > if (UCSI_CCI_CONNECTOR(cci)) > ucsi_connector_change(g0->ucsi, UCSI_CCI_CONNECTOR(cci)); > > - if (test_bit(COMMAND_PENDING, &g0->flags) && > - cci & (UCSI_CCI_ACK_COMPLETE | UCSI_CCI_COMMAND_COMPLETE)) > + if (cci & UCSI_CCI_ACK_COMPLETE && test_and_clear_bit(ACK_PENDING, &g0->flags)) > + complete(&g0->complete); > + if (cci & UCSI_CCI_COMMAND_COMPLETE && test_and_clear_bit(COMMAND_PENDING, &g0->flags)) > complete(&g0->complete); > > return IRQ_HANDLED; > -- > 2.25.1 -- heikki
