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

["Abdurrahman Hussain" <abdurrahman@nexthop.ai>]

On Thu Oct 8, 2026 at 6:01 PM PDT, Abdurrahman Hussain wrote:
> 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@nexthop.ai>
> Cc: stable@vger.kernel.org # 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@nexthop.ai/
>
> 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@nexthop.ai
>
> To: Michal Simek <michal.simek@amd.com>
> To: Andi Shyti <andi.shyti@kernel.org>
> To: Abdurrahman Hussain <abdurrahman@nexthop.ai>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-i2c@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> 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@nexthop.ai>

Hi Andi,

Gentle ping on this one. It missed 7.3, so v7.3 and v7.3.1 now ship
e6fe3ea04f01, and any pmbus device on xiic that reads a block (the
FS1412 above, for one) fails to probe. Could you take the revert
through i2c-fixes for 7.4-rc?

When you apply it, the stable hint should be

Cc: stable@vger.kernel.org # v7.3

rather than "# v6.3+". Sasha dropped e6fe3ea04f01 from the older stable
queues, so 7.3.y is the only stable branch that has it.

The patch still applies cleanly on current mainline. I can resend
against -rc1 if that's easier for you.

Thanks,
Abdurrahman