netdev - Re: [PATCH net-next v4] net: sfp: add SMBus I2C block 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: <a04ac2eb-1bf8-428a-8b77-639d01bfd5c4@gmail.com>
Date: Fri, 9 Jan 2026 17:46:21 +0100
From: Jonas Jelonek <jelonek.jonas@...il.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>,
 "David S . Miller" <davem@...emloft.net>, Eric Dumazet
 <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, Bjørn Mork <bjorn@...k.no>,
 Maxime Chevallier <maxime.chevallier@...tlin.com>
Subject: Re: [PATCH net-next v4] net: sfp: add SMBus I2C block support

Hi Russell,

On 09.01.26 17:21, Russell King (Oracle) wrote:
> On Fri, Jan 09, 2026 at 12:13:06PM +0000, Russell King (Oracle) wrote:
>> On Fri, Jan 09, 2026 at 10:13:21AM +0000, Jonas Jelonek wrote:
>>> Commit 7662abf4db94 ("net: phy: sfp: Add support for SMBus module access")
>>> added support for SMBus-only controllers for module access. However,
>>> this is restricted to single-byte accesses and has the implication that
>>> hwmon is disabled (due to missing atomicity of 16-bit accesses) and
>>> warnings are printed.
>>>
>>> There are probably a lot of SMBus-only I2C controllers out in the wild
>>> which support block reads. Right now, they don't work with SFP modules.
>>> This applies - amongst others - to I2C/SMBus-only controllers in Realtek
>>> longan and mango SoCs.
>>>
>>> Downstream in OpenWrt, a patch similar to the abovementioned patch is
>>> used for current LTS kernel 6.12. However, this uses byte-access for all
>>> kinds of access and thus disregards the atomicity for wider access.
>>>
>>> Introduce read/write SMBus I2C block operations to support SMBus-only
>>> controllers with appropriate support for block read/write. Those
>>> operations are used for all accesses if supported, otherwise the
>>> single-byte operations will be used. With block reads, atomicity for
>>> 16-bit reads as required by hwmon is preserved and thus, hwmon can be
>>> used.
>>>
>>> The implementation requires the I2C_FUNC_SMBUS_I2C_BLOCK to be
>>> supported as it relies on reading a pre-defined amount of bytes.
>>> This isn't intended by the official SMBus Block Read but supported by
>>> several I2C controllers/drivers.
>>>
>>> Support for word access is not implemented due to issues regarding
>>> endianness.
>> I'm wondering whether we should go further with this - we implement
>> byte mode SMBus support, but there is also word mode, too, which
>> would solve the HWMON issues. It looks like more SMBus devices support
>> word mode than I2C block mode.
>>
>> So, if we're seeing more SMBus adapters being used with SFPs, maybe
>> we should be thinking about a more adaptive approach to SMBus, where
>> we try to do the best with the features that the SMBus adapter
>> provides us.

Makes totally sense, my initial version was just a pragmatic attempt to solve
the issues downstream for a single target. But covering byte, word and I2C block
is a better approach.

>> Maybe something like:
>>
>> static int sfp_smbus_write(struct sfp *sfp, bool a2, u8 dev_addr, void *buf,
>>                            size_t len)
>> {
>> 	size_t this_len, transferred, total;
>> 	union i2c_smbus_data smbus_data;
>> 	u8 bus_addr = a2 ? 0x51 : 0x50;
>> 	u32 functionality;
>> 	int ret;
>>
>> 	functioality = i2c_get_functionality(sfp->i2c);
>> 	total = len;
>>
>> 	while (len) {
>> 		if (len > sfp->i2c_max_block_size)
>> 			this_len = sfp->i2c_max_block_size;
>> 		else
>> 			this_len = len;
>>
>> 		if (this_len > 2 &&
>> 		    functionality & I2C_FUNC_SMBUS_READ_I2C_BLOCK) {
>> 			.. use smbus i2c block mode ..
>> 			transferred = this_len;
>> 		} else if (this_len >= 2 &&
>> 		           functionality & I2C_FUNC_SMBUS_READ_WORD_DATA) {
>> 			.. use smbus word mode ..
>> 			transferred = 2;
>> 		} else {
>> 			.. use smbus byte mode ..
>> 			transferred = 1;
>> 		}
>>
>> 		buf += transferred;
>> 		len -= transferred;
>> 	}
>>
>> 	return ret < 0 : ret : total - len;
>> }
>>
>> sfp_hwmon_probe() will do the right thing based upon i2c_block_size, so
>> where only byte mode is supported, we don't get hwmon support.

Thanks for providing this. I'll take that as a starting point and come up with
a better patch.

> I should also note that, when checking for the appropriate
> functionality in sfp_i2c_configure(), we need to be careful that
> we can read single bytes.
>
> In other words, if an adapter reports that it supports smbus word data
> access, we need it to also support smbus byte data access or smbus i2c
> block access.
>

Good that you mention it, haven't considered this yet.

Best,
Jonas