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:

quoted

On Fri, Jan 09, 2026 at 10:13:21AM +0000, Jonas Jelonek wrote:

quoted

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.

quoted

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