linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH i2c-host-fixes v4 0/5] i2c: rtl9300: Fix multi-byte I2C operations
@ 2025-08-09  6:40 Sven Eckelmann
  2025-08-09  6:40 ` [PATCH i2c-host-fixes v4 1/5] i2c: rtl9300: Fix out-of-bounds bug in rtl9300_i2c_smbus_xfer Sven Eckelmann
                   ` (4 more replies)
  0 siblings, 5 replies; 9+ messages in thread
From: Sven Eckelmann @ 2025-08-09  6:40 UTC (permalink / raw)
  To: Chris Packham, Andi Shyti
  Cc: linux-i2c, linux-kernel, Jonas Jelonek, Harshal Gohel,
	Simon Wunderlich, Sven Eckelmann, Alex Guo, stable

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 maker +
Ack) was mixed up on the wire. At first glance, it looked like an
endianness problem. But for transfers where the number of count + data
bytes was not divisible by 4, the last bytes were not looking like an
endianness problem because they 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 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
transmission in the least significant byte of the first register. And the
last byte (assuming it is a 16 byte transfer) is expected 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 format 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 a 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 problems 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.

I've picked up Alex Guo's patch [2] to reduce conflicts between pending
fixes.

[1] https://patchwork.ozlabs.org/project/linux-i2c/cover/20250727114800.3046-1-jelonek.jonas@gmail.com/
[2] https://lore.kernel.org/r/20250615235248.529019-1-alexguo1023@gmail.com

Signed-off-by: Sven Eckelmann <sven@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@vger.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

---
Alex Guo (1):
      [i2c-host-fixes] i2c: rtl9300: Fix out-of-bounds bug in rtl9300_i2c_smbus_xfer

Harshal Gohel (2):
      [i2c-host-fixes] i2c: rtl9300: Fix multi-byte I2C write
      [i2c-host] i2c: rtl9300: Implement I2C block read and write

Sven Eckelmann (2):
      [i2c-host-fixes] i2c: rtl9300: Increase timeout for transfer polling
      [i2c-host-fixes] i2c: rtl9300: Add missing count byte for SMBus Block Ops

 drivers/i2c/busses/i2c-rtl9300.c | 50 +++++++++++++++++++++++++++++++++-------
 1 file changed, 42 insertions(+), 8 deletions(-)
---
base-commit: 09eaa2a604ed1bfda7e6fb10488127ce8fdc8048
change-id: 20250802-i2c-rtl9300-multi-byte-edaa1fb0872c

Best regards,
-- 
Sven Eckelmann <sven@narfation.org>


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2025-08-10  5:59 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-08-09  6:40 [PATCH i2c-host-fixes v4 0/5] i2c: rtl9300: Fix multi-byte I2C operations Sven Eckelmann
2025-08-09  6:40 ` [PATCH i2c-host-fixes v4 1/5] i2c: rtl9300: Fix out-of-bounds bug in rtl9300_i2c_smbus_xfer Sven Eckelmann
2025-08-09 17:34   ` Wolfram Sang
2025-08-09  6:40 ` [PATCH i2c-host-fixes v4 2/5] i2c: rtl9300: Fix multi-byte I2C write Sven Eckelmann
2025-08-09  6:40 ` [PATCH i2c-host-fixes v4 3/5] i2c: rtl9300: Increase timeout for transfer polling Sven Eckelmann
2025-08-09  6:40 ` [PATCH i2c-host-fixes v4 4/5] i2c: rtl9300: Add missing count byte for SMBus Block Ops Sven Eckelmann
2025-08-09  6:40 ` [PATCH i2c-host v4 5/5] i2c: rtl9300: Implement I2C block read and write Sven Eckelmann
2025-08-09 22:11   ` Jonas Jelonek
2025-08-10  5:59     ` Sven Eckelmann

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).