| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID:
<VI2PR04MB11147E36D800B50E0C1818AB3E8D7A@VI2PR04MB11147.eurprd04.prod.outlook.com>
Date: Wed, 19 Nov 2025 02:33:44 +0000
From: Carlos Song <carlos.song@....com>
To: Frank Li <frank.li@....com>
CC: Aisheng Dong <aisheng.dong@....com>, "andi.shyti@...nel.org"
<andi.shyti@...nel.org>, "shawnguo@...nel.org" <shawnguo@...nel.org>,
"s.hauer@...gutronix.de" <s.hauer@...gutronix.de>, "kernel@...gutronix.de"
<kernel@...gutronix.de>, "festevam@...il.com" <festevam@...il.com>,
"pandy.gao@....com" <pandy.gao@....com>, "wsa@...nel.org" <wsa@...nel.org>,
"vz@...ia.com" <vz@...ia.com>, "B38611@...escale.com" <B38611@...escale.com>,
"linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
"imx@...ts.linux.dev" <imx@...ts.linux.dev>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] i2c: imx-lpi2c: support smbus block read feature
> -----Original Message-----
> From: Frank Li <frank.li@....com>
> Sent: Wednesday, November 19, 2025 12:45 AM
> To: Carlos Song <carlos.song@....com>
> Cc: Aisheng Dong <aisheng.dong@....com>; andi.shyti@...nel.org;
> shawnguo@...nel.org; s.hauer@...gutronix.de; kernel@...gutronix.de;
> festevam@...il.com; pandy.gao@....com; wsa@...nel.org; vz@...ia.com;
> B38611@...escale.com; linux-i2c@...r.kernel.org; imx@...ts.linux.dev;
> linux-arm-kernel@...ts.infradead.org; linux-kernel@...r.kernel.org
> Subject: Re: [PATCH] i2c: imx-lpi2c: support smbus block read feature
>
> On Tue, Nov 18, 2025 at 07:13:23PM +0800, Carlos Song wrote:
> > The LPI2C controller automatically transmits a NACK on the last byte
> > of a receive data command. It transmits the NACK unless the next
> > command in the TXFIFO is also a receive data command. If the transmit
> > FIFO is empty when a receive data command completes, a NACK is also
> > automatically transmitted.
> >
> > Specially set read 2 bytes command initially. When the RXFIFO receives
> > count data, controller should immediately read out this value and
> > update MTDR register to keep the TXFIFO not empty. Set new receive
> > data command to read other data before the 2nd byte is returned.
> >
> > Fixes: a55fa9d0e42e ("i2c: imx-lpi2c: add low power i2c bus driver")
> > Signed-off-by: Carlos Song <carlos.song@....com>
> >
> > ---
> > I setup an env to test this feature change:
> >
> > On I.MX93-EVK:
> > LPI2C5 as slave device and use i2ctool to redesign the slave-eeprom mem:
> >
> > hexdump -C /sys/bus/i2c/devices/5-1064/slave-eeprom
> >
> > root@...93evk:~# hexdump -C /sys/bus/i2c/devices/5-1064/slave-eeprom
> > 00000000 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
> > |................|
> > 00000010 20 00 01 02 03 04 05 06 07 08 09 0a 0b 0c 0d 0e |
> > ...............|
> > 00000020 0f 10 11 12 13 14 15 16 17 18 19 1a 1b 1c 1d 1e
> > |................|
> > 00000030 1f ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
> > |................|
> > 00000040 08 00 01 02 03 04 05 06 07 ff ff ff ff ff ff ff
> > |................|
> > 00000050 00 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
> > |................|
> > 00000060 40 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
> > |@...............|
> > 00000070 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
> > |................|
> >
> > case1: SMBus block read 0x10 register. SMBus len = 32 bytes.
> > case2: SMBus block read 0x40 register. SMBus len = 8 bytes.
> > case3: SMBus block read 0x50 register. SMBus len = 0 bytes. It break
> > the SMBus spec(len = 0).
> > case4: SMBus block read 0x60 register. SMBus len = 64 bytes. It break
> > the SMBus spec(len >= 32bytes).
> >
> > LPI2C3 as master controller to smbus block tead the slave device at 0x64.
> > Not apply this fix:
> > root@...93evk:~# i2cget -f -y 3 0x64 0x10 s
> > Error: Read failed
> > root@...93evk:~# i2cget -f -y 3 0x64 0x40 s
> > Error: Read failed
> > root@...93evk:~# i2cget -f -y 3 0x64 0x50 s
> > Error: Read failed
> > root@...93evk:~# i2cget -f -y 3 0x64 0x60 s
> > 0x00
> >
> > After apply this fix:
> > root@...93evk:~# i2cget -f -y 3 0x64 0x10 s
>
> where show your read 32 byte, do you means "i2cget -f -y 3 0x64 0x10 s 0x20"?
>
> > 0x00 0x01 0x02 0x03 0x04 0x05 0x06 0x07 0x08 0x09 0x0a 0x0b 0x0c 0x0d
> > 0x0e 0x0f
> > 0x10 0x11 0x12 0x13 0x14 0x15 0x16 0x17 0x18 0x19 0x1a 0x1b 0x1c 0x1d
> > 0x1e 0x1f
>
> according to above dump, the data from 0x11, not 0x10.
>
> Frank
>
Hi, Frank
Wow! Important point!
SMBUS protocol shows. RX buffer[0] is Byte Count. Then RX buffer[1] [2] [3].. dataN.
[Slave Addr][Command][Read] → [Byte Count][Data0][Data1]...[DataN]
The printed values are indeed the actual data bytes returned by the device, starting from the “first data byte” and excluding the "SMBus protocol length byte".
Raw code in i2ctool is:
/* Returns the number of read bytes */
__s32 i2c_smbus_read_block_data(int file, __u8 command, __u8 *values)
{
union i2c_smbus_data data;
int i, err;
err = i2c_smbus_access(file, I2C_SMBUS_READ, command,
I2C_SMBUS_BLOCK_DATA, &data);
if (err < 0)
return err;
for (i = 1; i <= data.block[0]; i++)
values[i-1] = data.block[i];
return data.block[0];
}
int main(int argc, char *argv[])
{
...
case I2C_SMBUS_BLOCK_DATA:
res = i2c_smbus_read_block_data(file, daddress, block_data);
break;
...
if (size == I2C_SMBUS_BLOCK_DATA ||
size == I2C_SMBUS_I2C_BLOCK_DATA) {
int i;
for (i = 0; i < res - 1; ++i)
printf("0x%02hhx ", block_data[i]);
printf("0x%02hhx\n", block_data[res - 1]);
...
Carlos
> > root@...93evk:~# i2cget -f -y 3 0x64 0x40 s
> > 0x00 0x01 0x02 0x03 0x04 0x05 0x06 0x07 root@...93evk:~# i2cget -f -y
> > 3 0x64 0x50 s [ 184.098094] i2c i2c-3: Invalid SMBus block read
> > length
> > Error: Read failed
> > root@...93evk:~# i2cget -f -y 3 0x64 0x60 s [ 179.722105] i2c i2c-3:
> > Invalid SMBus block read length
> > Error: Read failed
> >
> > ---
> > drivers/i2c/busses/i2c-imx-lpi2c.c | 93
> > +++++++++++++++++++++++-------
> > 1 file changed, 73 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c
> > b/drivers/i2c/busses/i2c-imx-lpi2c.c
> > index d882126c1778..39088567db59 100644
> > --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> > +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
> > @@ -90,6 +90,7 @@
> > #define MRDR_RXEMPTY BIT(14)
> > #define MDER_TDDE BIT(0)
> > #define MDER_RDDE BIT(1)
> > +#define MSR_RDF_ASSERT(x) FIELD_GET(MSR_RDF, (x))
> >
> > #define SCR_SEN BIT(0)
> > #define SCR_RST BIT(1)
> > @@ -461,7 +462,7 @@ static bool lpi2c_imx_write_txfifo(struct
> > lpi2c_imx_struct *lpi2c_imx, bool atom
> >
> > static bool lpi2c_imx_read_rxfifo(struct lpi2c_imx_struct *lpi2c_imx,
> > bool atomic) {
> > - unsigned int blocklen, remaining;
> > + unsigned int remaining;
> > unsigned int temp, data;
> >
> > do {
> > @@ -472,15 +473,6 @@ static bool lpi2c_imx_read_rxfifo(struct
> lpi2c_imx_struct *lpi2c_imx, bool atomi
> > lpi2c_imx->rx_buf[lpi2c_imx->delivered++] = data & 0xff;
> > } while (1);
> >
> > - /*
> > - * First byte is the length of remaining packet in the SMBus block
> > - * data read. Add it to msgs->len.
> > - */
> > - if (lpi2c_imx->block_data) {
> > - blocklen = lpi2c_imx->rx_buf[0];
> > - lpi2c_imx->msglen += blocklen;
> > - }
> > -
> > remaining = lpi2c_imx->msglen - lpi2c_imx->delivered;
> >
> > if (!remaining) {
> > @@ -493,12 +485,7 @@ static bool lpi2c_imx_read_rxfifo(struct
> lpi2c_imx_struct *lpi2c_imx, bool atomi
> > lpi2c_imx_set_rx_watermark(lpi2c_imx);
> >
> > /* multiple receive commands */
> > - if (lpi2c_imx->block_data) {
> > - lpi2c_imx->block_data = 0;
> > - temp = remaining;
> > - temp |= (RECV_DATA << 8);
> > - writel(temp, lpi2c_imx->base + LPI2C_MTDR);
> > - } else if (!(lpi2c_imx->delivered & 0xff)) {
> > + if (!(lpi2c_imx->delivered & 0xff)) {
> > temp = (remaining > CHUNK_DATA ? CHUNK_DATA : remaining) - 1;
> > temp |= (RECV_DATA << 8);
> > writel(temp, lpi2c_imx->base + LPI2C_MTDR); @@ -536,18 +523,80
> @@
> > static int lpi2c_imx_write_atomic(struct lpi2c_imx_struct *lpi2c_imx,
> > return err;
> > }
> >
> > +static unsigned int lpi2c_SMBus_block_read_single_byte(struct
> > +lpi2c_imx_struct *lpi2c_imx) {
> > + unsigned int val, data;
> > + int ret;
> > +
> > + ret = readl_poll_timeout(lpi2c_imx->base + LPI2C_MSR, val,
> > + MSR_RDF_ASSERT(val), 1, 1000);
> > + if (ret) {
> > + dev_err(&lpi2c_imx->adapter.dev, "SMBus read count timeout %d\n",
> ret);
> > + return ret;
> > + }
> > +
> > + data = readl(lpi2c_imx->base + LPI2C_MRDR);
> > + lpi2c_imx->rx_buf[lpi2c_imx->delivered++] = data & 0xff;
> > +
> > + return data;
> > +}
> > +
> > static void lpi2c_imx_read_init(struct lpi2c_imx_struct *lpi2c_imx,
> > struct i2c_msg *msgs)
> > {
> > - unsigned int temp;
> > + unsigned int temp, ret, block_len;
> >
> > lpi2c_imx->rx_buf = msgs->buf;
> > lpi2c_imx->block_data = msgs->flags & I2C_M_RECV_LEN;
> >
> > lpi2c_imx_set_rx_watermark(lpi2c_imx);
> > - temp = msgs->len > CHUNK_DATA ? CHUNK_DATA - 1 : msgs->len - 1;
> > - temp |= (RECV_DATA << 8);
> > - writel(temp, lpi2c_imx->base + LPI2C_MTDR);
> > +
> > + if (!lpi2c_imx->block_data) {
> > + temp = msgs->len > CHUNK_DATA ? CHUNK_DATA - 1 : msgs->len - 1;
> > + temp |= (RECV_DATA << 8);
> > + writel(temp, lpi2c_imx->base + LPI2C_MTDR);
> > + } else {
> > + /*
> > + * The LPI2C controller automatically transmits a NACK on the last
> byte
> > + * of a receive data command. It transmits the NACK unless the next
> > + * command in the TXFIFO is also a receive data command. If the
> transmit
> > + * FIFO is empty when a receive data command completes, a NACK is
> also
> > + * automatically transmitted.
> > + * So specially set read 2 bytes read command initially. First byte in
> > + * RXFIFO is SMBus block data length, when the data enter the
> RXFIFO,
> > + * controller should immediately read out and update MTDR register
> to keep
> > + * the TXFIFO not empty. Second byte is the first byte of block data.
> > + * So the data length of the subsequent block data read command
> should be
> > + * block_len - 1, because in the first read command, the first byte of
> block
> > + * data has already been stored in RXFIFO.
> > + */
> > + writel((RECV_DATA << 8) | 0x01, lpi2c_imx->base + LPI2C_MTDR);
> > +
> > + /* Read the first byte as block len */
> > + block_len = lpi2c_SMBus_block_read_single_byte(lpi2c_imx);
> > + if (block_len < 0) {
> > + dev_err(&lpi2c_imx->adapter.dev, "SMBus read data length
> timeout\n");
> > + return;
> > + }
> > +
> > + /* Confirm SMBus transfer meets protocol */
> > + if (block_len == 0 || block_len > I2C_SMBUS_BLOCK_MAX) {
> > + dev_err(&lpi2c_imx->adapter.dev, "Invalid SMBus block read
> length\n");
> > + return;
> > + }
> > +
> > + /* If just read 1 byte then read out from fifo. No need new command
> update */
> > + if (block_len == 1) {
> > + ret = lpi2c_SMBus_block_read_single_byte(lpi2c_imx);
> > + if (ret < 0)
> > + dev_err(&lpi2c_imx->adapter.dev, "SMBus read data
> timeout\n");
> > + return;
> > + }
> > +
> > + /* Block read other length data need to update command again*/
> > + writel((RECV_DATA << 8) | (block_len - 2), lpi2c_imx->base +
> LPI2C_MTDR);
> > + lpi2c_imx->msglen += block_len;
> > + }
> > }
> >
> > static bool lpi2c_imx_read_chunk_atomic(struct lpi2c_imx_struct
> > *lpi2c_imx) @@ -599,6 +648,10 @@ static bool is_use_dma(struct
> lpi2c_imx_struct *lpi2c_imx, struct i2c_msg *msg)
> > if (pm_suspend_in_progress())
> > return false;
> >
> > + /* DMA is not suitable for SMBus block read */
> > + if (msg->flags & I2C_M_RECV_LEN)
> > + return false;
> > +
> > /*
> > * When the length of data is less than I2C_DMA_THRESHOLD,
> > * cpu mode is used directly to avoid low performance.
> > --
> > 2.34.1
> >
Powered by blists - more mailing lists