| Message ID | 20231121180855.1278717-2-robert.hancock@calian.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Xilinx I2C driver fixes | expand |
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@calian.com> > --- > 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? 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 >
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@calian.com> > > --- > > 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 > >
> > > 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. yeah... I've been fishing patches that have been left behind :-) > 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... oh yes, sorry, because you are chekcking for: if (pend & (XIIC_INTR_TX_EMPTY_MASK | XIIC_INTR_TX_HALF_MASK)) { ... ... if (... && (pend & XIIC_INTR_TX_EMPTY_MASK)) { ... ... } ... ... } So that yes, it's possible. I misread the first 'if' :-) Then this is good to go, I'm taking both in. The second patch had some issues as patch formatting, but I will take care of them. Thanks, Andi
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)) { 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) {
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 | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-)