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

[Robert Hancock]

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 at 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) {

-- 
Robert Hancock <robert.hancock at calian.com>