[PATCH net-next v4] net: phy: sfp: probe for RollBall I2C-to-MDIO bridge in mdio-i2c

[PATCH net-next v4] net: phy: sfp: probe for RollBall I2C-to-MDIO bridge in mdio-i2c

[Petr Wozniak]

The "OEM"/"SFP-10G-T" quirk entry in sfp_fixup_rollball_cc()
unconditionally forces MDIO_I2C_ROLLBALL for all modules matching that
vendor/part-number combination.  This works for modules that genuinely
implement a RollBall I2C-to-MDIO bridge, but silently breaks modules
that share the same EEPROM strings without having such a bridge.

The Realtek RTL8261BE-CG is one such module: a pure copper 10G SFP+
media converter with no I2C-to-MDIO bridge.  Its EEPROM reports
vendor="OEM", part="SFP-10G-T-I", and -- critically -- Vendor OUI
00:00:00, making OUI-based differentiation impossible.  With
MDIO_I2C_ROLLBALL the kernel stalls waiting for a PHY that never
appears:

  sfp sfp2: probing phy device through the [MDIO_I2C_ROLLBALL] protocol

Move the probe into i2c_mii_init_rollball() in mdio-i2c.c, where the
RollBall protocol constants are already defined.  After sending the
unlock password, issue a CMD_READ and wait ~70 ms for CMD_DONE.  A
genuine RollBall bridge asserts CMD_DONE within that window; modules
without a bridge never do, so i2c_mii_init_rollball() returns -ENODEV.
mdio_i2c_alloc() propagates -ENODEV to the caller without logging an
error.  sfp_sm_add_mdio_bus() catches -ENODEV and transitions
sfp->mdio_protocol to MDIO_I2C_NONE so the rest of the state machine
skips PHY probing for this module.

Add "OEM"/"SFP-10G-T-I" to the quirk table so RTL8261BE modules enter
the probe path; genuine RollBall modules continue to work as before.

Signed-off-by: Petr Wozniak <petr.wozniak@gmail.com>
---

Changes since v3 (feedback from Jakub Kicinski):
  - Drop spurious Tested-by: tag -- author and tester are the same person
  - Use PATCH net-next subject prefix
  - Move -ENODEV handling from sfp_i2c_mdiobus_create() into
    sfp_sm_add_mdio_bus() so bus-creation code does not mutate
    sfp->mdio_protocol; the state machine is the correct place for
    protocol-state transitions
  - Split combined variable declaration for clarity

Changes since v2:
  - Compile-tested and hardware-tested on BPI-R4 (MT7988A, 6.12.87)
  - RTL8261BE (OEM/SFP-10G-T-I): probes MDIO_I2C_NONE, link Up 10Gbps
  - Genuine RollBall (OEM/SFP-10G-T): bridge detected, link Up 10Gbps

 drivers/net/mdio/mdio-i2c.c | 57 +++++++++++++++++++++++++++++++-----
 drivers/net/phy/sfp.c       | 15 ++++++++--
 2 files changed, 62 insertions(+), 10 deletions(-)

--- a/drivers/net/mdio/mdio-i2c.c
+++ b/drivers/net/mdio/mdio-i2c.c
@@ -419,6 +419,46 @@
 	return 0;
 }
 
+static int i2c_mii_probe_rollball(struct i2c_adapter *i2c)
+{
+	u8 data_buf[] = { ROLLBALL_DATA_ADDR, 0x01, 0x00, 0x00 };
+	u8 cmd_buf[]  = { ROLLBALL_CMD_ADDR, ROLLBALL_CMD_READ };
+	u8 cmd_addr = ROLLBALL_CMD_ADDR;
+	u8 result;
+	struct i2c_msg msgs[2];
+	int ret;
+
+	msgs[0].addr  = ROLLBALL_PHY_I2C_ADDR;
+	msgs[0].flags = 0;
+	msgs[0].len   = sizeof(data_buf);
+	msgs[0].buf   = data_buf;
+	msgs[1].addr  = ROLLBALL_PHY_I2C_ADDR;
+	msgs[1].flags = 0;
+	msgs[1].len   = sizeof(cmd_buf);
+	msgs[1].buf   = cmd_buf;
+
+	ret = i2c_transfer_rollball(i2c, msgs, ARRAY_SIZE(msgs));
+	if (ret)
+		return ret;
+
+	msleep(70);
+
+	msgs[0].addr  = ROLLBALL_PHY_I2C_ADDR;
+	msgs[0].flags = 0;
+	msgs[0].len   = 1;
+	msgs[0].buf   = &cmd_addr;
+	msgs[1].addr  = ROLLBALL_PHY_I2C_ADDR;
+	msgs[1].flags = I2C_M_RD;
+	msgs[1].len   = 1;
+	msgs[1].buf   = &result;
+
+	ret = i2c_transfer_rollball(i2c, msgs, ARRAY_SIZE(msgs));
+	if (ret)
+		return ret;
+
+	return result == ROLLBALL_CMD_DONE ? 0 : -ENODEV;
+}
+
 static int i2c_mii_init_rollball(struct i2c_adapter *i2c)
 {
 	struct i2c_msg msg;
@@ -439,10 +479,10 @@
 	ret = i2c_transfer(i2c, &msg, 1);
 	if (ret < 0)
 		return ret;
-	else if (ret != 1)
+	if (ret != 1)
 		return -EIO;
-	else
-		return 0;
+
+	return i2c_mii_probe_rollball(i2c);
 }
 
 static bool mdio_i2c_check_functionality(struct i2c_adapter *i2c,
@@ -487,9 +527,10 @@
 	case MDIO_I2C_ROLLBALL:
 		ret = i2c_mii_init_rollball(i2c);
 		if (ret < 0) {
-			dev_err(parent,
-				"Cannot initialize RollBall MDIO I2C protocol: %d\n",
-				ret);
+			if (ret != -ENODEV)
+				dev_err(parent,
+					"Cannot initialize RollBall MDIO I2C protocol: %d\n",
+					ret);
 			mdiobus_free(mii);
 			return ERR_PTR(ret);
 		}
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -579,7 +579,8 @@
 	// OEM SFP-GE-T is a 1000Base-T module with broken TX_FAULT indicator
 	SFP_QUIRK_F("OEM", "SFP-GE-T", sfp_fixup_ignore_tx_fault),
 
-	SFP_QUIRK_F("OEM", "SFP-10G-T", sfp_fixup_rollball_cc),
+	SFP_QUIRK_F("OEM", "SFP-10G-T-I", sfp_fixup_rollball),
+	SFP_QUIRK_F("OEM", "SFP-10G-T",   sfp_fixup_rollball_cc),
 	SFP_QUIRK_S("OEM", "SFP-2.5G-T", sfp_quirk_oem_2_5g),
 	SFP_QUIRK_S("OEM", "SFP-2.5G-BX10-D", sfp_quirk_2500basex),
 	SFP_QUIRK_S("OEM", "SFP-2.5G-BX10-U", sfp_quirk_2500basex),
@@ -2022,10 +2023,17 @@
 
 static int sfp_sm_add_mdio_bus(struct sfp *sfp)
 {
-	if (sfp->mdio_protocol != MDIO_I2C_NONE)
-		return sfp_i2c_mdiobus_create(sfp);
+	int ret;
 
-	return 0;
+	if (sfp->mdio_protocol == MDIO_I2C_NONE)
+		return 0;
+
+	ret = sfp_i2c_mdiobus_create(sfp);
+	if (ret == -ENODEV) {
+		sfp->mdio_protocol = MDIO_I2C_NONE;
+		return 0;
+	}
+	return ret;
 }
 
 /* Probe a SFP for a PHY device if the module supports copper - the PHY
-- 
2.51.0

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH net-next v4] net: phy: sfp: probe for RollBall I2C-to-MDIO bridge in mdio-i2c

[Maxime Chevallier]

Hi Petr,

On 5/19/26 06:32, Petr Wozniak wrote:
> The "OEM"/"SFP-10G-T" quirk entry in sfp_fixup_rollball_cc()
> unconditionally forces MDIO_I2C_ROLLBALL for all modules matching that
> vendor/part-number combination.  This works for modules that genuinely
> implement a RollBall I2C-to-MDIO bridge, but silently breaks modules
> that share the same EEPROM strings without having such a bridge.
> 
> The Realtek RTL8261BE-CG is one such module: a pure copper 10G SFP+
> media converter with no I2C-to-MDIO bridge.  Its EEPROM reports
> vendor="OEM", part="SFP-10G-T-I", and -- critically -- Vendor OUI
> 00:00:00, making OUI-based differentiation impossible.  With
> MDIO_I2C_ROLLBALL the kernel stalls waiting for a PHY that never
> appears:
> 
>    sfp sfp2: probing phy device through the [MDIO_I2C_ROLLBALL] protocol

Is it really stalling, or are you facing the 25 seconds retry loop for 
rollball ?

> 
> Move the probe into i2c_mii_init_rollball() in mdio-i2c.c, where the
> RollBall protocol constants are already defined.  After sending the
> unlock password, issue a CMD_READ and wait ~70 ms for CMD_DONE.  A
> genuine RollBall bridge asserts CMD_DONE within that window; modules
> without a bridge never do, so i2c_mii_init_rollball() returns -ENODEV.
> mdio_i2c_alloc() propagates -ENODEV to the caller without logging an
> error.  sfp_sm_add_mdio_bus() catches -ENODEV and transitions
> sfp->mdio_protocol to MDIO_I2C_NONE so the rest of the state machine
> skips PHY probing for this module.
> 
> Add "OEM"/"SFP-10G-T-I" to the quirk table so RTL8261BE modules enter
> the probe path; genuine RollBall modules continue to work as before.
> 
> Signed-off-by: Petr Wozniak <petr.wozniak@gmail.com>

The overall approach is fine by me, I see that being potentially useful
for other use-cases (e.g. the 100FX modules that may or may not embed a
PHY).

we should document that a bit better though, stating that returning
-ENODEV from mdio_i2c_alloc() means we know for a fact there's no
PHY there.

I do have a few nitpicks, mostly style, see bellow, with these fixed you 
can add :

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

> ---
> 
> Changes since v3 (feedback from Jakub Kicinski):
>    - Drop spurious Tested-by: tag -- author and tester are the same person
>    - Use PATCH net-next subject prefix
>    - Move -ENODEV handling from sfp_i2c_mdiobus_create() into
>      sfp_sm_add_mdio_bus() so bus-creation code does not mutate
>      sfp->mdio_protocol; the state machine is the correct place for
>      protocol-state transitions
>    - Split combined variable declaration for clarity
> 
> Changes since v2:
>    - Compile-tested and hardware-tested on BPI-R4 (MT7988A, 6.12.87)
>    - RTL8261BE (OEM/SFP-10G-T-I): probes MDIO_I2C_NONE, link Up 10Gbps
>    - Genuine RollBall (OEM/SFP-10G-T): bridge detected, link Up 10Gbps
> 
>   drivers/net/mdio/mdio-i2c.c | 57 +++++++++++++++++++++++++++++++-----
>   drivers/net/phy/sfp.c       | 15 ++++++++--
>   2 files changed, 62 insertions(+), 10 deletions(-)
> 
> --- a/drivers/net/mdio/mdio-i2c.c
> +++ b/drivers/net/mdio/mdio-i2c.c
> @@ -419,6 +419,46 @@
>   	return 0;
>   }
>   
> +static int i2c_mii_probe_rollball(struct i2c_adapter *i2c)
> +{
> +	u8 data_buf[] = { ROLLBALL_DATA_ADDR, 0x01, 0x00, 0x00 };
> +	u8 cmd_buf[]  = { ROLLBALL_CMD_ADDR, ROLLBALL_CMD_READ };
> +	u8 cmd_addr = ROLLBALL_CMD_ADDR;
> +	u8 result;
> +	struct i2c_msg msgs[2];
> +	int ret;

You should follow reverse-xmas tree ordering, sorting by descending line
length.

> +
> +	msgs[0].addr  = ROLLBALL_PHY_I2C_ADDR;
> +	msgs[0].flags = 0;
> +	msgs[0].len   = sizeof(data_buf);
> +	msgs[0].buf   = data_buf;
> +	msgs[1].addr  = ROLLBALL_PHY_I2C_ADDR;
> +	msgs[1].flags = 0;
> +	msgs[1].len   = sizeof(cmd_buf);
> +	msgs[1].buf   = cmd_buf;
> +
> +	ret = i2c_transfer_rollball(i2c, msgs, ARRAY_SIZE(msgs));
> +	if (ret)
> +		return ret;
> +
> +	msleep(70);
> +
> +	msgs[0].addr  = ROLLBALL_PHY_I2C_ADDR;
> +	msgs[0].flags = 0;
> +	msgs[0].len   = 1;
> +	msgs[0].buf   = &cmd_addr;
> +	msgs[1].addr  = ROLLBALL_PHY_I2C_ADDR;
> +	msgs[1].flags = I2C_M_RD;
> +	msgs[1].len   = 1;
> +	msgs[1].buf   = &result;
> +
> +	ret = i2c_transfer_rollball(i2c, msgs, ARRAY_SIZE(msgs));
> +	if (ret)
> +		return ret;
> +
> +	return result == ROLLBALL_CMD_DONE ? 0 : -ENODEV;
> +}
> +
>   static int i2c_mii_init_rollball(struct i2c_adapter *i2c)
>   {
>   	struct i2c_msg msg;
> @@ -439,10 +479,10 @@
>   	ret = i2c_transfer(i2c, &msg, 1);
>   	if (ret < 0)
>   		return ret;
> -	else if (ret != 1)
> +	if (ret != 1)
>   		return -EIO;
> -	else
> -		return 0;
> +
> +	return i2c_mii_probe_rollball(i2c);
>   }
>   
>   static bool mdio_i2c_check_functionality(struct i2c_adapter *i2c,
> @@ -487,9 +527,10 @@
>   	case MDIO_I2C_ROLLBALL:
>   		ret = i2c_mii_init_rollball(i2c);
>   		if (ret < 0) {
> -			dev_err(parent,
> -				"Cannot initialize RollBall MDIO I2C protocol: %d\n",
> -				ret);
> +			if (ret != -ENODEV)
> +				dev_err(parent,
> +					"Cannot initialize RollBall MDIO I2C protocol: %d\n",
> +					ret);
>   			mdiobus_free(mii);
>   			return ERR_PTR(ret);
>   		}
> --- a/drivers/net/phy/sfp.c
> +++ b/drivers/net/phy/sfp.c
> @@ -579,7 +579,8 @@
>   	// OEM SFP-GE-T is a 1000Base-T module with broken TX_FAULT indicator
>   	SFP_QUIRK_F("OEM", "SFP-GE-T", sfp_fixup_ignore_tx_fault),
>   
> -	SFP_QUIRK_F("OEM", "SFP-10G-T", sfp_fixup_rollball_cc),
> +	SFP_QUIRK_F("OEM", "SFP-10G-T-I", sfp_fixup_rollball),
> +	SFP_QUIRK_F("OEM", "SFP-10G-T",   sfp_fixup_rollball_cc),

there's an extra space added for the "SFP-10G-T" entry, which makes it 
appear in the diff unnecessarily.

>   	SFP_QUIRK_S("OEM", "SFP-2.5G-T", sfp_quirk_oem_2_5g),
>   	SFP_QUIRK_S("OEM", "SFP-2.5G-BX10-D", sfp_quirk_2500basex),
>   	SFP_QUIRK_S("OEM", "SFP-2.5G-BX10-U", sfp_quirk_2500basex),
> @@ -2022,10 +2023,17 @@
>   
>   static int sfp_sm_add_mdio_bus(struct sfp *sfp)
>   {
> -	if (sfp->mdio_protocol != MDIO_I2C_NONE)
> -		return sfp_i2c_mdiobus_create(sfp);
> +	int ret;
>   
> -	return 0;
> +	if (sfp->mdio_protocol == MDIO_I2C_NONE)
> +		return 0;
> +
> +	ret = sfp_i2c_mdiobus_create(sfp);
> +	if (ret == -ENODEV) {
> +		sfp->mdio_protocol = MDIO_I2C_NONE;
> +		return 0;
> +	}
> +	return ret;
>   }
>   
>   /* Probe a SFP for a PHY device if the module supports copper - the PHY

Maxime


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH net-next v4] net: phy: sfp: probe for RollBall I2C-to-MDIO bridge in mdio-i2c

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Destructive I2C probing blindly writes to generic SFP EEPROM, causing permanent data corruption.
- [High] Unhandled I2C NACK errors break the fallback mechanism, causing complete SFP initialization failure.
- [High] The RollBall PHY probe relies on a single fixed 70ms delay followed by a single read, which is brittle and prone to false negatives.
--

commit 540349fd79e1edba85ca3827cf32c57605686ef5
Author: Petr Wozniak <petr.wozniak@gmail.com>

net: phy: sfp: probe for RollBall I2C-to-MDIO bridge in mdio-i2c

This patch adds a probe to dynamically check if an SFP module implements the
RollBall I2C-to-MDIO bridge protocol by attempting a read command. It routes
RTL8261BE modules through this probe to differentiate them from genuine
RollBall modules sharing the same EEPROM strings.

> diff --git a/drivers/net/mdio/mdio-i2c.c b/drivers/net/mdio/mdio-i2c.c
> index ed20352a589a3..dda849c264ad0 100644
> --- a/drivers/net/mdio/mdio-i2c.c
> +++ b/drivers/net/mdio/mdio-i2c.c
[ ... ]
> @@ -419,6 +419,46 @@ static int i2c_mii_write_rollball(struct mii_bus *bus, int phy_id, int devad,
>  	return 0;
>  }
>  
> +static int i2c_mii_probe_rollball(struct i2c_adapter *i2c)
> +{
> +	u8 data_buf[] = { ROLLBALL_DATA_ADDR, 0x01, 0x00, 0x00 };
> +	u8 cmd_buf[]  = { ROLLBALL_CMD_ADDR, ROLLBALL_CMD_READ };
> +	u8 cmd_addr = ROLLBALL_CMD_ADDR;
> +	u8 result;
> +	struct i2c_msg msgs[2];
> +	int ret;
> +
> +	msgs[0].addr  = ROLLBALL_PHY_I2C_ADDR;
> +	msgs[0].flags = 0;
> +	msgs[0].len   = sizeof(data_buf);
> +	msgs[0].buf   = data_buf;
> +	msgs[1].addr  = ROLLBALL_PHY_I2C_ADDR;
> +	msgs[1].flags = 0;
> +	msgs[1].len   = sizeof(cmd_buf);
> +	msgs[1].buf   = cmd_buf;
> +
> +	ret = i2c_transfer_rollball(i2c, msgs, ARRAY_SIZE(msgs));
> +	if (ret)
> +		return ret;

For non-RollBall modules that do accept writes without NACKing, could these
probe writes permanently corrupt their EEPROM data?

The SFF-8472 specification defines 0x51 (A2h) offsets 128-255 as
user-writable non-volatile memory. By explicitly adding the SFP-10G-T-I quirk
to route non-RollBall modules (like the Realtek RTL8261BE) into this probe
path, they are subjected to writes that could permanently overwrite their
contents if they don't hardware write-protect this area.

> +
> +	msleep(70);

Could a genuine RollBall module take slightly longer than 70ms to respond?

The existing i2c_rollball_mii_poll() function polls for up to 200ms
(10 iterations of 20ms). If a module takes longer than 70ms, it seems this
single fixed delay might cause it to fail the probe and return -ENODEV,
resulting in misidentification.

> +
> +	msgs[0].addr  = ROLLBALL_PHY_I2C_ADDR;
[ ... ]
> +	return result == ROLLBALL_CMD_DONE ? 0 : -ENODEV;
> +}
> +
>  static int i2c_mii_init_rollball(struct i2c_adapter *i2c)
>  {
>  	struct i2c_msg msg;
> @@ -439,10 +479,10 @@ static int i2c_mii_init_rollball(struct i2c_adapter *i2c)
>  	ret = i2c_transfer(i2c, &msg, 1);
>  	if (ret < 0)
>  		return ret;

If a non-RollBall module's EEPROM is write-protected or lacks an A2h device
entirely, will the I2C bus NACK this transfer?

If i2c_transfer() returns a negative error (like -ENXIO) on NACK, it will
propagate up through the initialization chain.

[ ... ]
> diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
> index bd970f753beb6..2be355ebacec3 100644
> --- a/drivers/net/phy/sfp.c
> +++ b/drivers/net/phy/sfp.c
[ ... ]
> @@ -2022,10 +2023,17 @@ static void sfp_sm_fault(struct sfp *sfp, unsigned int next_state, bool warn)
>  
>  static int sfp_sm_add_mdio_bus(struct sfp *sfp)
>  {
> -	if (sfp->mdio_protocol != MDIO_I2C_NONE)
> -		return sfp_i2c_mdiobus_create(sfp);
> +	int ret;
>  
> -	return 0;
> +	if (sfp->mdio_protocol == MDIO_I2C_NONE)
> +		return 0;
> +
> +	ret = sfp_i2c_mdiobus_create(sfp);
> +	if (ret == -ENODEV) {
> +		sfp->mdio_protocol = MDIO_I2C_NONE;
> +		return 0;
> +	}
> +	return ret;
>  }

Since sfp_sm_add_mdio_bus() only falls back to MDIO_I2C_NONE when the error
is exactly -ENODEV, won't returning a NACK error from the I2C transfer cause
the initialization to abort completely for non-RollBall modules?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260519043249.2868-1-petr.wozniak@gmail.com?part=1

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH net-next v4] net: phy: sfp: probe for RollBall I2C-to-MDIO bridge in mdio-i2c

[Jan Hoffmann]

On 2026-05-29 at 12:13, Maxime Chevallier wrote:
> Hi Petr,
> 
> On 5/19/26 06:32, Petr Wozniak wrote:
>> The "OEM"/"SFP-10G-T" quirk entry in sfp_fixup_rollball_cc()
>> unconditionally forces MDIO_I2C_ROLLBALL for all modules matching that
>> vendor/part-number combination.  This works for modules that genuinely
>> implement a RollBall I2C-to-MDIO bridge, but silently breaks modules
>> that share the same EEPROM strings without having such a bridge.
>>
>> The Realtek RTL8261BE-CG is one such module: a pure copper 10G SFP+
>> media converter with no I2C-to-MDIO bridge.  Its EEPROM reports
>> vendor="OEM", part="SFP-10G-T-I", and -- critically -- Vendor OUI
>> 00:00:00, making OUI-based differentiation impossible.  With
>> MDIO_I2C_ROLLBALL the kernel stalls waiting for a PHY that never
>> appears:
>>
>>    sfp sfp2: probing phy device through the [MDIO_I2C_ROLLBALL] protocol
> 
> Is it really stalling, or are you facing the 25 seconds retry loop for 
> rollball ?

The retry loop unfortunately takes much longer than 25 seconds if the 
controller in the module is not responding to the Rollball protocol:

- The ".read_c45" method of the Rollball implementation returns 0xffff 
on timeout (which happens after 10 tries, sleeping 20 ms for each).

- When all reads return 0xffff, "get_phy_c45_ids" reads 62 registers 
("MDIO_DEVS1" and "MDIO_DEVS2" for MMD devices 0-29, plus "MDIO_STAT2" 
for MMD 30 and 31).

Considering the 25 attempts for probing, this means the actual retry 
loop is at least 5.5 minutes long. In practice it is even worse, as the 
I2C transfers also take some time.

It seems to me like this is the core of the issue here. If the timeout 
was just 25 seconds, it would be a bit annoying, but still usable.

Jan


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy