Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v6 0/3] i2c: xiic: fix SMBus block read and PEC support
@ 2026-09-24  1:45 Abdurrahman Hussain
  2026-09-24  1:45 ` [PATCH v6 1/3] i2c: xiic: preserve PEC byte length in SMBus block read setup Abdurrahman Hussain
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Abdurrahman Hussain @ 2026-09-24  1:45 UTC (permalink / raw)
  To: Michal Simek, Andi Shyti, Wolfram Sang, Raviteja Narayanam,
	Wolfram Sang, Manikanta Guntupalli
  Cc: Shubhrajyoti Datta, linux-arm-kernel, linux-i2c, linux-kernel,
	Abdurrahman Hussain, stable

This series fixes three independent bugs in the Xilinx AXI IIC driver
that together make SMBus block reads with PEC return -EBADMSG or -EIO
on otherwise clean transfers. They only surface when the client has
I2C_CLIENT_PEC set; non-PEC block reads happen to mask each issue in
turn.

The problems were uncovered driving an adm1266 PMBus device behind a
Xilinx AXI IIC FPGA block and reading its 64-byte blackbox record.

Patch 1 stops xiic_smbus_block_read_setup() from truncating rx_msg->len.
The i2c core appends a byte to msg->len when PEC is enabled, so
overwriting the length to "block size + 1" silently drops the PEC byte
and i2c_smbus_check_pec() then reads the last payload byte as the PEC.

Patch 2 raises the RX_FULL threshold so the interrupt only fires once
every remaining byte (payload plus optional PEC) is already buffered in
the FIFO. The previous threshold of rxmsg_len - 2 caused the
bytes_rem == 1 path in xiic_read_rx() to NACK a byte still on the wire.
The chunk-vs-defer guard now also accounts for the PEC byte so a
rxmsg_len == IIC_RX_FIFO_DEPTH PEC-enabled read does not push
XIIC_RFD_REG_OFFSET past its 4-bit range.

Patch 3 stops the BNB handler from forcing tx_msg->len = 1 to signal
completion. tx_msg and rx_msg alias the same i2c_msg during a receive,
so this also clobbered rx_msg->len; and because tx_pos is already at 2
in the PEC case, the unsigned subtraction in xiic_tx_space() underflowed
and the STATE_DONE check fell through to STATE_ERROR. Advancing tx_pos
up to msg->len drives tx_space to zero without touching the length.

All three patches are pure bug fixes; non-PEC behaviour is unchanged.
Tested on real hardware -- a Xilinx AXI IIC controller talking to an
adm1266, where 64-byte PEC-checked block reads now complete cleanly.

Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
---
Changes in v6:
- Patch 1: also trim rx_msg->len in xiic_recv_atomic() (Andi). An
  I2C_M_RECV_LEN message forces standard mode for the whole transfer,
  so an atomic SMBus block read reaches the same padded branch but
  completed without the trim. The trim moved into a small helper,
  xiic_smbus_trim_len(), now called from both sites that clear rx_msg.
- Patches 2 and 3 unchanged.
- Link to v5: https://patch.msgid.link/20260921-i2c-xiic-v5-0-2fca81e810ea@nexthop.ai

Changes in v5:
- Patch 1: clear smbus_actual_len in xiic_start_recv() so the trim state
  is reset before every receive (Andi). Only the padded branch sets it,
  and a block read aborted by arbitration loss or a TX error returns
  through the error path without reaching the completion site, so a
  stale value could trim the length of an unrelated later read.
- Patch 3: drop the defensive smbus_actual_len reset in the BNB handler,
  now redundant with the reset in patch 1, and the paragraph describing
  it. No other change.
- Patch 2 unchanged.
- All three patches now carry a Fixes: tag and Cc: stable. All three
  bugs were introduced together by e4c1ff772e1a ("i2c: xiic: Add
  smbus_block_read functionality"), first released in v6.3, and a PEC
  block read needs all three to complete, so they should be backported
  as a set.
- Shubhrajyoti Datta's Reviewed-by from v1 is still not carried over.
  Shubhrajyoti, if the current code looks good to you, a fresh tag would
  be appreciated.
- Link to v4: https://patch.msgid.link/20260909-i2c-xiic-v4-0-218df31e9d3b@nexthop.ai

Changes in v4:
- No code changes; resend of v3 to collect Michal Simek's Acked-by
  (given on the v3 cover letter) into each patch.
- Shubhrajyoti Datta's Reviewed-by from v1 is not carried over since all
  three patches have changed since then. Shubhrajyoti, if the current
  code still looks good to you, a fresh tag would be appreciated.
- Link to v3: https://patch.msgid.link/20260513-i2c-xiic-v3-0-ccb3cf70ba03@nexthop.ai

Changes in v3 (addresses the sashiko automated review of v2):
- Patch 1: handle short SMBus block reads where the controller pads
  rx_msg->len up to SMBUS_BLOCK_READ_MIN_LEN for its end-of-message
  workaround. In v2 this branch left the PEC byte at the padded
  offset rather than the actual end-of-payload, so the i2c core's
  PEC validator read past the chip data. Track the on-wire length
  in a new smbus_actual_len field populated in the minlen branch
  of xiic_smbus_block_read_setup(), and trim rx_msg->len back at
  RX_FULL completion before passing the message up. Addresses
  sashiko's v2 note about the pec_len adjustment missing the
  rxmsg_len < 3 padding branch; that branch was indeed the cause
  of pmbus_check_block_register() silently failing on zero-length
  MFR_* fields and skipping debugfs auto-discovery on affected
  hardware.
- Patch 3: defensively reset smbus_actual_len in the BNB completion
  handler so a subsequent non-SMBus transfer cannot see a stale
  trim value from a completed short block read.
- Patch 2 is unchanged from v2. sashiko's two other v2 notes were
  investigated and judged not to require code changes: the concern
  about removed padding in the chunked-vs-deferred drain misread
  the patch (the padding survives via the else branch and the new
  PEC-aware guard preserves the original semantics), and the
  flagged unsigned underflow in xiic_tx_space() is unreachable
  because tx_pos is bounded by tx_msg->len at the call site.
- Link to v2: https://patch.msgid.link/20260511-i2c-xiic-v2-0-c16380cb1594@nexthop.ai

Changes in v2:
- Patch 2: widen the chunk-vs-defer guard in xiic_smbus_block_read_setup()
  to include pec_len, so a 16-byte PEC-enabled block read routes through
  the chunked drain rather than writing 16 into the 4-bit
  XIIC_RFD_REG_OFFSET register. No tree-level change to patches 1 or 3.
- Link to v1: https://patch.msgid.link/20260427-i2c-xiic-v1-0-e6207f9aa5ad@nexthop.ai

To: Michal Simek <michal.simek@amd.com>
To: Andi Shyti <andi.shyti@kernel.org>
To: Raviteja Narayanam <raviteja.narayanam@xilinx.com>
To: Wolfram Sang <wsa@kernel.org>
To: Manikanta Guntupalli <manikanta.guntupalli@amd.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-i2c@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

---
Abdurrahman Hussain (3):
      i2c: xiic: preserve PEC byte length in SMBus block read setup
      i2c: xiic: defer RX_FULL until all trailing bytes are in FIFO
      i2c: xiic: don't clobber msg->len to signal block-read completion

 drivers/i2c/busses/i2c-xiic.c | 60 +++++++++++++++++++++++++++++++------------
 1 file changed, 43 insertions(+), 17 deletions(-)
---
base-commit: 70eda68668d1476b459b64e69b8f36659fa9dfa8
change-id: 20260427-i2c-xiic-2aeb501ec02a

Best regards,
--  
Abdurrahman Hussain <abdurrahman@nexthop.ai>



^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v6 1/3] i2c: xiic: preserve PEC byte length in SMBus block read setup
  2026-09-24  1:45 [PATCH v6 0/3] i2c: xiic: fix SMBus block read and PEC support Abdurrahman Hussain
@ 2026-09-24  1:45 ` Abdurrahman Hussain
  2026-09-24 20:09   ` Andi Shyti
  2026-09-24  1:45 ` [PATCH v6 2/3] i2c: xiic: defer RX_FULL until all trailing bytes are in FIFO Abdurrahman Hussain
  2026-09-24  1:45 ` [PATCH v6 3/3] i2c: xiic: don't clobber msg->len to signal block-read completion Abdurrahman Hussain
  2 siblings, 1 reply; 6+ messages in thread
From: Abdurrahman Hussain @ 2026-09-24  1:45 UTC (permalink / raw)
  To: Michal Simek, Andi Shyti, Wolfram Sang, Raviteja Narayanam,
	Wolfram Sang, Manikanta Guntupalli
  Cc: Shubhrajyoti Datta, linux-arm-kernel, linux-i2c, linux-kernel,
	Abdurrahman Hussain, stable

xiic_smbus_block_read_setup() recalculates i2c->rx_msg->len based on the
length byte returned by the device, but historically clobbered the PEC
byte expectation the SMBus core had baked into msg->len. That dropped
the PEC byte from the caller's buffer on the normal and chunked
receive-fifo branches.

Compute pec_len up-front as (i2c->rx_msg->len - 1) and add it to the
new length in every branch:

  - chunked (rxmsg_len + pec_len > IIC_RX_FIFO_DEPTH): set the drain
    target to rxmsg_len + 1 + pec_len.

  - deferred (small enough to drain in one fill but >= MIN_LEN total):
    same.

  - padded (1 + rxmsg_len + pec_len < SMBUS_BLOCK_READ_MIN_LEN): the
    hardware needs at least 3 bytes on the bus to exit the read
    cleanly (the second byte is already being clocked in by the time
    the ISR reads the length byte and is too late to NACK), so we
    still pad rx_msg->len up to SMBUS_BLOCK_READ_MIN_LEN. The dummy
    trailing byte that gets drained must then be trimmed off before
    handing the message back to the SMBus core; otherwise
    i2c_smbus_check_pec() reads buf[len-1] (= dummy) instead of the
    real PEC byte at buf[1] and rejects every clean zero-length block
    read with -EBADMSG.

    Record the true valid byte count in a new field
    i2c->smbus_actual_len and trim rx_msg->len down to it in
    xiic_smbus_trim_len(), called from both completion sites that
    clear rx_msg: xiic_process()'s RX_FULL branch and
    xiic_recv_atomic(), which drains the FIFO with interrupts off.

    smbus_actual_len is per-receive state, so xiic_start_recv() clears
    it before every receive. Only the padded branch ever sets it, and
    a block read aborted by arbitration loss or a TX error never
    reaches the completion site, so without that clear a stale value
    would trim the length of an unrelated later read.

    Widen the branch condition from the old "(rxmsg_len == 1) ||
    (rxmsg_len == 0)" to "(1 + rxmsg_len + pec_len) < MIN_LEN" so that
    user requests with multi-byte trailing bytes (e.g. pec_len == 2 on
    a zero-length block) flow through the deferred branch instead of
    getting truncated to MIN_LEN here.

Fixes: e4c1ff772e1a ("i2c: xiic: Add smbus_block_read functionality")
Cc: stable@vger.kernel.org
Acked-by: Michal Simek <michal.simek@amd.com>
Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
---
 drivers/i2c/busses/i2c-xiic.c | 39 ++++++++++++++++++++++++++++++++-------
 1 file changed, 32 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index 3e7735e1dae0..6cb264cbc366 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -73,6 +73,9 @@ enum i2c_scl_freq {
  * @prev_msg_tx: Previous message is Tx
  * @quirks: To hold platform specific bug info
  * @smbus_block_read: Flag to handle block read
+ * @smbus_actual_len: Valid byte count (length + payload + optional PEC) of a
+ *	padded SMBus block read. msg->len is trimmed to this on completion.
+ *	Zero when no trimming is needed.
  * @input_clk: Input clock to I2C controller
  * @i2c_clk: I2C SCL frequency
  * @atomic: Mode of transfer
@@ -98,6 +101,7 @@ struct xiic_i2c {
 	bool prev_msg_tx;
 	u32 quirks;
 	bool smbus_block_read;
+	unsigned int smbus_actual_len;
 	unsigned long input_clk;
 	unsigned int i2c_clk;
 	bool atomic;
@@ -539,6 +543,8 @@ static void xiic_smbus_block_read_setup(struct xiic_i2c *i2c)
 
 	/* Check if received length is valid */
 	if (rxmsg_len <= I2C_SMBUS_BLOCK_MAX) {
+		unsigned int pec_len = i2c->rx_msg->len - 1;
+
 		/* Set Receive fifo depth */
 		if (rxmsg_len > IIC_RX_FIFO_DEPTH) {
 			/*
@@ -546,23 +552,26 @@ static void xiic_smbus_block_read_setup(struct xiic_i2c *i2c)
 			 * Receive fifo depth should set to Rx fifo capacity minus 1
 			 */
 			rfd_set = IIC_RX_FIFO_DEPTH - 1;
-			i2c->rx_msg->len = rxmsg_len + 1;
-		} else if ((rxmsg_len == 1) ||
-			(rxmsg_len == 0)) {
+			i2c->rx_msg->len = rxmsg_len + 1 + pec_len;
+		} else if (1 + rxmsg_len + pec_len < SMBUS_BLOCK_READ_MIN_LEN) {
 			/*
-			 * Minimum of 3 bytes required to exit cleanly. 1 byte
-			 * already received, Second byte is being received. Have
-			 * to set NACK in read_rx before receiving the last byte
+			 * The HW needs SMBUS_BLOCK_READ_MIN_LEN bytes on the
+			 * bus to exit cleanly: by the time the ISR reads the
+			 * length byte the second byte is already being clocked
+			 * in, too late to NACK. Pad the drain target and record
+			 * the real length, trimmed back on completion so the
+			 * PEC check sees the right byte.
 			 */
 			rfd_set = 0;
 			i2c->rx_msg->len = SMBUS_BLOCK_READ_MIN_LEN;
+			i2c->smbus_actual_len = 1 + rxmsg_len + pec_len;
 		} else {
 			/*
 			 * When Rx msg len less than Rx fifo capacity
 			 * Receive fifo depth should set to Rx msg len minus 2
 			 */
 			rfd_set = rxmsg_len - 2;
-			i2c->rx_msg->len = rxmsg_len + 1;
+			i2c->rx_msg->len = rxmsg_len + 1 + pec_len;
 		}
 		xiic_setreg8(i2c, XIIC_RFD_REG_OFFSET, rfd_set);
 
@@ -575,6 +584,16 @@ static void xiic_smbus_block_read_setup(struct xiic_i2c *i2c)
 	dev_err(i2c->adap.dev.parent, "smbus_block_read Invalid msg length\n");
 }
 
+/*
+ * Undo the setup-time padding of a short SMBus block read before rx_msg is
+ * cleared, so the PEC check sees the right byte.
+ */
+static void xiic_smbus_trim_len(struct xiic_i2c *i2c)
+{
+	if (i2c->rx_msg && i2c->smbus_actual_len)
+		i2c->rx_msg->len = i2c->smbus_actual_len;
+}
+
 static void xiic_read_rx(struct xiic_i2c *i2c)
 {
 	u8 bytes_in_fifo, cr = 0, bytes_to_read = 0;
@@ -797,6 +816,8 @@ static irqreturn_t xiic_process(int irq, void *dev_id)
 
 		xiic_read_rx(i2c);
 		if (xiic_rx_space(i2c) == 0) {
+			xiic_smbus_trim_len(i2c);
+
 			/* this is the last part of the message */
 			i2c->rx_msg = NULL;
 
@@ -938,6 +959,7 @@ static void xiic_recv_atomic(struct xiic_i2c *i2c)
 			return;
 	}
 
+	xiic_smbus_trim_len(i2c);
 	i2c->rx_msg = NULL;
 	xiic_irq_clr_en(i2c, XIIC_INTR_TX_ERROR_MASK);
 
@@ -955,6 +977,9 @@ static void xiic_start_recv(struct xiic_i2c *i2c)
 	u8 cr = 0, rfd_set = 0;
 	struct i2c_msg *msg = i2c->rx_msg = i2c->tx_msg;
 
+	/* A stale value from an aborted block read would truncate this msg. */
+	i2c->smbus_actual_len = 0;
+
 	if (!i2c->atomic)
 		dev_dbg(i2c->adap.dev.parent, "%s entry, ISR: 0x%x, CR: 0x%x\n",
 			__func__, xiic_getreg32(i2c, XIIC_IISR_OFFSET),

-- 
2.54.0



^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v6 2/3] i2c: xiic: defer RX_FULL until all trailing bytes are in FIFO
  2026-09-24  1:45 [PATCH v6 0/3] i2c: xiic: fix SMBus block read and PEC support Abdurrahman Hussain
  2026-09-24  1:45 ` [PATCH v6 1/3] i2c: xiic: preserve PEC byte length in SMBus block read setup Abdurrahman Hussain
@ 2026-09-24  1:45 ` Abdurrahman Hussain
  2026-09-24  1:45 ` [PATCH v6 3/3] i2c: xiic: don't clobber msg->len to signal block-read completion Abdurrahman Hussain
  2 siblings, 0 replies; 6+ messages in thread
From: Abdurrahman Hussain @ 2026-09-24  1:45 UTC (permalink / raw)
  To: Michal Simek, Andi Shyti, Wolfram Sang, Raviteja Narayanam,
	Wolfram Sang, Manikanta Guntupalli
  Cc: Shubhrajyoti Datta, linux-arm-kernel, linux-i2c, linux-kernel,
	Abdurrahman Hussain, stable

For the normal path of xiic_smbus_block_read_setup() (rxmsg_len less
than IIC_RX_FIFO_DEPTH), RFD was programmed to rxmsg_len - 2, which
fires the RX_FULL interrupt while the last payload byte is still in
flight. xiic_read_rx()'s bytes_rem == 1 branch then sets NACK on that
byte still on the wire, truncating the read in the PEC-enabled case.

Raise the threshold so RX_FULL fires only once every remaining byte
(payload plus optional PEC) is already buffered in the FIFO. That
routes the drain through xiic_read_rx()'s bytes_rem == 0 path, which
reads everything out and emits the stop cleanly. For the non-PEC path
the full payload is still read out through the same bytes_rem == 0
branch; the only user-visible change is that the controller waits one
extra byte-time before servicing the interrupt.

The deferred-fire formula is rxmsg_len + pec_len - 1, and the RFD
register at XIIC_RFD_REG_OFFSET is a 4-bit field. Widen the
chunk-vs-defer guard to (rxmsg_len + pec_len > IIC_RX_FIFO_DEPTH) so
the boundary case rxmsg_len == IIC_RX_FIFO_DEPTH with PEC enabled
cannot write 16 into that 4-bit register; it routes through the
chunked drain instead, which already caps RFD at IIC_RX_FIFO_DEPTH - 1.

Fixes: e4c1ff772e1a ("i2c: xiic: Add smbus_block_read functionality")
Cc: stable@vger.kernel.org
Acked-by: Michal Simek <michal.simek@amd.com>
Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
---
 drivers/i2c/busses/i2c-xiic.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index 6cb264cbc366..b4177d655cce 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -546,10 +546,11 @@ static void xiic_smbus_block_read_setup(struct xiic_i2c *i2c)
 		unsigned int pec_len = i2c->rx_msg->len - 1;
 
 		/* Set Receive fifo depth */
-		if (rxmsg_len > IIC_RX_FIFO_DEPTH) {
+		if (rxmsg_len + pec_len > IIC_RX_FIFO_DEPTH) {
 			/*
-			 * When Rx msg len greater than or equal to Rx fifo capacity
-			 * Receive fifo depth should set to Rx fifo capacity minus 1
+			 * Trailing payload (data + optional PEC) exceeds Rx FIFO
+			 * capacity; drain in chunks. Fire RX_FULL when the FIFO is
+			 * full and let the ISR re-arm for the remainder.
 			 */
 			rfd_set = IIC_RX_FIFO_DEPTH - 1;
 			i2c->rx_msg->len = rxmsg_len + 1 + pec_len;
@@ -566,11 +567,8 @@ static void xiic_smbus_block_read_setup(struct xiic_i2c *i2c)
 			i2c->rx_msg->len = SMBUS_BLOCK_READ_MIN_LEN;
 			i2c->smbus_actual_len = 1 + rxmsg_len + pec_len;
 		} else {
-			/*
-			 * When Rx msg len less than Rx fifo capacity
-			 * Receive fifo depth should set to Rx msg len minus 2
-			 */
-			rfd_set = rxmsg_len - 2;
+			/* Defer RX_FULL until all trailing bytes are in FIFO. */
+			rfd_set = rxmsg_len + pec_len - 1;
 			i2c->rx_msg->len = rxmsg_len + 1 + pec_len;
 		}
 		xiic_setreg8(i2c, XIIC_RFD_REG_OFFSET, rfd_set);

-- 
2.54.0



^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v6 3/3] i2c: xiic: don't clobber msg->len to signal block-read completion
  2026-09-24  1:45 [PATCH v6 0/3] i2c: xiic: fix SMBus block read and PEC support Abdurrahman Hussain
  2026-09-24  1:45 ` [PATCH v6 1/3] i2c: xiic: preserve PEC byte length in SMBus block read setup Abdurrahman Hussain
  2026-09-24  1:45 ` [PATCH v6 2/3] i2c: xiic: defer RX_FULL until all trailing bytes are in FIFO Abdurrahman Hussain
@ 2026-09-24  1:45 ` Abdurrahman Hussain
  2 siblings, 0 replies; 6+ messages in thread
From: Abdurrahman Hussain @ 2026-09-24  1:45 UTC (permalink / raw)
  To: Michal Simek, Andi Shyti, Wolfram Sang, Raviteja Narayanam,
	Wolfram Sang, Manikanta Guntupalli
  Cc: Shubhrajyoti Datta, linux-arm-kernel, linux-i2c, linux-kernel,
	Abdurrahman Hussain, stable

At the end of a SMBus block read the BNB handler force-set
tx_msg->len = 1 to push xiic_tx_space() to zero so the STATE_DONE
branch would fire. Two problems:

1. tx_msg and rx_msg alias the same i2c_msg struct during a receive
   (see xiic_start_recv), so overwriting tx_msg->len also changes
   rx_msg->len. The i2c core's i2c_smbus_check_pec() then reads the
   PEC from the wrong offset -- buf[0] instead of buf[rxmsg_len + 1]
   -- and either mis-validates or returns -EBADMSG.

2. xiic_start_recv sets tx_pos = msg->len (typically 2 when PEC is
   enabled). xiic_tx_space() is unsigned msg->len - tx_pos, so
   setting msg->len = 1 with tx_pos = 2 underflows to 0xFFFFFFFF and
   xiic_tx_space() never compares equal to 0 -- the STATE_DONE check
   falls through to STATE_ERROR, giving -EIO.

Instead, advance tx_pos up to msg->len. That drives tx_space to 0
without touching msg->len, preserving the buffer length that
xiic_smbus_block_read_setup() already grew to cover the length byte,
the payload and the optional PEC byte.

Fixes: e4c1ff772e1a ("i2c: xiic: Add smbus_block_read functionality")
Cc: stable@vger.kernel.org
Acked-by: Michal Simek <michal.simek@amd.com>
Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
---
 drivers/i2c/busses/i2c-xiic.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index b4177d655cce..5d8492ed25f1 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -884,8 +884,11 @@ static irqreturn_t xiic_process(int irq, void *dev_id)
 
 		if (i2c->tx_msg && i2c->smbus_block_read) {
 			i2c->smbus_block_read = false;
-			/* Set requested message len=1 to indicate STATE_DONE */
-			i2c->tx_msg->len = 1;
+			/*
+			 * Drive xiic_tx_space() to 0 to signal STATE_DONE
+			 * without truncating the rx_msg length.
+			 */
+			i2c->tx_pos = i2c->tx_msg->len;
 		}
 
 		if (!i2c->tx_msg)

-- 
2.54.0



^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v6 1/3] i2c: xiic: preserve PEC byte length in SMBus block read setup
  2026-09-24  1:45 ` [PATCH v6 1/3] i2c: xiic: preserve PEC byte length in SMBus block read setup Abdurrahman Hussain
@ 2026-09-24 20:09   ` Andi Shyti
  2026-09-25  0:09     ` Abdurrahman Hussain
  0 siblings, 1 reply; 6+ messages in thread
From: Andi Shyti @ 2026-09-24 20:09 UTC (permalink / raw)
  To: Abdurrahman Hussain
  Cc: Michal Simek, Wolfram Sang, Raviteja Narayanam, Wolfram Sang,
	Manikanta Guntupalli, Shubhrajyoti Datta, linux-arm-kernel,
	linux-i2c, linux-kernel, stable

Hi Abdurraham,

there is still another issue here.

...

> @@ -546,23 +552,26 @@ static void xiic_smbus_block_read_setup(struct xiic_i2c *i2c)
>  			 * Receive fifo depth should set to Rx fifo capacity minus 1
>  			 */
>  			rfd_set = IIC_RX_FIFO_DEPTH - 1;
> -			i2c->rx_msg->len = rxmsg_len + 1;
> -		} else if ((rxmsg_len == 1) ||
> -			(rxmsg_len == 0)) {
> +			i2c->rx_msg->len = rxmsg_len + 1 + pec_len;
> +		} else if (1 + rxmsg_len + pec_len < SMBUS_BLOCK_READ_MIN_LEN) {
>  			/*
> -			 * Minimum of 3 bytes required to exit cleanly. 1 byte
> -			 * already received, Second byte is being received. Have
> -			 * to set NACK in read_rx before receiving the last byte
> +			 * The HW needs SMBUS_BLOCK_READ_MIN_LEN bytes on the
> +			 * bus to exit cleanly: by the time the ISR reads the
> +			 * length byte the second byte is already being clocked
> +			 * in, too late to NACK. Pad the drain target and record
> +			 * the real length, trimmed back on completion so the
> +			 * PEC check sees the right byte.
>  			 */
>  			rfd_set = 0;
>  			i2c->rx_msg->len = SMBUS_BLOCK_READ_MIN_LEN;
> +			i2c->smbus_actual_len = 1 + rxmsg_len + pec_len;
>  		} else {
>  			/*
>  			 * When Rx msg len less than Rx fifo capacity
>  			 * Receive fifo depth should set to Rx msg len minus 2
>  			 */
>  			rfd_set = rxmsg_len - 2;

what if rxmsg is 1? Before this could never happen because we
were checking for rxmsg_len == 0 or 1 and we were ending up here
for values greater than 1.

In patch 2 you fix things, but we don't want to have dependencies
between patches.

Andi

> -			i2c->rx_msg->len = rxmsg_len + 1;
> +			i2c->rx_msg->len = rxmsg_len + 1 + pec_len;
>  		}
>  		xiic_setreg8(i2c, XIIC_RFD_REG_OFFSET, rfd_set);


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v6 1/3] i2c: xiic: preserve PEC byte length in SMBus block read setup
  2026-09-24 20:09   ` Andi Shyti
@ 2026-09-25  0:09     ` Abdurrahman Hussain
  0 siblings, 0 replies; 6+ messages in thread
From: Abdurrahman Hussain @ 2026-09-25  0:09 UTC (permalink / raw)
  To: Andi Shyti, Abdurrahman Hussain
  Cc: Michal Simek, Wolfram Sang, Raviteja Narayanam, Wolfram Sang,
	Manikanta Guntupalli, Shubhrajyoti Datta, linux-arm-kernel,
	linux-i2c, linux-kernel, stable

Hi Andi,

On Thu Sep 24, 2026 at 1:09 PM PDT, Andi Shyti wrote:
> what if rxmsg is 1? Before this could never happen because we
> were checking for rxmsg_len == 0 or 1 and we were ending up here
> for values greater than 1.
>
> In patch 2 you fix things, but we don't want to have dependencies
> between patches.
>

Confirmed. The widened condition makes the else branch reachable with
rxmsg_len < 2, where rfd_set = rxmsg_len - 2 wraps the u8. Traced on
hardware against a zero-length block read, sweeping pec_len:

  pec_len:             0    1     2      3
  v6 patch 1 rfd_set:  0    0    254    254
  v7 patch 1 rfd_set:  0    0     0      1

It doesn't actually hang on my boards -- 254 lands in the 4-bit RFD
field as 14, and both slaves I have keep clocking past the end of the
block, so the FIFO still reaches 15. The programmed value is wrong
regardless.

Restoring the old "(rxmsg_len == 1) || (rxmsg_len == 0)" isn't right
either: pec_len isn't limited to 0 or 1, because i2c-dev sets msg->len
from caller-supplied buf[0]. At rxmsg_len == 1, pec_len == 2 the padded
branch would record smbus_actual_len = 4 while draining only 3.

So v7 moves the bounds into patch 1, where the arithmetic is introduced:

  - the guard becomes (rxmsg_len + pec_len > IIC_RX_FIFO_DEPTH), which
    is what keeps rfd_set inside the 4-bit field;
  - the else branch becomes rfd_set = rxmsg_len + pec_len - 2, identical
    to the old expression at pec_len == 0 and unable to underflow, since
    the padded branch already takes everything below MIN_LEN total.

Patch 2 is then just - 2 to - 1. Resulting tree is identical to v6.

I also finally exercised the atomic trim you asked for in v5, by routing
transfers through xiic_xfer_atomic: it fires, and block lengths 0 to 32
read back correctly on that path.

Two notes from the per-patch run. Unpatched, pmbus_core creates no mfr_*
files for these devices and every PEC block read returns -EIO; with the
series they read correctly. But patch 1 alone isn't observable
end-to-end (patch 3's -EIO masks it), and patch 2 changes nothing I can
measure on my two slaves -- its bug needs one that NACKs promptly rather
than streaming.

While I have your attention: the other xiic patch, "i2c: xiic: restore
non-managed runtime PM to fix clk WARN flood", is still unapplied. Andy
acked it on 10 Sep and the review comment it had is closed. It fixes a
regression from my own 50c63491ff26, which is in both v7.1 and v7.2, so
7.3-rcX would be a good target if it looks right to you.

  https://patch.msgid.link/20260821-i2c-xiic-restore-runtime-pm-teardown-v7-1-954e06765144@nexthop.ai

Unrelated, for later: smbus_block_read is only cleared in the BNB
handler, which the atomic path never reaches.

Abdurrahman



^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2026-09-25  0:09 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-09-24  1:45 [PATCH v6 0/3] i2c: xiic: fix SMBus block read and PEC support Abdurrahman Hussain
2026-09-24  1:45 ` [PATCH v6 1/3] i2c: xiic: preserve PEC byte length in SMBus block read setup Abdurrahman Hussain
2026-09-24 20:09   ` Andi Shyti
2026-09-25  0:09     ` Abdurrahman Hussain
2026-09-24  1:45 ` [PATCH v6 2/3] i2c: xiic: defer RX_FULL until all trailing bytes are in FIFO Abdurrahman Hussain
2026-09-24  1:45 ` [PATCH v6 3/3] i2c: xiic: don't clobber msg->len to signal block-read completion Abdurrahman Hussain

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox