[PATCH v2] Revert "i2c: xiic: defer RX_FULL until all trailing bytes are in FIFO"

[Abdurrahman Hussain]

This reverts commit e6fe3ea04f0113fe4d47c03d76e03805a4768ca7.

That commit raised the Rx FIFO threshold for an SMBus block read whose
trailing bytes fit in the FIFO, on the grounds that firing RX_FULL with
the last byte still in flight made xiic_read_rx() NACK, and so truncate,
that byte. NACKing the last byte does not truncate it: it is how a
master ends a read. The byte is still clocked in and is drained on the
bytes_rem == 0 path that follows, which also issues the STOP.

What the higher threshold does instead is skip the bytes_rem == 1
branch, the only place that sets NO_ACK, so every trailing byte is now
ACKed. Having had its last byte ACKed, the target goes on to drive the
next one, and the STOP that follows lands in the middle of it.

Some targets ride this out; others are left wedged until the next
transactions clock them free. A TDK FS1412 fails the two transfers that
follow any block read, so it never probes: pmbus reads MFR_ID as a block
and then VOUT_MODE as a byte. A Renesas RAA228246 and one RAA228244 on
the same controller fail the plain block read itself and the byte read
after it.

With commit b7e6df2f52ed ("i2c: xiic: preserve PEC byte length in SMBus
block read setup") and commit 840d8acc8792 ("i2c: xiic: don't clobber
msg->len to signal block-read completion") in place, PEC block reads
pass at the original threshold. The PEC failures the reverted commit
attributed to the threshold came from the bugs those two fix; all three
were developed together against an ADM1266, which rides out the extra
ACK, so the regression did not show.

Twenty rounds per device of a block read and a PEC block read of MFR_ID,
each followed by a byte read:

               before                      after
            block  PEC   byte after     block  PEC   byte after
  FS1412    20/20  0/20  20/40 fail     20/20 20/20   0/40 fail
  RAA228246  0/20 20/20  20/40 fail     20/20 20/20   0/40 fail
  RAA228244  0/20 20/20  20/40 fail     20/20 20/20   0/40 fail

A second RAA228244 and an Infineon XDPE1A2G5B passed every round before
and after.

Fixes: e6fe3ea04f01 ("i2c: xiic: defer RX_FULL until all trailing bytes are in FIFO")
Signed-off-by: Abdurrahman Hussain <abdurrahman@xxxxxxxxxx>
Cc: stable@xxxxxxxxxxxxxxx # v6.3+
---
A follow-up to my xiic block-read series, now in 7.3-rc as
b7e6df2f52ed, e6fe3ea04f01 and 840d8acc8792. The second of those
stopped NACKing the final byte of a block read that fits in the Rx
FIFO, which leaves some PMBus targets wedged for the next couple of
transfers.

Tested on a 6.12 kernel carrying that series, against an AXI IIC
controller with an FS1412, RAA228244/RAA228246 and XDPE1A2G5B behind
it, using plain and PEC block reads and the byte reads that follow.

The same happens on a second board, where two of four ISL68225s behind
the same AXI IIC core fail plain block reads and the transfer
after them. A capture of SCL/SDA taken inside the FPGA shows the
mechanism: after the last byte of an MFR_ID block read the master
drives ACK, the target starts on the next byte with SDA low, and the
STOP the master then attempts never appears, leaving SDA held low. With
this change the last byte is NACKed and the STOP is clean, and all four
pass.

PEC block reads do not regress: 1000 PEC block reads of MFR_ID per
device, each followed by a byte read, on the FS1412, both RAA parts, the
XDPE1A2G5B and a further RAA228244 passed without a failure, both with
this revert alone and with Siddarth's patch on top of it.

With this change a block read ends with a NACK, and so with TX_ERROR,
like any other read. That puts it in the window Siddarth's patch closes,
where xiic_process() sees TX_ERROR before RX_FULL and resets the core
even though the data is already in the Rx FIFO:

  https://lore.kernel.org/linux-i2c/20261005-i2c-xiic-rx-fifo-v1-1-0177ac5f72d7@xxxxxxxxxx/

The two touch different functions and apply cleanly to v7.3-rc6 in
either order.
---
Changes in v2:
- Revert e6fe3ea04f01 outright instead of moving the threshold back by
  truncated), and with b7e6df2f52ed and 840d8acc8792 in place PEC block
  reads pass at the original threshold. The code change is the same
  single line as v1 minus its tail == 1 branch, which could never run
  (Siddarth, Sashiko): reads with fewer than two trailing bytes are
  padded to SMBUS_BLOCK_READ_MIN_LEN first.
  Sashiko review: https://sashiko.dev/#/patchset/20261008-i2c-xiic-block-read-nack-v1-1-94882af6e598%40nexthop.ai
- Link to v1: https://patch.msgid.link/20261008-i2c-xiic-block-read-nack-v1-1-94882af6e598@xxxxxxxxxx

To: Michal Simek <michal.simek@xxxxxxx>
To: Andi Shyti <andi.shyti@xxxxxxxxxx>
To: Abdurrahman Hussain <abdurrahman@xxxxxxxxxx>
Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
Cc: linux-i2c@xxxxxxxxxxxxxxx
Cc: linux-kernel@xxxxxxxxxxxxxxx
hand: that commit's premise was wrong (a NACKed last byte is not
---
 drivers/i2c/busses/i2c-xiic.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index 5e397a7e63f6..76ef9c26ed57 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -569,11 +569,11 @@ static void xiic_smbus_block_read_setup(struct xiic_i2c *i2c)
 			i2c->smbus_actual_len = 1 + rxmsg_len + pec_len;
 		} else {
 			/*
-			 * All trailing bytes fit in the Rx FIFO. Defer RX_FULL
-			 * until every one of them is buffered, so the drain
-			 * takes xiic_read_rx()'s bytes_rem == 0 path.
+			 * All trailing bytes fit in the Rx FIFO. The widened
+			 * condition above guarantees rxmsg_len + pec_len >= 2,
+			 * so this cannot underflow.
 			 */
-			rfd_set = rxmsg_len + pec_len - 1;
+			rfd_set = rxmsg_len + pec_len - 2;
 			i2c->rx_msg->len = rxmsg_len + 1 + pec_len;
 		}
 		xiic_setreg8(i2c, XIIC_RFD_REG_OFFSET, rfd_set);

---
base-commit: a90ee4305c4a5df72c11b31dacfdc76e00fcf78a
change-id: 20261007-i2c-xiic-block-read-nack-d2b735baddd3

Best regards,
--  
Abdurrahman Hussain <abdurrahman@xxxxxxxxxx>