* Re: [PATCH] i2c: amd: fix SMBus block header and PEC handling in the RX path
2026-06-27 15:35 [PATCH] i2c: amd: fix SMBus block header and PEC handling in the RX path Paritosh Potukuchi
@ 2026-09-10 18:44 ` Paritosh Potukuchi
2026-11-02 7:38 ` Shyam Sundar S K
1 sibling, 0 replies; 3+ messages in thread
From: Paritosh Potukuchi @ 2026-09-10 18:44 UTC (permalink / raw)
To: andi.shyti, shyam-sundar.s-k; +Cc: linux-i2c, linux-kernel, Paritosh Potukuchi
Hi Andi and Shyam,
Friendly ping on this patch.
Could you please let me know if there are any comments or concerns?
Thanks,
Paritosh
On Sat, 27 Jun 2026 at 21:05, Paritosh Potukuchi
<paritoshpotukuchi@gmail.com> wrote:
>
> Strip SMBus block header bytes from the received buffer before
> forwarding the payload to the upper layers.
>
> Handles the optional PEC Byte at the end of the received
> payload.
>
> Signed-off-by: Paritosh Potukuchi <paritosh.potukuchi@amd.com>
> ---
> drivers/i2c/busses/i2c-amd-asf-plat.c | 34 +++++++++++++++++++++++++--
> 1 file changed, 32 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-amd-asf-plat.c b/drivers/i2c/busses/i2c-amd-asf-plat.c
> index e1699da838c2..daf318ce3bbb 100644
> --- a/drivers/i2c/busses/i2c-amd-asf-plat.c
> +++ b/drivers/i2c/busses/i2c-amd-asf-plat.c
> @@ -46,6 +46,8 @@
> #define ASF_BLOCK_MAX_BYTES 72
> #define ASF_ERROR_STATUS GENMASK(3, 1)
>
> +#define SMBUS_BLOCK_HEADER_SIZE 2
> +
> struct amd_asf_dev {
> struct i2c_adapter adap;
> void __iomem *eoi_base;
> @@ -61,7 +63,7 @@ static void amd_asf_process_target(struct work_struct *work)
> unsigned short piix4_smba = dev->port_addr->start;
> u8 data[ASF_BLOCK_MAX_BYTES];
> u8 bank, reg, cmd;
> - u8 len = 0, idx, val;
> + u8 len = 0, idx, val, offset, data_len, payload_len, has_pec;
>
> /* Read target status register */
> reg = inb_p(ASFSLVSTA);
> @@ -110,12 +112,40 @@ static void amd_asf_process_target(struct work_struct *work)
> if (cmd & BIT(0))
> return;
>
> +
> + /*Below is the SMBus block header received on the wire:
> + * Command -- 1 Byte
> + * Data Length -- 1 Byte (number of data bytes)
> + * Data -- N Bytes
> + * PEC -- optional, 1 Byte
> + *
> + * PEC, if present, is the final byte of the transfer
> + */
> +
> + offset = SMBUS_BLOCK_HEADER_SIZE;
> + if (len < offset)
> + return;
> +
> + payload_len = data[1];
> +
> + if (payload_len > len - offset)
> + return;
> +
> + if (len == offset + payload_len + 1)
> + has_pec = 1;
> + else if (len == offset + payload_len)
> + has_pec = 0;
> + else
> + return;
> +
> + data_len = len - has_pec;
> +
> /*
> * Although i2c_slave_event() returns an appropriate error code, we
> * don't check it here because we're operating in the workqueue context.
> */
> i2c_slave_event(dev->target, I2C_SLAVE_WRITE_REQUESTED, &val);
> - for (idx = 0; idx < len; idx++) {
> + for (idx = offset; idx < data_len; idx++) {
> val = data[idx];
> i2c_slave_event(dev->target, I2C_SLAVE_WRITE_RECEIVED, &val);
> }
> --
> 2.43.0
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] i2c: amd: fix SMBus block header and PEC handling in the RX path
2026-06-27 15:35 [PATCH] i2c: amd: fix SMBus block header and PEC handling in the RX path Paritosh Potukuchi
2026-09-10 18:44 ` Paritosh Potukuchi
@ 2026-11-02 7:38 ` Shyam Sundar S K
1 sibling, 0 replies; 3+ messages in thread
From: Shyam Sundar S K @ 2026-11-02 7:38 UTC (permalink / raw)
To: Paritosh Potukuchi, andi.shyti
Cc: linux-i2c, linux-kernel, Paritosh Potukuchi, Sanket Goswami
Hi Paritosh,
Apologies, I missed this. Comments below.
On 27-06-2026 21:05, Paritosh Potukuchi wrote:
> [You don't often get email from paritoshpotukuchi@gmail.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> Strip SMBus block header bytes from the received buffer before
> forwarding the payload to the upper layers.
>
> Handles the optional PEC Byte at the end of the received
> payload.
Can you say a bit more about why this is needed? Which target backend
are you using that wants only the payload?
I don't think the bus driver is the right place for this. The target
interface hands the raw bytes from the wire to the backend, and the
backend decides what they mean. mctp-i2c, for example, expects to see
the command code, byte count and PEC, and checks the PEC itself (see
mctp_i2c_recv()). With this change every MCTP packet would get dropped
there.
>
> Signed-off-by: Paritosh Potukuchi <paritosh.potukuchi@amd.com>
> ---
> drivers/i2c/busses/i2c-amd-asf-plat.c | 34 +++++++++++++++++++++++++--
> 1 file changed, 32 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-amd-asf-plat.c b/drivers/i2c/busses/i2c-amd-asf-plat.c
> index e1699da838c2..daf318ce3bbb 100644
> --- a/drivers/i2c/busses/i2c-amd-asf-plat.c
> +++ b/drivers/i2c/busses/i2c-amd-asf-plat.c
> @@ -46,6 +46,8 @@
> #define ASF_BLOCK_MAX_BYTES 72
> #define ASF_ERROR_STATUS GENMASK(3, 1)
>
> +#define SMBUS_BLOCK_HEADER_SIZE 2
Use a tab here so it lines up with the defines above.
> +
> struct amd_asf_dev {
> struct i2c_adapter adap;
> void __iomem *eoi_base;
> @@ -61,7 +63,7 @@ static void amd_asf_process_target(struct work_struct *work)
> unsigned short piix4_smba = dev->port_addr->start;
> u8 data[ASF_BLOCK_MAX_BYTES];
> u8 bank, reg, cmd;
> - u8 len = 0, idx, val;
> + u8 len = 0, idx, val, offset, data_len, payload_len, has_pec;
That line is getting long. has_pec should be a bool, and offset is
always SMBUS_BLOCK_HEADER_SIZE, so it doesn't need its own variable.
>
> /* Read target status register */
> reg = inb_p(ASFSLVSTA);
> @@ -110,12 +112,40 @@ static void amd_asf_process_target(struct work_struct *work)
> if (cmd & BIT(0))
> return;
>
> +
Extra blank line.
> + /*Below is the SMBus block header received on the wire:
Missing space after "/*". Also, this describes the whole frame, not
only the header.
> + * Command -- 1 Byte
> + * Data Length -- 1 Byte (number of data bytes)
> + * Data -- N Bytes
> + * PEC -- optional, 1 Byte
> + *
> + * PEC, if present, is the final byte of the transfer
> + */
> +
> + offset = SMBUS_BLOCK_HEADER_SIZE;
> + if (len < offset)
> + return;
> +
> + payload_len = data[1];
This assumes every write is an SMBus block write. Byte/word writes,
or i2c-slave-eeprom (offset byte followed by data), don't have a
length in data[1], so they'll fail the checks below and get dropped
without any message.
> +
> + if (payload_len > len - offset)
> + return;
> +
> + if (len == offset + payload_len + 1)
> + has_pec = 1;
> + else if (len == offset + payload_len)
> + has_pec = 0;
> + else
> + return;
PEC is only guessed from the length here, and it's never checked
before it gets removed. Does the controller check it when
ASF_PEC_SP is set? If not, a corrupted frame of the right length
would be passed up as good data.
These silent returns will also be hard to debug in the field. A
dev_dbg() would help.
> +
> + data_len = len - has_pec;
data_len is really the end index, not a length. The name is a bit
misleading.
> +
> /*
> * Although i2c_slave_event() returns an appropriate error code, we
> * don't check it here because we're operating in the workqueue context.
> */
> i2c_slave_event(dev->target, I2C_SLAVE_WRITE_REQUESTED, &val);
> - for (idx = 0; idx < len; idx++) {
> + for (idx = offset; idx < data_len; idx++) {
> val = data[idx];
> i2c_slave_event(dev->target, I2C_SLAVE_WRITE_RECEIVED, &val);
> }
If your backend only needs the payload, I'd strip the header and
handle PEC in the backend and leave this loop as it is.
Also, the subject line should start with "i2c: amd-asf:".
Thanks,
Shyam
^ permalink raw reply [flat|nested] 3+ messages in thread