Re: [PATCH v3 0/3] i2c: xiic: fix SMBus block read and PEC support

[Abdurrahman Hussain]

On Tue Jun 9, 2026 at 7:41 AM PDT, Michal Simek wrote:
>
>
> On 5/13/26 12:09, Abdurrahman Hussain wrote:
>> 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@xxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxx
>> 
>> 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@xxxxxxxxxx
>> 
>> To: Michal Simek <michal.simek@xxxxxxx>
>> To: Andi Shyti <andi.shyti@xxxxxxxxxx>
>> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
>> Cc: linux-i2c@xxxxxxxxxxxxxxx
>> Cc: linux-kernel@xxxxxxxxxxxxxxx
>> 
>> ---
>> 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 | 67 ++++++++++++++++++++++++++++++++-----------
>>   1 file changed, 50 insertions(+), 17 deletions(-)
>> ---
>> base-commit: 254f49634ee16a731174d2ae34bc50bd5f45e731
>> change-id: 20260427-i2c-xiic-2aeb501ec02a
>> 
>> Best regards,
>> --
>> Abdurrahman Hussain <abdurrahman@xxxxxxxxxx>
>> 
>
> Acked-by: Michal Simek <michal.simek@xxxxxxx>
>
> Thanks,
> Michal

Hi Andi,

I haven't heard from the list on this patch series in a while. Could
this be added to the next merge window? I'd be more than happy to
address any issues/comments.

Thanks,
Abdurrahman