| 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: <f62668cb-ad01-495e-86c3-82f92fa5ad90@gmail.com>
Date: Fri, 26 Sep 2025 10:16:19 +0200
From: Jonas Jelonek <jelonek.jonas@...il.com>
To: Sven Eckelmann <sven@...fation.org>
Cc: linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org,
Harshal Gohel <hg@...onwunderlich.de>,
Simon Wunderlich <sw@...onwunderlich.de>, Andi Shyti
<andi.shyti@...nel.org>, Chris Packham <chris.packham@...iedtelesis.co.nz>,
Wolfram Sang <wsa+renesas@...g-engineering.com>
Subject: Re: [PATCH i2c-host v6] i2c: rtl9300: Implement I2C block read and
write
Hi Sven,
On 26.09.25 09:52, Sven Eckelmann wrote:
> From: Harshal Gohel <hg@...onwunderlich.de>
>
> It was noticed that the original implementation of SMBus Block Write in the
> driver was actually an I2C Block Write. Both differ only in the Count byte
> before the actual data:
>
> S Addr Wr [A] Comm [A] Count [A] Data [A] Data [A] ... [A] Data [A] P
>
> The I2C Block Write is just skipping this Count byte and starts directly
> with the data:
>
> S Addr Wr [A] Comm [A] Data [A] Data [A] ... [A] Data [A] P
>
> The I2C controller of RTL93xx doesn't handle this Count byte special and it
> is simply another one of (16 possible) data bytes. Adding support for the
> I2C Block Write therefore only requires skipping the count byte (0) in
> data->block.
>
> It is similar for reads. The SMBUS Block read is having a Count byte before
> the data:
>
> S Addr Wr [A] Comm [A]
> Sr Addr Rd [A] [Count] A [Data] A [Data] A ... A [Data] NA P
>
> And the I2C Block Read is directly starting with the actual data:
>
> S Addr Wr [A] Comm [A]
> Sr Addr Rd [A] [Data] A [Data] A ... A [Data] NA P
>
> The I2C controller is also not handling this byte in a special way. It
> simply provides every byte after the Rd marker + Ack as part of the 16 byte
> receive buffer (registers). The content of this buffer just has to be
> copied to the right position in the receive data->block.
>
> Signed-off-by: Harshal Gohel <hg@...onwunderlich.de>
> Co-developed-by: Sven Eckelmann <sven@...fation.org>
> Signed-off-by: Sven Eckelmann <sven@...fation.org>
> Reviewed-by: Chris Packham <chris.packham@...iedtelesis.co.nz>
> Tested-by: Chris Packham <chris.packham@...iedtelesis.co.nz>
> Reviewed-by: Jonas Jelonek <jelonek.jonas@...il.com>
> Tested-by: Jonas Jelonek <jelonek.jonas@...il.com>
> ---
> This patch was already applied [1] but then removed. Instead, only the
> chunk
>
> @@ -314,7 +343,7 @@ static u32 rtl9300_i2c_func(struct i2c_adapter *a)
> {
> return I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE |
> I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA |
> - I2C_FUNC_SMBUS_BLOCK_DATA;
> + I2C_FUNC_SMBUS_BLOCK_DATA | I2C_FUNC_SMBUS_I2C_BLOCK;
> }
>
> was added as part of a patch which has nothing to do with
> I2C_FUNC_SMBUS_I2C_BLOCK [2] and was never submitted like this [3].
>
> I am therefore resubmitting this patch again without the rtl9300_i2c_func
> change.
>
> [1] https://lore.kernel.org/r/a422shurtl3xrvnh2ieynqq2kw5awqnmall2wjdpozx336m26i@54ekftmkwvrv
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git/commit/?h=i2c/i2c-host&id=ede965fd555ac2536cf651893a998dbfd8e57b86
> [3] https://lore.kernel.org/r/20250831100457.3114-4-jelonek.jonas@gmail.com
> ---
> Changes in v6:
> - drop all fixes patches (which were already applied)
> - drop rtl9300_i2c_func chunk which was incorrectly added by another commit
> [2] (but was not intended to be in there by the original patch [3]
> - Link to v5: https://lore.kernel.org/r/20250810-i2c-rtl9300-multi-byte-v5-0-cd9dca0db722@narfation.org
>
> Changes in v5:
> - Simplify function/capability registration by using
> I2C_FUNC_SMBUS_I2C_BLOCK, thanks Jonas Jelonek
> - Link to v4: https://lore.kernel.org/r/20250809-i2c-rtl9300-multi-byte-v4-0-d71dd5eb6121@narfation.org
>
> Changes in v4:
> - Provide only "write" examples for "i2c: rtl9300: Fix multi-byte I2C write"
> - drop the second initialization of vals in rtl9300_i2c_write() directly in
> the "Fix multi-byte I2C write" fix
> - indicate in target branch for each patch in PATCH prefix
> - minor commit message cleanups
> - Link to v3: https://lore.kernel.org/r/20250804-i2c-rtl9300-multi-byte-v3-0-e20607e1b28c@narfation.org
>
> Changes in v3:
> - integrated patch
> https://lore.kernel.org/r/20250615235248.529019-1-alexguo1023@gmail.com
> to avoid conflicts in the I2C_SMBUS_BLOCK_DATA code
> - added Fixes and stable@...r.kernel.org to Alex Guo's patch
> - added Chris Packham's Reviewed-by/Acked-by
> - Link to v2: https://lore.kernel.org/r/20250803-i2c-rtl9300-multi-byte-v2-0-9b7b759fe2b6@narfation.org
>
> Changes in v2:
> - add the missing transfer width and read length increase for the SMBus
> Write/Read
> - Link to v1: https://lore.kernel.org/r/20250802-i2c-rtl9300-multi-byte-v1-0-5f687e0098e2@narfation.org
> ---
> drivers/i2c/busses/i2c-rtl9300.c | 35 ++++++++++++++++++++++++++++++++---
> 1 file changed, 32 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-rtl9300.c b/drivers/i2c/busses/i2c-rtl9300.c
> index 9e1f71fed0feac41e1534709de2406c7a63fa9cd..9e623207513718970dc1af82aa8756144a771819 100644
> --- a/drivers/i2c/busses/i2c-rtl9300.c
> +++ b/drivers/i2c/busses/i2c-rtl9300.c
> @@ -186,22 +186,32 @@ static int rtl9300_i2c_execute_xfer(struct rtl9300_i2c *i2c, char read_write,
> return -EIO;
>
> if (read_write == I2C_SMBUS_READ) {
> - if (size == I2C_SMBUS_BYTE || size == I2C_SMBUS_BYTE_DATA) {
> + switch (size) {
> + case I2C_SMBUS_BYTE:
> + case I2C_SMBUS_BYTE_DATA:
> ret = regmap_read(i2c->regmap,
> i2c->reg_base + RTL9300_I2C_MST_DATA_WORD0, &val);
> if (ret)
> return ret;
> data->byte = val & 0xff;
> - } else if (size == I2C_SMBUS_WORD_DATA) {
> + break;
> + case I2C_SMBUS_WORD_DATA:
> ret = regmap_read(i2c->regmap,
> i2c->reg_base + RTL9300_I2C_MST_DATA_WORD0, &val);
> if (ret)
> return ret;
> data->word = val & 0xffff;
> - } else {
> + break;
> + case I2C_SMBUS_I2C_BLOCK_DATA:
> + ret = rtl9300_i2c_read(i2c, &data->block[1], len);
> + if (ret)
> + return ret;
> + break;
> + default:
> ret = rtl9300_i2c_read(i2c, &data->block[0], len);
> if (ret)
> return ret;
> + break;
> }
> }
>
> @@ -290,6 +300,25 @@ static int rtl9300_i2c_smbus_xfer(struct i2c_adapter *adap, u16 addr, unsigned s
> len = data->block[0] + 1;
> break;
>
> + case I2C_SMBUS_I2C_BLOCK_DATA:
> + ret = rtl9300_i2c_reg_addr_set(i2c, command, 1);
> + if (ret)
> + goto out_unlock;
> + if (data->block[0] < 1 || data->block[0] > I2C_SMBUS_BLOCK_MAX) {
> + ret = -EINVAL;
> + goto out_unlock;
> + }
> + ret = rtl9300_i2c_config_xfer(i2c, chan, addr, data->block[0]);
> + if (ret)
> + goto out_unlock;
> + if (read_write == I2C_SMBUS_WRITE) {
> + ret = rtl9300_i2c_write(i2c, &data->block[1], data->block[0]);
> + if (ret)
> + goto out_unlock;
> + }
> + len = data->block[0];
> + break;
> +
> default:
> dev_err(&adap->dev, "Unsupported transaction %d\n", size);
> ret = -EOPNOTSUPP;
>
> ---
> base-commit: 217f92d91c9faeb6b78bd6205b3585944cbcb433
> change-id: 20250802-i2c-rtl9300-multi-byte-edaa1fb0872c
>
> Best regards,
Thanks for taking care of this quickly.
Maybe we should include another patch here which fixes the committed version
of my patch, i.e. removing I2C_FUNC_SMBUS_I2C_BLOCK, with CC to stable. Since
the patch was also merged to stable, it is somewhat broken there now.
Best,
Jonas Jelonek
Powered by blists - more mailing lists