linux-kernel - Re: [PATCH v2 0/4] i2c: rtl9300: Fix multi-byte I2C operations

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4152a424-13ea-4437-b9e9-f1b5561cca9e@alliedtelesis.co.nz>
Date: Sun, 3 Aug 2025 22:39:40 +0000
From: Chris Packham <Chris.Packham@...iedtelesis.co.nz>
To: Sven Eckelmann <sven@...fation.org>, Andi Shyti <andi.shyti@...nel.org>
CC: "linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, Jonas Jelonek
	<jelonek.jonas@...il.com>, Harshal Gohel <hg@...onwunderlich.de>, "Simon
 Wunderlich" <sw@...onwunderlich.de>, "stable@...r.kernel.org"
	<stable@...r.kernel.org>
Subject: Re: [PATCH v2 0/4] i2c: rtl9300: Fix multi-byte I2C operations

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=npSP6NDPXTaQTClSdFmD6RMng5fg4WMz7KSCx5tgNw&u=https%3a%2f%2fpatchwork%2eozlabs%2eorg%2fproject%2flinux-i2c%2fcover%2f20250727114800%2e3046-1-jelonek%2ejonas%40gmail%2ecom%2f
>
> Signed-off-by: Sven Eckelmann <sven@...fation.org>

For the series

Reviewed-by: Chris Packham <chris.packham@...iedtelesis.co.nz>
Tested-by: Chris Packham <chris.packham@...iedtelesis.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=npSP6NDPXTaQTClSdFmD6RMng5fg4WMz7KHQxMVsYg&u=https%3a%2f%2flore%2ekernel%2eorg%2fr%2f20250802-i2c-rtl9300-multi-byte-v1-0-5f687e0098e2%40narfation%2eorg
>
> ---
> 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,