Re: [PATCH net-next v3 2/2] net: phy: mdio-i2c: defer RollBall bridge probe to PHY discovery

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Moving the RollBall bridge probe to `sfp_sm_probe_for_phy()` and immediately dropping the MDIO protocol to `MDIO_I2C_NONE` upon failure breaks the PHY probe retry loop for genuine RollBall modules that are slow to initialize.
- [Medium] Removing the `-ENODEV` fallback mechanism in `sfp_sm_add_mdio_bus()` causes the entire SFP module to hard fail (`SFP_S_FAIL`) if the initial I2C password transfer fails.
- [Medium] Missing `err = 0` causes an unnecessary 1-second retry delay after explicitly destroying the MDIO bus for non-RollBall modules.
--

commit 7a74af3ae93e2b90c965b0568d5060a89ecb49ad
Author: Petr Wozniak <petr.wozniak@gmail.com>

net: phy: mdio-i2c: defer RollBall bridge probe to PHY discovery

commit 8fe125892f40 ("net: phy: sfp: probe for RollBall I2C-to-MDIO
bridge in mdio-i2c") introduced a regression: the RollBall I2C-to-MDIO
bridge is not yet ready to respond to CMD_READ/CMD_DONE cycles when
sfp_sm_add_mdio_bus() runs in SFP_S_INIT.

> diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
> index c4d274ab651e3..bbfaa0450798d 100644
> --- a/drivers/net/phy/sfp.c
> +++ b/drivers/net/phy/sfp.c
> @@ -2174,17 +2174,10 @@ static void sfp_sm_fault(struct sfp *sfp, unsigned int next_state, bool warn)
>  
>  static int sfp_sm_add_mdio_bus(struct sfp *sfp)
>  {
> -	int ret;
> -
>  	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;
> +	return sfp_i2c_mdiobus_create(sfp);
>  }

[Severity: Medium]
Does removing this -ENODEV fallback cause the SFP module to hard fail if the
initial I2C password transfer fails?

In i2c_mii_init_rollball(), if the module NAKs the password sequence, it
returns -ENODEV. Previously, this was intercepted here to safely degrade the
protocol to MDIO_I2C_NONE. Now that -ENODEV propagates up to sfp_sm_main(),
it appears it will treat it as a fatal error and transition the module to
SFP_S_FAIL, completely disabling the module instead of continuing without MDIO.

[ ... ]

> @@ -2215,7 +2208,18 @@ static int sfp_sm_probe_for_phy(struct sfp *sfp)
>  		break;
>  
>  	case MDIO_I2C_ROLLBALL:
> -		err = sfp_sm_probe_phy(sfp, SFP_PHY_ADDR_ROLLBALL, true);
> +		/* Probe here, after module initialization delays, so that
> +		 * genuine RollBall bridges have had time to start up.
> +		 * Modules without a bridge (e.g. RTL8261BE) return -ENODEV.
> +		 */
> +		err = mdio_i2c_probe_rollball(sfp->i2c);
> +		if (err == -ENODEV) {
> +			sfp_i2c_mdiobus_destroy(sfp);
> +			sfp->mdio_protocol = MDIO_I2C_NONE;

[Severity: High]
Can setting sfp->mdio_protocol to MDIO_I2C_NONE on the first probe failure
break the PHY probe retry loop for slow genuine RollBall modules?

The SFP state machine provides a retry loop for modules that take a long time
to boot. If a slow module fails the 200ms probe on this first attempt, the
protocol is permanently set to MDIO_I2C_NONE. When the state machine triggers
its retry 1 second later, sfp_sm_probe_for_phy() will see MDIO_I2C_NONE,
instantly return 0, and bypass PHY discovery completely.

> +			break;
> +		}

[Severity: Medium]
Will leaving err as -ENODEV cause an unnecessary 1-second retry delay in
sfp_sm_main() for non-RollBall modules?

The caller sfp_sm_main() interprets -ENODEV as "PHY not yet found" and
schedules a state machine retry after 1 second. If the intent is to cleanly
skip the retry loop after destroying the bus, should err be set to 0 before
breaking?

> +		if (!err)
> +			err = sfp_sm_probe_phy(sfp, SFP_PHY_ADDR_ROLLBALL, true);
>  		break;
>  	}

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

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