[PATCH v5 06/11] i2c: rtl9300: remove SMBus Quick operation support

Jonas Jelonek posted 11 patches 1 month, 3 weeks ago
[PATCH v5 06/11] i2c: rtl9300: remove SMBus Quick operation support
Posted by Jonas Jelonek 1 month, 3 weeks ago
Remove the SMBus Quick operation from this driver because it is not
natively supported by the hardware and is wrongly implemented in the
driver.

The I2C controllers in Realtek RTL9300 and RTL9310 are SMBus-compliant
but there doesn't seem to be native support for the SMBus Quick
operation. It is not explicitly mentioned in the documentation but
looking at the registers which configure an SMBus transaction, one can
see that the data length cannot be set to 0. This suggests that the
hardware doesn't allow any SMBus message without data bytes (except for
those it does on it's own, see SMBus Block Read).

The current implementation of SMBus Quick operation passes a length of
0 (which is actually invalid). Before the fix of a bug in a previous
commit, this led to a read operation of 16 bytes from any register (the
one of a former transaction or any other value.

Although there are currently no reports of actual issues this caused.
However, as an example, i2cdetect by default uses Quick Write operation
to probe the bus and this may already write anything to some register
of a device, causing unintended behaviour. This could be the cause of a
recent brick of one of my DAC cables where there was a checksum mismatch
of the EEPROM after having run 'i2cdetect -l' before.

Because SMBus Quick operation is obviously not supported on these
controllers (because a length of 0 cannot be set, even when no register
address is set), remove that instead of claiming there is support. There
also shouldn't be any kind of emulated 'Quick' which just does another
kind of operation in the background. Otherwise, specific issues may
occur in case of a 'Quick' Write which writes unknown data to an unknown
register.

Signed-off-by: Jonas Jelonek <jelonek.jonas@gmail.com>
---
 drivers/i2c/busses/i2c-rtl9300.c | 16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rtl9300.c b/drivers/i2c/busses/i2c-rtl9300.c
index 35b05fb59f88..ead5aa6e60f8 100644
--- a/drivers/i2c/busses/i2c-rtl9300.c
+++ b/drivers/i2c/busses/i2c-rtl9300.c
@@ -251,15 +251,6 @@ static int rtl9300_i2c_smbus_xfer(struct i2c_adapter *adap, u16 addr, unsigned s
 	}
 
 	switch (size) {
-	case I2C_SMBUS_QUICK:
-		ret = rtl9300_i2c_config_xfer(i2c, chan, addr, 0);
-		if (ret)
-			goto out_unlock;
-		ret = rtl9300_i2c_reg_addr_set(i2c, 0, 0);
-		if (ret)
-			goto out_unlock;
-		break;
-
 	case I2C_SMBUS_BYTE:
 		if (read_write == I2C_SMBUS_WRITE) {
 			ret = rtl9300_i2c_config_xfer(i2c, chan, addr, 0);
@@ -360,10 +351,9 @@ static int rtl9300_i2c_smbus_xfer(struct i2c_adapter *adap, u16 addr, unsigned s
 
 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_READ_I2C_BLOCK | I2C_FUNC_SMBUS_WRITE_I2C_BLOCK |
-	       I2C_FUNC_SMBUS_BLOCK_DATA;
+	return I2C_FUNC_SMBUS_BYTE | I2C_FUNC_SMBUS_BYTE_DATA |
+	       I2C_FUNC_SMBUS_WORD_DATA | I2C_FUNC_SMBUS_READ_I2C_BLOCK |
+	       I2C_FUNC_SMBUS_WRITE_I2C_BLOCK | I2C_FUNC_SMBUS_BLOCK_DATA;
 }
 
 static const struct i2c_algorithm rtl9300_i2c_algo = {
-- 
2.48.1
Re: [PATCH v5 06/11] i2c: rtl9300: remove SMBus Quick operation support
Posted by Sven Eckelmann 1 month, 3 weeks ago
On Sunday, 10 August 2025 00:07:07 CEST Jonas Jelonek wrote:
[...]
> The current implementation of SMBus Quick operation passes a length of
> 0 (which is actually invalid). Before the fix of a bug in a previous
> commit, this led to a read operation of 16 bytes from any register (the
> one of a former transaction or any other value.
> 
> Although there are currently no reports of actual issues this caused.
> However, as an example, i2cdetect by default uses Quick Write operation
> to probe the bus and this may already write anything to some register
> of a device, causing unintended behaviour. This could be the cause of a
> recent brick of one of my DAC cables where there was a checksum mismatch
> of the EEPROM after having run 'i2cdetect -l' before.
[...]

Nice find. I've actually observed odd behavior after/during probing and 
attributed it only to the other problems (especially the low timeout + missing 
check) we found and never did a deep dive to figure out what happened on the 
bus during the probe. Possible that this could be related.

Reviewed-by: Sven Eckelmann <sven@narfation.org>

Kind regards,
	Sven
Re: [PATCH v5 06/11] i2c: rtl9300: remove SMBus Quick operation support
Posted by Jonas Jelonek 1 month, 3 weeks ago

On 10.08.2025 09:13, Sven Eckelmann wrote:
> On Sunday, 10 August 2025 00:07:07 CEST Jonas Jelonek wrote:
> [...]
>> The current implementation of SMBus Quick operation passes a length of
>> 0 (which is actually invalid). Before the fix of a bug in a previous
>> commit, this led to a read operation of 16 bytes from any register (the
>> one of a former transaction or any other value.
>>
>> Although there are currently no reports of actual issues this caused.
>> However, as an example, i2cdetect by default uses Quick Write operation
>> to probe the bus and this may already write anything to some register
>> of a device, causing unintended behaviour. This could be the cause of a
>> recent brick of one of my DAC cables where there was a checksum mismatch
>> of the EEPROM after having run 'i2cdetect -l' before.
> [...]
>
> Nice find. I've actually observed odd behavior after/during probing and 
> attributed it only to the other problems (especially the low timeout + missing 
> check) we found and never did a deep dive to figure out what happened on the 
> bus during the probe. Possible that this could be related.

Haven't actually described my issue in detail in the commit message (may add
this in the next version) but it perfectly makes sense. Quick operation in the
driver passed a length of 0 to config_xfer. Internally, this leads to a value of
0xf in the DATA_WIDTH register meaning 16 bytes to be read/written because
of:

(len - 1) & 0xf

The register value obviously was assumed to be 0 by the hardware and data
was completely zeroed too. Then it did a 16-byte write of that. Unfortunately,
the EEPROM of my DAC isn't write-protected so it was written to the EEPROM
causing checksum error on next boot (was able to fix that though).

> Reviewed-by: Sven Eckelmann <sven@narfation.org>

Thanks!

> Kind regards,
> 	Sven

Best,
Jonas