Re: [PATCH v2 1/2] i2c: at91: fix write transfers by clearing pending interrupt first

[Wolfram Sang]

On Wed, Oct 21, 2015 at 03:44:03PM +0200, Ludovic Desroches wrote:
> From: Cyrille Pitchen 
> 
> In some cases a NACK interrupt may be pending in the Status Register (SR)
> as a result of a previous transfer. However at91_do_twi_transfer() did not
> read the SR to clear pending interruptions before starting a new transfer.
> Hence a NACK interrupt rose as soon as it was enabled again at the I2C
> controller level, resulting in a wrong sequence of operations and strange
> patterns of behaviour on the I2C bus, such as a clock stretch followed by
> a restart of the transfer.
> 
> This first issue occurred with both DMA and PIO write transfers.
> 
> Also when a NACK error was detected during a PIO write transfer, the
> interrupt handler used to wrongly start a new transfer by writing into the
> Transmit Holding Register (THR). Then the I2C slave was likely to reply
> with a second NACK.
> 
> This second issue is fixed in atmel_twi_interrupt() by handling the TXRDY
> status bit only if both the TXCOMP and NACK status bits are cleared.
> 
> Tested with a at24 eeprom on sama5d36ek board running a linux-4.1-at91
> kernel image. Adapted to linux-next.
> 
> Signed-off-by: Cyrille Pitchen 
> Fixes: 93563a6a71bb ("i2c: at91: fix a race condition when using the DMA 
> controller")
> Reported-by: Peter Rosin 
> Signed-off-by: Ludovic Desroches 
> Tested-by: Peter Rosin 
> Cc: sta...@vger.kernel.org #4.1

Applied to for-next, thanks!

I considered for-current, but really want this to sit a little in
for-next to make sure there are no regressions. It will go back via
stable.



signature.asc
Description: Digital signature

[PATCH v2 1/2] i2c: at91: fix write transfers by clearing pending interrupt first

[Ludovic Desroches]

From: Cyrille Pitchen 

In some cases a NACK interrupt may be pending in the Status Register (SR)
as a result of a previous transfer. However at91_do_twi_transfer() did not
read the SR to clear pending interruptions before starting a new transfer.
Hence a NACK interrupt rose as soon as it was enabled again at the I2C
controller level, resulting in a wrong sequence of operations and strange
patterns of behaviour on the I2C bus, such as a clock stretch followed by
a restart of the transfer.

This first issue occurred with both DMA and PIO write transfers.

Also when a NACK error was detected during a PIO write transfer, the
interrupt handler used to wrongly start a new transfer by writing into the
Transmit Holding Register (THR). Then the I2C slave was likely to reply
with a second NACK.

This second issue is fixed in atmel_twi_interrupt() by handling the TXRDY
status bit only if both the TXCOMP and NACK status bits are cleared.

Tested with a at24 eeprom on sama5d36ek board running a linux-4.1-at91
kernel image. Adapted to linux-next.

Signed-off-by: Cyrille Pitchen 
Fixes: 93563a6a71bb ("i2c: at91: fix a race condition when using the DMA 
controller")
Reported-by: Peter Rosin 
Signed-off-by: Ludovic Desroches 
Tested-by: Peter Rosin 
Cc: sta...@vger.kernel.org #4.1
---
 drivers/i2c/busses/i2c-at91.c | 58 +--
 1 file changed, 50 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
index 1c758cd..94c087b 100644
--- a/drivers/i2c/busses/i2c-at91.c
+++ b/drivers/i2c/busses/i2c-at91.c
@@ -465,19 +465,57 @@ static irqreturn_t atmel_twi_interrupt(int irq, void 
*dev_id)
 
if (!irqstatus)
return IRQ_NONE;
-   else if (irqstatus & AT91_TWI_RXRDY)
-   at91_twi_read_next_byte(dev);
-   else if (irqstatus & AT91_TWI_TXRDY)
-   at91_twi_write_next_byte(dev);
-
-   /* catch error flags */
-   dev->transfer_status |= status;
 
+   /*
+* When a NACK condition is detected, the I2C controller sets the NACK,
+* TXCOMP and TXRDY bits all together in the Status Register (SR).
+*
+* 1 - Handling NACK errors with CPU write transfer.
+*
+* In such case, we should not write the next byte into the Transmit
+* Holding Register (THR) otherwise the I2C controller would start a new
+* transfer and the I2C slave is likely to reply by another NACK.
+*
+* 2 - Handling NACK errors with DMA write transfer.
+*
+* By setting the TXRDY bit in the SR, the I2C controller also triggers
+* the DMA controller to write the next data into the THR. Then the
+* result depends on the hardware version of the I2C controller.
+*
+* 2a - Without support of the Alternative Command mode.
+*
+* This is the worst case: the DMA controller is triggered to write the
+* next data into the THR, hence starting a new transfer: the I2C slave
+* is likely to reply by another NACK.
+* Concurrently, this interrupt handler is likely to be called to manage
+* the first NACK before the I2C controller detects the second NACK and
+* sets once again the NACK bit into the SR.
+* When handling the first NACK, this interrupt handler disables the I2C
+* controller interruptions, especially the NACK interrupt.
+* Hence, the NACK bit is pending into the SR. This is why we should
+* read the SR to clear all pending interrupts at the beginning of
+* at91_do_twi_transfer() before actually starting a new transfer.
+*
+* 2b - With support of the Alternative Command mode.
+*
+* When a NACK condition is detected, the I2C controller also locks the
+* THR (and sets the LOCK bit in the SR): even though the DMA controller
+* is triggered by the TXRDY bit to write the next data into the THR,
+* this data actually won't go on the I2C bus hence a second NACK is not
+* generated.
+*/
if (irqstatus & (AT91_TWI_TXCOMP | AT91_TWI_NACK)) {
at91_disable_twi_interrupts(dev);
complete(>cmd_complete);
+   } else if (irqstatus & AT91_TWI_RXRDY) {
+   at91_twi_read_next_byte(dev);
+   } else if (irqstatus & AT91_TWI_TXRDY) {
+   at91_twi_write_next_byte(dev);
}
 
+   /* catch error flags */
+   dev->transfer_status |= status;
+
return IRQ_HANDLED;
 }
 
@@ -487,6 +525,7 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev)
unsigned long time_left;
bool has_unre_flag = dev->pdata->has_unre_flag;
bool has_alt_cmd = dev->pdata->has_alt_cmd;
+   unsigned sr;
 
/*
 * WARNING: the TXCOMP bit in the