On Wed, 2024-09-11 at 14:40 +0200, Andi Shyti wrote:
> Hi Robert,
>
> For some reason this patch and the next was set in patchwork as
> "Changes requested" and did not appear in the list of things to
> review.
>
> On Tue, Nov 21, 2023 at 06:11:16PM GMT, 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 | 19 +++++++++----------
> > 1 file changed, 9 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-xiic.c
> > b/drivers/i2c/busses/i2c-xiic.c
> > index 71391b590ada..fd623e8ad08a 100644
> > --- a/drivers/i2c/busses/i2c-xiic.c
> > +++ b/drivers/i2c/busses/i2c-xiic.c
> > @@ -772,14 +772,17 @@ static irqreturn_t xiic_process(int irq, void
> > *dev_id)
> > goto out;
> > }
> >
> > - 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) {
> > + if (xiic_tx_space(i2c)) {
> > + xiic_fill_tx_fifo(i2c);
> > + } else {
> > + /* current message fully written */
> > 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)) {
>
> can "pend" be both XIIC_INTR_TX_EMPTY_MASK and
> XIIC_INTR_TX_HALF_MASK?
>
It's been a while since I looked at this, but I believe it potentially
could be. However, it seems like the behavior should still be correct -
if the TX FIFO is empty then it is also half empty, but really the fact
it is empty is what we care about in that situation...
> Andi
>
> > 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) {
> > --
> > 2.42.0
> >
