From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtpout-04.galae.net (smtpout-04.galae.net [185.171.202.116]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id B73E517A309 for ; Thu, 7 May 2026 20:45:46 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=185.171.202.116 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1778186749; cv=none; b=ahPq5AC/eawLf2x1jE+BSBgD1WC1gJtggmp9dkIiLN6R4vTh86y+uN0sL1BdVStvlALk4RPypFWSXXxo3cNOOXU4J+F1DkNMO44djif1k43CNNBK9DwF0IsAwRKwYNnhQAkKdrWYcpIrumILZ3Vtuqq/9wnyIOAbo4mu9aVmsYM= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1778186749; c=relaxed/simple; bh=xsHpc7kTZjKR/CbWOpG7Z8ZZi+Rdk42MCFDwzPC/LBg=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=UEHtQEYKJcOgqUJQp1zAGaF9GqrYaubtkKKV/2pfJuQvpazd7GLcbNpJewSLQPSdQttblWUBzrD6bddMD7rqvxtI6tN4AwRn00FLFr6F2pSwd3Taeh98HAqCOPwGfeYFgd6HqfU8JY3AMXq3PyoUo1lOBjhH4UthUwxHOIy0Ea4= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=bootlin.com; spf=pass smtp.mailfrom=bootlin.com; dkim=pass (2048-bit key) header.d=bootlin.com header.i=@bootlin.com header.b=R72SXyVy; arc=none smtp.client-ip=185.171.202.116 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=bootlin.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=bootlin.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=bootlin.com header.i=@bootlin.com header.b="R72SXyVy" Received: from smtpout-01.galae.net (smtpout-01.galae.net [212.83.139.233]) by smtpout-04.galae.net (Postfix) with ESMTPS id 12C93C5DC67; Thu, 7 May 2026 20:46:33 +0000 (UTC) Received: from mail.galae.net (mail.galae.net [212.83.136.155]) by smtpout-01.galae.net (Postfix) with ESMTPS id 09505605CF; Thu, 7 May 2026 20:45:45 +0000 (UTC) Received: from [127.0.0.1] (localhost [127.0.0.1]) by localhost (Mailerdaemon) with ESMTPSA id 80C611081952F; Thu, 7 May 2026 22:45:38 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bootlin.com; s=dkim; t=1778186744; h=from:subject:date:message-id:to:cc:mime-version:content-type: content-transfer-encoding:content-language:in-reply-to:references; bh=Du0JaGK7Wa1gUYSYMCegKLL7ek+1tnRBdh+CwVTaO1U=; b=R72SXyVy+mHuWTLy+FkkFWfKBYokdjO2VSqy6qc1rj6yJilMD4ygJsQuJWwRx8xaJE5c1z uYIqq4cRz/f4fGuJWAEEeDVu9JqyXxt0h0m6rZ0zh5v6umb3bKtgPtOXLU0JemT3LfHxRg oRrvndQV7p75jQfJhM+fFu8uhNZdE8Yazkd+aTYsvZvSguGr10Vk6nvpBpRsI/9GKwycwA DEFb9nnRIfBnrdpBifx1zsqskNOg/vB/RE7xA8TDYgJvRVfM+HpYXjiXqlrfEhEEl3Ukh7 JXWB2uiGB4e2W9aEBqgPoyTt0gepeHf8zBP6v94kJBab8BWDm37dy8fTuDPPCw== Message-ID: Date: Thu, 7 May 2026 22:45:37 +0200 Precedence: bulk X-Mailing-List: netdev@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH net-next v7 2/2] net: sfp: extend SMBus support To: Jonas Jelonek , Russell King , Andrew Lunn , Heiner Kallweit , "David S . Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org, =?UTF-8?Q?Bj=C3=B8rn_Mork?= References: <20260507093301.1144740-1-jelonek.jonas@gmail.com> <20260507093301.1144740-3-jelonek.jonas@gmail.com> From: Maxime Chevallier Content-Language: en-US In-Reply-To: <20260507093301.1144740-3-jelonek.jonas@gmail.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Last-TLS-Session-Version: TLSv1.3 On 07/05/2026 11:33, Jonas Jelonek wrote: > Commit 7662abf4db94 ("net: phy: sfp: Add support for SMBus module access") > added SMBus access for SFP modules, but limited it to single-byte > transfers. As a side effect, hwmon is disabled (16-bit reads cannot be > guaranteed atomic) and a warning is printed. > > Many SMBus-only I2C controllers in the wild support more than just > byte access, and SFP cages are often wired to such controllers > rather than to a full-featured I2C controller -- e.g. the SMBus > controllers in the Realtek longan and mango SoCs, which advertise > word access and I2C block reads. Today, they cannot drive an SFP at > all without falling back to the byte-only path. > > Extend sfp_smbus_read()/sfp_smbus_write() so that, in addition to > the existing byte access, they also use SMBus word access and SMBus > I2C block access whenever the adapter advertises them. Both > directions are handled in a single read and a single write helper > that pick the largest supported transfer per chunk and fall back as > needed. > > I2C-block is preferred unconditionally when available: the protocol > carries any length 1..32, so it can serve every chunk -- including > the 1- and 2-byte tails -- without help from word or byte access. > Note that this requires I2C_FUNC_SMBUS_I2C_BLOCK, which reads a > caller-specified number of bytes. This deviates from the official > SMBus Block Read (length is supplied by the slave) but is widely > supported by Linux I2C controllers/drivers. > > Capability matrix this implementation supports: > > - BYTE only: works (unchanged behaviour); 1-byte > xfers, hwmon disabled. > - BYTE + WORD: word for >=2-byte chunks, byte for > trailing odd byte. > - I2C_BLOCK present (with or > without BYTE/WORD): block as the universal transport for > every chunk. > - WORD only (no BYTE/BLOCK): accepted with WARN_ONCE. Even-length > transfers work; odd-length transfers > (e.g. the 3-byte cotsworks fixup > write) hit the BYTE branch which the > adapter does not implement, so the > xfer returns an error and the > operation is aborted. No mainline > I2C driver was found to advertise > WORD without BYTE; the warning lets > us learn about it if it ever shows > up. > > Adapters with asymmetric R/W capabilities (e.g. only READ_I2C_BLOCK > but not WRITE_I2C_BLOCK) remain functionally correct -- the > per-iteration fallback uses the direction-specific bits -- but the > shared i2c_max_block_size is sized by the all-bits-set check, so a > transfer in the better-supported direction is not upgraded. None of > the mainline I2C bus drivers surveyed during review advertise such > asymmetry; promoting i2c_max_block_size to per-direction sizes can > be revisited if needed. > > Signed-off-by: Jonas Jelonek Reviewed-by: Maxime Chevallier Thank you ! Maxime > --- > drivers/net/phy/sfp.c | 134 +++++++++++++++++++++++++++++++++--------- > 1 file changed, 107 insertions(+), 27 deletions(-) > > diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c > index e58e29a1e8d2..16d41d7ee632 100644 > --- a/drivers/net/phy/sfp.c > +++ b/drivers/net/phy/sfp.c > @@ -14,6 +14,7 @@ > #include > #include > #include > +#include > #include > > #include "sfp.h" > @@ -756,50 +757,110 @@ static int sfp_i2c_write(struct sfp *sfp, bool a2, u8 dev_addr, void *buf, > return ret == ARRAY_SIZE(msgs) ? len : 0; > } > > -static int sfp_smbus_byte_read(struct sfp *sfp, bool a2, u8 dev_addr, > - void *buf, size_t len) > +static int sfp_smbus_read(struct sfp *sfp, bool a2, u8 dev_addr, void *buf, > + size_t len) > { > union i2c_smbus_data smbus_data; > u8 bus_addr = a2 ? 0x51 : 0x50; > + size_t this_len, transferred; > + u32 functionality; > u8 *data = buf; > int ret; > > - while (len) { > - ret = i2c_smbus_xfer(sfp->i2c, bus_addr, 0, > - I2C_SMBUS_READ, dev_addr, > - I2C_SMBUS_BYTE_DATA, &smbus_data); > - if (ret < 0) > - return ret; > + functionality = i2c_get_functionality(sfp->i2c); > > - *data = smbus_data.byte; > + while (len) { > + this_len = min(len, sfp->i2c_block_size); > + > + if (functionality & I2C_FUNC_SMBUS_READ_I2C_BLOCK) { > + smbus_data.block[0] = this_len; > + ret = i2c_smbus_xfer(sfp->i2c, bus_addr, 0, > + I2C_SMBUS_READ, dev_addr, > + I2C_SMBUS_I2C_BLOCK_DATA, &smbus_data); > + if (ret < 0) > + return ret; > + > + memcpy(data, &smbus_data.block[1], this_len); > + transferred = this_len; > + } else if (this_len >= 2 && > + (functionality & I2C_FUNC_SMBUS_READ_WORD_DATA)) { > + ret = i2c_smbus_xfer(sfp->i2c, bus_addr, 0, > + I2C_SMBUS_READ, dev_addr, > + I2C_SMBUS_WORD_DATA, &smbus_data); > + if (ret < 0) > + return ret; > + > + put_unaligned_le16(smbus_data.word, data); > + transferred = 2; > + } else { > + ret = i2c_smbus_xfer(sfp->i2c, bus_addr, 0, > + I2C_SMBUS_READ, dev_addr, > + I2C_SMBUS_BYTE_DATA, &smbus_data); > + if (ret < 0) > + return ret; > + > + *data = smbus_data.byte; > + transferred = 1; > + } > > - len--; > - data++; > - dev_addr++; > + data += transferred; > + len -= transferred; > + dev_addr += transferred; > } > > return data - (u8 *)buf; > } > > -static int sfp_smbus_byte_write(struct sfp *sfp, bool a2, u8 dev_addr, > - void *buf, size_t len) > +static int sfp_smbus_write(struct sfp *sfp, bool a2, u8 dev_addr, void *buf, > + size_t len) > { > union i2c_smbus_data smbus_data; > u8 bus_addr = a2 ? 0x51 : 0x50; > + size_t this_len, transferred; > + u32 functionality; > u8 *data = buf; > int ret; > > + functionality = i2c_get_functionality(sfp->i2c); > + > while (len) { > - smbus_data.byte = *data; > - ret = i2c_smbus_xfer(sfp->i2c, bus_addr, 0, > - I2C_SMBUS_WRITE, dev_addr, > - I2C_SMBUS_BYTE_DATA, &smbus_data); > - if (ret) > - return ret; > + this_len = min(len, sfp->i2c_block_size); > + > + if (functionality & I2C_FUNC_SMBUS_WRITE_I2C_BLOCK) { > + smbus_data.block[0] = this_len; > + memcpy(&smbus_data.block[1], data, this_len); > + > + ret = i2c_smbus_xfer(sfp->i2c, bus_addr, 0, > + I2C_SMBUS_WRITE, dev_addr, > + I2C_SMBUS_I2C_BLOCK_DATA, &smbus_data); > + if (ret < 0) > + return ret; > + > + transferred = this_len; > + } else if (this_len >= 2 && > + (functionality & I2C_FUNC_SMBUS_WRITE_WORD_DATA)) { > + smbus_data.word = get_unaligned_le16(data); > + ret = i2c_smbus_xfer(sfp->i2c, bus_addr, 0, > + I2C_SMBUS_WRITE, dev_addr, > + I2C_SMBUS_WORD_DATA, &smbus_data); > + if (ret < 0) > + return ret; > + > + transferred = 2; > + } else { > + smbus_data.byte = *data; > + ret = i2c_smbus_xfer(sfp->i2c, bus_addr, 0, > + I2C_SMBUS_WRITE, dev_addr, > + I2C_SMBUS_BYTE_DATA, &smbus_data); > + if (ret < 0) > + return ret; > + > + transferred = 1; > + } > > - len--; > - data++; > - dev_addr++; > + data += transferred; > + len -= transferred; > + dev_addr += transferred; > } > > return data - (u8 *)buf; > @@ -815,10 +876,29 @@ static int sfp_i2c_configure(struct sfp *sfp, struct i2c_adapter *i2c) > sfp->read = sfp_i2c_read; > sfp->write = sfp_i2c_write; > max_block_size = SFP_EEPROM_BLOCK_SIZE; > - } else if (i2c_check_functionality(i2c, I2C_FUNC_SMBUS_BYTE_DATA)) { > - sfp->read = sfp_smbus_byte_read; > - sfp->write = sfp_smbus_byte_write; > - max_block_size = 1; > + } else if (i2c_check_functionality(i2c, I2C_FUNC_SMBUS_BYTE_DATA) || > + i2c_check_functionality(i2c, I2C_FUNC_SMBUS_I2C_BLOCK)) { > + /* Either protocol alone covers any length: I2C-block carries > + * 1..32 bytes per xfer, byte iterates one byte at a time. > + */ > + sfp->read = sfp_smbus_read; > + sfp->write = sfp_smbus_write; > + > + if (i2c_check_functionality(i2c, I2C_FUNC_SMBUS_I2C_BLOCK)) > + max_block_size = SFP_EEPROM_BLOCK_SIZE; > + else if (i2c_check_functionality(i2c, I2C_FUNC_SMBUS_WORD_DATA)) > + max_block_size = 2; > + else > + max_block_size = 1; > + } else if (WARN_ONCE(i2c_check_functionality(i2c, I2C_FUNC_SMBUS_WORD_DATA), > + "SMBus word-only adapter; odd-length transfers will fail\n")) { > + /* Word-only: even-length xfers work; odd-length xfers fall > + * to BYTE, which the adapter does not advertise and will > + * likely fail. > + */ > + sfp->read = sfp_smbus_read; > + sfp->write = sfp_smbus_write; > + max_block_size = 2; > } else { > sfp->i2c = NULL; > return -EINVAL;