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

[sashiko-bot@kernel.org]

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