linux-kernel - Re: [PATCH 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: <50d3d886-8352-4c21-9243-d058f14d8c65@alliedtelesis.co.nz>
Date: Sun, 3 Aug 2025 22:16:41 +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 0/4] i2c: rtl9300: Fix multi-byte I2C operations


On 04/08/2025 10:12, Chris Packham wrote:
> Hi Sven,
>
> On 02/08/2025 22:31, 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.
>
> Thanks. I was only really able to test with basic eeprom like devices 
> so it's not entirely surprising that the SMBUS stuff was wrong.
>
> 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.

Conflict appears to be with 
https://lore.kernel.org/all/20250615235248.529019-1-alexguo1023@gmail.com/

>
> At a cursory glance your changes all look sensible I'll reply with a 
> proper r-by when I've been able to tests them.
>
>>
>> Signed-off-by: Sven Eckelmann <sven@...fation.org>
>> ---
>> 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 Write
>>
>>   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,