linux-kernel - Re: [PATCH v7 03/12] i2c: rtl9300: remove broken SMBus Quick operation support

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: <cfdleondrrpfyfts423cwdcsb5mmqovej5hwke7ndghzlnwci7@d6i7ltgoxbee>
Date: Thu, 4 Sep 2025 00:59:20 +0200
From: Andi Shyti <andi.shyti@...nel.org>
To: Jonas Jelonek <jelonek.jonas@...il.com>
Cc: Chris Packham <chris.packham@...iedtelesis.co.nz>, 
	Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, 
	linux-i2c@...r.kernel.org, Conor Dooley <conor+dt@...nel.org>, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Markus Stockhausen <markus.stockhausen@....de>, 
	Sven Eckelmann <sven@...fation.org>, Harshal Gohel <hg@...onwunderlich.de>, 
	Wolfram Sang <wsa+renesas@...g-engineering.com>, stable@...r.kernel.org
Subject: Re: [PATCH v7 03/12] i2c: rtl9300: remove broken SMBus Quick
 operation support

Hi Jonas,

On Sun, Aug 31, 2025 at 10:04:48AM +0000, Jonas Jelonek wrote:
> 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.
> 
> This caused issues like soft-bricked SFP modules after a simple probe
> with i2cdetect which uses Quick by default. Running this with SFP
> modules whose EEPROM isn't write-protected, some of the initial bytes
> are overwritten because a 16-byte write operation is executed instead of
> a Quick Write. (This temporarily soft-bricked one of my DAC cables.)
> 
> 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 occur
> in case of a 'Quick' Write which actually writes unknown data to an
> unknown register.
> 
> Fixes: c366be720235 ("i2c: Add driver for the RTL9300 I2C controller")
> Cc: <stable@...r.kernel.org> # v6.13+
> Signed-off-by: Jonas Jelonek <jelonek.jonas@...il.com>
> Tested-by: Sven Eckelmann <sven@...fation.org>
> Reviewed-by: Chris Packham <chris.packham@...iedtelesis.co.nz>
> Tested-by: Chris Packham <chris.packham@...iedtelesis.co.nz> # On RTL9302C based board
> Tested-by: Markus Stockhausen <markus.stockhausen@....de>

Applied from 1-3 to i2c/i2c-host-fixes.

But...

> ---
>  drivers/i2c/busses/i2c-rtl9300.c | 15 +++------------
>  1 file changed, 3 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-rtl9300.c b/drivers/i2c/busses/i2c-rtl9300.c
> index ebd4a85e1bde..9e6232075137 100644
> --- a/drivers/i2c/busses/i2c-rtl9300.c
> +++ b/drivers/i2c/busses/i2c-rtl9300.c
> @@ -235,15 +235,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);
> @@ -344,9 +335,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_BLOCK_DATA | I2C_FUNC_SMBUS_I2C_BLOCK;
> +	return I2C_FUNC_SMBUS_BYTE | I2C_FUNC_SMBUS_BYTE_DATA |
> +	       I2C_FUNC_SMBUS_WORD_DATA | I2C_FUNC_SMBUS_BLOCK_DATA |
> +	       I2C_FUNC_SMBUS_I2C_BLOCK;

this was creating a conflict with:

5090e2b3808e ("i2c: rtl9300: Implement I2C block read and write")

In the sense that I don't have this change in the fixes path, but
I have it in the non-fixes. For now, until Wolfram pulls the
fixes, I removed the patch and I will add it back next week to
avoid conflicts in the -next branch.

Next week I will apply the rest of the patches in the series, as
well.

Thanks,
Andi

>  }
>  
>  static const struct i2c_algorithm rtl9300_i2c_algo = {
> -- 
> 2.48.1
>