diff mbox series

[1/2] i2c: xiic: Wait for TX empty to avoid missed TX NAKs

Message ID 20231117181238.3989861-2-robert.hancock@calian.com
State Superseded
Headers show
Series Xilinx I2C driver fixes | expand

Commit Message

Robert Hancock Nov. 17, 2023, 6:14 p.m. UTC
Frequently an I2C write will be followed by a read, such as a register
address write followed by a read of the register value. In this driver,
when the TX FIFO half empty interrupt was raised and it was determined
that there was enough space in the TX FIFO to send the following read
command, it would do so without waiting for the TX FIFO to actually
empty.

Unfortunately it appears that in some cases this can result in a NAK
that was raised by the target device on the write, such as due to an
unsupported register address, being ignored and the subsequent read
being done anyway. This can potentially put the I2C bus into an
invalid state and/or result in invalid read data being processed.

To avoid this, once a message has been fully written to the TX FIFO,
wait for the TX FIFO empty interrupt before moving on to the next
message, to ensure NAKs are handled properly.

Fixes: e1d5b6598cdc ("i2c: Add support for Xilinx XPS IIC Bus Interface")
Signed-off-by: Robert Hancock <robert.hancock@calian.com>
---
 drivers/i2c/busses/i2c-xiic.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

Comments

Robert Hancock Nov. 20, 2023, 11:36 p.m. UTC | #1
On Fri, 2023-11-17 at 18:14 +0000, Robert Hancock wrote:
> Frequently an I2C write will be followed by a read, such as a
> register
> address write followed by a read of the register value. In this
> driver,
> when the TX FIFO half empty interrupt was raised and it was
> determined
> that there was enough space in the TX FIFO to send the following read
> command, it would do so without waiting for the TX FIFO to actually
> empty.
> 
> Unfortunately it appears that in some cases this can result in a NAK
> that was raised by the target device on the write, such as due to an
> unsupported register address, being ignored and the subsequent read
> being done anyway. This can potentially put the I2C bus into an
> invalid state and/or result in invalid read data being processed.
> 
> To avoid this, once a message has been fully written to the TX FIFO,
> wait for the TX FIFO empty interrupt before moving on to the next
> message, to ensure NAKs are handled properly.
> 
> Fixes: e1d5b6598cdc ("i2c: Add support for Xilinx XPS IIC Bus
> Interface")
> Signed-off-by: Robert Hancock <robert.hancock@calian.com>
> ---
>  drivers/i2c/busses/i2c-xiic.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-
> xiic.c
> index 71391b590ada..6e84b4d67fd9 100644
> --- a/drivers/i2c/busses/i2c-xiic.c
> +++ b/drivers/i2c/busses/i2c-xiic.c
> @@ -774,12 +774,15 @@ static irqreturn_t xiic_process(int irq, void
> *dev_id)
>  
>                 xiic_fill_tx_fifo(i2c);
>  
> -               /* current message sent and there is space in the
> fifo */
> -               if (!xiic_tx_space(i2c) && xiic_tx_fifo_space(i2c) >=
> 2) {
> +               /* current message fully written */
> +               if (!xiic_tx_space(i2c)) {
> 

Looks like there may be another small bug here - we should only proceed
with the next message if the FIFO was empty when the interrupt was
raised, not if we just finished writing it in the previous
xiic_fill_tx_fifo call. Another version coming.

>                         dev_dbg(i2c->adap.dev.parent,
>                                 "%s end of message sent, nmsgs:
> %d\n",
>                                 __func__, i2c->nmsgs);
> -                       if (i2c->nmsgs > 1) {
> +                       /* Don't move onto the next message until the
> TX FIFO empties,
> +                        * to ensure that a NAK is not missed.
> +                        */
> +                       if (i2c->nmsgs > 1 && (pend &
> XIIC_INTR_TX_EMPTY_MASK)) {
>                                 i2c->nmsgs--;
>                                 i2c->tx_msg++;
>                                 xfer_more = 1;
> @@ -790,11 +793,7 @@ static irqreturn_t xiic_process(int irq, void
> *dev_id)
>                                         "%s Got TX IRQ but no more to
> do...\n",
>                                         __func__);
>                         }
> -               } else if (!xiic_tx_space(i2c) && (i2c->nmsgs == 1))
> -                       /* current frame is sent and is last,
> -                        * make sure to disable tx half
> -                        */
> -                       xiic_irq_dis(i2c, XIIC_INTR_TX_HALF_MASK);
> +               }
>         }
>  
>         if (pend & XIIC_INTR_BNB_MASK) {
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index 71391b590ada..6e84b4d67fd9 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -774,12 +774,15 @@  static irqreturn_t xiic_process(int irq, void *dev_id)
 
 		xiic_fill_tx_fifo(i2c);
 
-		/* current message sent and there is space in the fifo */
-		if (!xiic_tx_space(i2c) && xiic_tx_fifo_space(i2c) >= 2) {
+		/* current message fully written */
+		if (!xiic_tx_space(i2c)) {
 			dev_dbg(i2c->adap.dev.parent,
 				"%s end of message sent, nmsgs: %d\n",
 				__func__, i2c->nmsgs);
-			if (i2c->nmsgs > 1) {
+			/* Don't move onto the next message until the TX FIFO empties,
+			 * to ensure that a NAK is not missed.
+			 */
+			if (i2c->nmsgs > 1 && (pend & XIIC_INTR_TX_EMPTY_MASK)) {
 				i2c->nmsgs--;
 				i2c->tx_msg++;
 				xfer_more = 1;
@@ -790,11 +793,7 @@  static irqreturn_t xiic_process(int irq, void *dev_id)
 					"%s Got TX IRQ but no more to do...\n",
 					__func__);
 			}
-		} else if (!xiic_tx_space(i2c) && (i2c->nmsgs == 1))
-			/* current frame is sent and is last,
-			 * make sure to disable tx half
-			 */
-			xiic_irq_dis(i2c, XIIC_INTR_TX_HALF_MASK);
+		}
 	}
 
 	if (pend & XIIC_INTR_BNB_MASK) {