During the integration of the RTL8239 POE chip + its frontend MCU, it was noticed that multi-byte operations were basically broken in the current driver.
Tests using SMBus Block Writes showed that the data (after the Wr + Ack marker) was mixed up on the wire. At first glance, it looked like an endianness problem. But for transfers were the number of count + data bytes was not divisible by 4, the last bytes were not looking like an endianness problem because they were were in the wrong order but not for example 0 - which would be the case for an endianness problem with 32 bit registers. At the end, it turned out to be a the way how i2c_write tried to add the bytes to the send registers.
Each 32 bit register was used similar to a shift register - shifting the various bytes up the register while the next one is added to the least significant byte. But the I2C controller expects the first byte of the tranmission in the least significant byte of the first register. And the last byte (assuming it is a 16 byte transfer) in the most significant byte of the fourth register.
While doing these tests, it was also observed that the count byte was missing from the SMBus Block Writes. The driver just removed them from the data->block (from the I2C subsystem). But the I2C controller DOES NOT automatically add this byte - for example by using the configured transmission length.
The RTL8239 MCU is not actually an SMBus compliant device. Instead, it expects I2C Block Reads + I2C Block Writes. But according to the already identified bugs in the driver, it was clear that the I2C controller can simply be modified to not send the count byte for I2C_SMBUS_I2C_BLOCK_DATA. The receive part, just needs to write the content of the receive buffer to the correct position in data->block.
While the on-wire formwat was now correct, reads were still not possible against the MCU (for the RTL8239 POE chip). It was always timing out because the 2ms were not enough for sending the read request and then receiving the 12 byte answer.
These changes were originally submitted to OpenWrt. But there are plans to migrate OpenWrt to the upstream Linux driver. As result, the pull request was stopped and the changes were redone against this driver.
For reasons of transparency: The work on I2C_SMBUS_I2C_BLOCK_DATA support for the RTL8239-MCU was done on RTL931xx. All problem were therefore detected with the patches from Jonas Jelonek [1] and not the vanilla Linux driver. But looking through the code, it seems like these are NOT regressions introduced by the RTL931x patchset.
[1] https://patchwork.ozlabs.org/project/linux-i2c/cover/20250727114800.3046-1-j...
Signed-off-by: Sven Eckelmann sven@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@...
--- Harshal Gohel (2): i2c: rtl9300: Fix multi-byte I2C write i2c: rtl9300: Implement I2C block read and write
Sven Eckelmann (2): i2c: rtl9300: Increase timeout for transfer polling i2c: rtl9300: Add missing count byte for SMBus Block Ops
drivers/i2c/busses/i2c-rtl9300.c | 43 +++++++++++++++++++++++++++++++++------- 1 file changed, 36 insertions(+), 7 deletions(-) --- base-commit: b9ddaa95fd283bce7041550ddbbe7e764c477110 change-id: 20250802-i2c-rtl9300-multi-byte-edaa1fb0872c
Best regards,
From: Harshal Gohel hg@simonwunderlich.de
The RTL93xx I2C controller has 4 32 bit registers to store the bytes for the upcoming I2C transmission. The first byte is stored in the least-significant byte of the first register. And the last byte in the most significant byte of the last register. A map of the transferred bytes to their order in the registers is:
reg 0: 0x04_03_02_01 reg 1: 0x08_07_06_05 reg 2: 0x0c_0b_0a_09 reg 3: 0x10_0f_0e_0d
The i2c_read() function basically demonstrates how the hardware would pick up bytes from this register set. But the i2c_write() function was just pushing bytes one after another to the least significant byte of a register AFTER shifting the last one to the next more significant byte position.
If you would then have tried to send a buffer with numbers 1-11 using i2c_write(), you would have ended up with following register content:
reg 0: 0x01_02_03_04 reg 1: 0x05_06_07_08 reg 2: 0x00_09_0a_0b reg 3: 0x00_00_00_00
On the wire, you would then have seen:
Sr Addr Rd/Wr [A] 04 A 03 A 02 A 01 A 08 A 07 A 06 A 05 A 0b A 0a A 09 A/NA P
But the correct data transmission was expected to be
Sr Addr Rd/Wr [A] 01 A 02 A 03 A 04 A 05 A 06 A 07 A 08 A 09 A 0a A 0b A/NA P
Because of this multi-byte ordering problem, only single byte i2c_write() operations were executed correctly (on the wire).
By shifting the byte directly to the correct end position in the register, it is possible to avoid this incorrect byte ordering and fix multi-byte transmissions.
Cc: stable@vger.kernel.org Fixes: c366be720235 ("i2c: Add driver for the RTL9300 I2C controller") Signed-off-by: Harshal Gohel hg@simonwunderlich.de Co-developed-by: Sven Eckelmann sven@narfation.org Signed-off-by: Sven Eckelmann sven@narfation.org --- drivers/i2c/busses/i2c-rtl9300.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/i2c/busses/i2c-rtl9300.c b/drivers/i2c/busses/i2c-rtl9300.c index e064e8a4a1f0824abc82fa677866b85f99fbe4a7..1b3cbe3ea84a4fa480c5c00438eecc551d047348 100644 --- a/drivers/i2c/busses/i2c-rtl9300.c +++ b/drivers/i2c/busses/i2c-rtl9300.c @@ -143,10 +143,13 @@ static int rtl9300_i2c_write(struct rtl9300_i2c *i2c, u8 *buf, int len) return -EIO;
for (i = 0; i < len; i++) { + unsigned int shift = (i % 4) * 8; + unsigned int reg = i / 4; + if (i % 4 == 0) - vals[i/4] = 0; - vals[i/4] <<= 8; - vals[i/4] |= buf[i]; + vals[reg] = 0; + + vals[reg] |= buf[i] << shift; }
return regmap_bulk_write(i2c->regmap, i2c->reg_base + RTL9300_I2C_MST_DATA_WORD0,
The timeout for transfers was only set to 2ms. Because of this relatively low limit, 12-byte read operations to the frontend MCU of a RTL8239 POE PSE chip cluster was consistently resulting in a timeout.
The original OpenWrt downstream driver [1] was not using any timeout limit at all. This is also possible by setting the timeout_us parameter of regmap_read_poll_timeout() to 0. But since the driver currently is implements the ETIMEDOUT error, it is more sensible to increase the timeout in such a way that the communication with the (quite common) Realtek I2C-connected POE management solution is possible.
[1] https://git.openwrt.org/?p=openwrt/openwrt.git%3Ba=blob%3Bf=target/linux/rea...
Cc: stable@vger.kernel.org Fixes: c366be720235 ("i2c: Add driver for the RTL9300 I2C controller") Signed-off-by: Sven Eckelmann sven@narfation.org --- drivers/i2c/busses/i2c-rtl9300.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-rtl9300.c b/drivers/i2c/busses/i2c-rtl9300.c index 1b3cbe3ea84a4fa480c5c00438eecc551d047348..a10e5e6e00075fabb8906d56f09f5b9141fbc06e 100644 --- a/drivers/i2c/busses/i2c-rtl9300.c +++ b/drivers/i2c/busses/i2c-rtl9300.c @@ -178,7 +178,7 @@ static int rtl9300_i2c_execute_xfer(struct rtl9300_i2c *i2c, char read_write, return ret;
ret = regmap_read_poll_timeout(i2c->regmap, i2c->reg_base + RTL9300_I2C_MST_CTRL1, - val, !(val & RTL9300_I2C_MST_CTRL1_I2C_TRIG), 100, 2000); + val, !(val & RTL9300_I2C_MST_CTRL1_I2C_TRIG), 100, 100000); if (ret) return ret;
The expected on-wire format of an SMBus Block Write is
S Addr Wr [A] Comm [A] Count [A] Data [A] Data [A] ... [A] Data [A] P
Everything starting from the Count byte is provided by the I2C subsystem in the array data->block. But the driver was skipping the Count byte (data->block[0]) when sending it to the RTL93xx I2C controller.
Only the actual data could be seen on the wire:
S Addr Wr [A] Comm [A] Data [A] Data [A] ... [A] Data [A] P
This wire format is not SMBus Block Write compatible but matches the format of an I2C Block Write. Simply adding the count byte to the buffer for the I2C controller enough to fix the transmission.
This also affects read because the I2C controller must receive the count byte + $count * data bytes.
Cc: stable@vger.kernel.org Fixes: c366be720235 ("i2c: Add driver for the RTL9300 I2C controller") Signed-off-by: Sven Eckelmann sven@narfation.org --- drivers/i2c/busses/i2c-rtl9300.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/i2c/busses/i2c-rtl9300.c b/drivers/i2c/busses/i2c-rtl9300.c index a10e5e6e00075fabb8906d56f09f5b9141fbc06e..4e0844d97607f64386a6d7a7c4086a81fdd89d6c 100644 --- a/drivers/i2c/busses/i2c-rtl9300.c +++ b/drivers/i2c/busses/i2c-rtl9300.c @@ -284,15 +284,15 @@ static int rtl9300_i2c_smbus_xfer(struct i2c_adapter *adap, u16 addr, unsigned s ret = rtl9300_i2c_reg_addr_set(i2c, command, 1); if (ret) goto out_unlock; - ret = rtl9300_i2c_config_xfer(i2c, chan, addr, data->block[0]); + ret = rtl9300_i2c_config_xfer(i2c, chan, addr, data->block[0] + 1); if (ret) goto out_unlock; if (read_write == I2C_SMBUS_WRITE) { - ret = rtl9300_i2c_write(i2c, &data->block[1], data->block[0]); + ret = rtl9300_i2c_write(i2c, &data->block[0], data->block[0] + 1); if (ret) goto out_unlock; } - len = data->block[0]; + len = data->block[0] + 1; break;
default:
Hi Sven, Andi,
On 04/08/2025 04:54, Sven Eckelmann wrote:
The expected on-wire format of an SMBus Block Write is
S Addr Wr [A] Comm [A] Count [A] Data [A] Data [A] ... [A] Data [A] P
Everything starting from the Count byte is provided by the I2C subsystem in the array data->block. But the driver was skipping the Count byte (data->block[0]) when sending it to the RTL93xx I2C controller.
Only the actual data could be seen on the wire:
S Addr Wr [A] Comm [A] Data [A] Data [A] ... [A] Data [A] P
This wire format is not SMBus Block Write compatible but matches the format of an I2C Block Write. Simply adding the count byte to the buffer for the I2C controller enough to fix the transmission.
This also affects read because the I2C controller must receive the count byte + $count * data bytes.
Cc: stable@vger.kernel.org Fixes: c366be720235 ("i2c: Add driver for the RTL9300 I2C controller") Signed-off-by: Sven Eckelmann sven@narfation.org
This conflicts with commit f46867c08a22 ("i2c: rtl9300: Fix out-of-bounds bug in rtl9300_i2c_smbus_xfer") which didn't get a Fixes tag. The merge conflict is reasonably straight forward to resolve. Not sure how you want to handle the patch for stable.
drivers/i2c/busses/i2c-rtl9300.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/i2c/busses/i2c-rtl9300.c b/drivers/i2c/busses/i2c-rtl9300.c index a10e5e6e00075fabb8906d56f09f5b9141fbc06e..4e0844d97607f64386a6d7a7c4086a81fdd89d6c 100644 --- a/drivers/i2c/busses/i2c-rtl9300.c +++ b/drivers/i2c/busses/i2c-rtl9300.c @@ -284,15 +284,15 @@ static int rtl9300_i2c_smbus_xfer(struct i2c_adapter *adap, u16 addr, unsigned s ret = rtl9300_i2c_reg_addr_set(i2c, command, 1); if (ret) goto out_unlock;
ret = rtl9300_i2c_config_xfer(i2c, chan, addr, data->block[0]);
ret = rtl9300_i2c_config_xfer(i2c, chan, addr, data->block[0] + 1);
ret = rtl9300_i2c_write(i2c, &data->block[1], data->block[0]);
ret = rtl9300_i2c_write(i2c, &data->block[0], data->block[0] + 1); if (ret) goto out_unlock;
len = data->block[0];
len = data->block[0] + 1;default:
Sorry for the noise. I did test v2 but replied to v1.
On 04/08/2025 04:54, Sven Eckelmann wrote:
During the integration of the RTL8239 POE chip + its frontend MCU, it was noticed that multi-byte operations were basically broken in the current driver.
Tests using SMBus Block Writes showed that the data (after the Wr + Ack marker) was mixed up on the wire. At first glance, it looked like an endianness problem. But for transfers were the number of count + data bytes was not divisible by 4, the last bytes were not looking like an endianness problem because they were were in the wrong order but not for example 0 - which would be the case for an endianness problem with 32 bit registers. At the end, it turned out to be a the way how i2c_write tried to add the bytes to the send registers.
Each 32 bit register was used similar to a shift register - shifting the various bytes up the register while the next one is added to the least significant byte. But the I2C controller expects the first byte of the tranmission in the least significant byte of the first register. And the last byte (assuming it is a 16 byte transfer) in the most significant byte of the fourth register.
While doing these tests, it was also observed that the count byte was missing from the SMBus Block Writes. The driver just removed them from the data->block (from the I2C subsystem). But the I2C controller DOES NOT automatically add this byte - for example by using the configured transmission length.
The RTL8239 MCU is not actually an SMBus compliant device. Instead, it expects I2C Block Reads + I2C Block Writes. But according to the already identified bugs in the driver, it was clear that the I2C controller can simply be modified to not send the count byte for I2C_SMBUS_I2C_BLOCK_DATA. The receive part, just needs to write the content of the receive buffer to the correct position in data->block.
While the on-wire formwat was now correct, reads were still not possible against the MCU (for the RTL8239 POE chip). It was always timing out because the 2ms were not enough for sending the read request and then receiving the 12 byte answer.
These changes were originally submitted to OpenWrt. But there are plans to migrate OpenWrt to the upstream Linux driver. As result, the pull request was stopped and the changes were redone against this driver.
For reasons of transparency: The work on I2C_SMBUS_I2C_BLOCK_DATA support for the RTL8239-MCU was done on RTL931xx. All problem were therefore detected with the patches from Jonas Jelonek [1] and not the vanilla Linux driver. But looking through the code, it seems like these are NOT regressions introduced by the RTL931x patchset.
[1] https://scanmail.trustwave.com/?c=20988&d=npSP6NDPXTaQTClSdFmD6RMng5fg4W...
Signed-off-by: Sven Eckelmann sven@narfation.org
For the series
Reviewed-by: Chris Packham chris.packham@alliedtelesis.co.nz Tested-by: Chris Packham chris.packham@alliedtelesis.co.nz
Note that I've only got the same simple eeprom devices that I did the initial development on so I don't think I've really exercised the block data paths but I can say the changes don't appear to have regressed anything.
Changes in v2:
- add the missing transfer width and read length increase for the SMBus Write/Read
- Link to v1: https://scanmail.trustwave.com/?c=20988&d=npSP6NDPXTaQTClSdFmD6RMng5fg4W...
Harshal Gohel (2): i2c: rtl9300: Fix multi-byte I2C write i2c: rtl9300: Implement I2C block read and write
Sven Eckelmann (2): i2c: rtl9300: Increase timeout for transfer polling i2c: rtl9300: Add missing count byte for SMBus Block Ops
drivers/i2c/busses/i2c-rtl9300.c | 43 +++++++++++++++++++++++++++++++++------- 1 file changed, 36 insertions(+), 7 deletions(-)
base-commit: b9ddaa95fd283bce7041550ddbbe7e764c477110 change-id: 20250802-i2c-rtl9300-multi-byte-edaa1fb0872c
Best regards,
On Monday, 4 August 2025 00:39:40 CEST Chris Packham wrote:
For the series
Reviewed-by: Chris Packham chris.packham@alliedtelesis.co.nz Tested-by: Chris Packham chris.packham@alliedtelesis.co.nz
Thank you.
Note that I've only got the same simple eeprom devices that I did the initial development on so I don't think I've really exercised the block data paths but I can say the changes don't appear to have regressed anything.
I can understand this problem quite well. We can all only try our best and then hope that someone with the actual HW can figure out the specific parts which we didn't had access to.
Is you series intended to apply on top of Jonas's? I'm trying to apply yours alone (for various reasons happens to be on top of net-next/main) and I'm getting conflicts.
No, I prepare something for downstream testing (with Jonas' patch): https://github.com/openwrt/openwrt/pull/19577#discussion_r2248520949
Conflict appears to be with https://lore.kernel.org/all/20250615235248.529019-1-alexguo1023@gmail.com/
Thanks, I was not aware of this specific one. I don't exactly know the repo structure for I2C Host drivers. But git://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git i2c/i2c-host-fixes or i2c/i2c-host-next didn't had this patch. I've also checked linux-next and couldn't find the patch at the moment.
I am guessing it is the best when I resent this patch as part of my patchset and modify my patches accordingly. The resent will then be done this evening (GMT+2). Preview can be found at https://git.open-mesh.org/linux-merge.git/log/?h=b4/i2c-rtl9300-multi-byte
I've also checked linux-next and couldn't find the patch at the moment.
Kind regards, Sven
On 04/08/2025 20:35, Sven Eckelmann wrote:
On Monday, 4 August 2025 00:39:40 CEST Chris Packham wrote:
For the series
Reviewed-by: Chris Packham chris.packham@alliedtelesis.co.nz Tested-by: Chris Packham chris.packham@alliedtelesis.co.nz
Thank you.
Note that I've only got the same simple eeprom devices that I did the initial development on so I don't think I've really exercised the block data paths but I can say the changes don't appear to have regressed anything.
I can understand this problem quite well. We can all only try our best and then hope that someone with the actual HW can figure out the specific parts which we didn't had access to.
Is you series intended to apply on top of Jonas's? I'm trying to apply yours alone (for various reasons happens to be on top of net-next/main) and I'm getting conflicts.
No, I prepare something for downstream testing (with Jonas' patch): https://scanmail.trustwave.com/?c=20988&d=4PCQ6BHJEPBu-7BQIQUZquRTjfxQxI...
Conflict appears to be with https://scanmail.trustwave.com/?c=20988&d=4PCQ6BHJEPBu-7BQIQUZquRTjfxQxI...
Thanks, I was not aware of this specific one. I don't exactly know the repo structure for I2C Host drivers. But git://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git i2c/i2c-host-fixes or i2c/i2c-host-next didn't had this patch. I've also checked linux-next and couldn't find the patch at the moment.
I am guessing it is the best when I resent this patch as part of my patchset and modify my patches accordingly. The resent will then be done this evening (GMT+2). Preview can be found at https://scanmail.trustwave.com/?c=20988&d=4PCQ6BHJEPBu-7BQIQUZquRTjfxQxI...
I've also checked linux-next and couldn't find the patch at the moment.
Oops my bad. I'd applied Alex's patch to test locally then added more stuff on top of it and promptly forgot. When I came to test your patches I just rebased on top of net-next which I incorrectly assumed had picked up Alex's change via linux-next hence reporting the conflict which you can't see. Glad you managed to figure it out on your end for v3.
Kind regards, Sven
linux-stable-mirror@lists.linaro.org
-
Chris Packham -
Sven Eckelmann