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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
