Linux-PHY Archive on lore.kernel.org

* Re: [PATCH net v4 2/2] net: phy: mdio-i2c: defer RollBall bridge probe to PHY discovery
From: sashiko-bot @ 2026-06-25  8:48 UTC (permalink / raw)
  To: Petr Wozniak; +Cc: olteanv, vkoul, neil.armstrong, linux-phy
In-Reply-To: <20260624084814.20972-3-petr.wozniak@gmail.com>

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Moving `mdio_i2c_probe_rollball()` into `sfp_sm_probe_for_phy()` aborts the PHY discovery retry loop on the first NAK or timeout, permanently bypassing PHY detection for genuine RollBall modules.
- [High] Removal of the `-ENODEV` fallback in `sfp_sm_add_mdio_bus()` causes fatal SFP module initialization failure instead of gracefully downgrading to non-MDIO mode.
--

commit 21ce5e35892ea02b56c9ead087f775ee5de05519
Author: Petr Wozniak <petr.wozniak@gmail.com>

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

This patch moves the RollBall I2C-to-MDIO bridge probe from bus-creation
time to PHY discovery time to allow slow-booting modules time to
initialize. It also removes the -ENODEV fallback from
sfp_sm_add_mdio_bus().

> By the time sfp_sm_probe_for_phy() runs (up to ~17 s later on affected
> hardware), the bridge is fully initialized but PHY probing is skipped
> because the protocol has already been changed to NONE.

Looking at sfp_sm_event(), sfp_sm_probe_for_phy() runs immediately after
sfp_sm_add_mdio_bus(). The 17-second delay on affected hardware appears to
be the duration of the PHY probe retry loop itself, rather than a delay
before sfp_sm_probe_for_phy() is called.

> diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
> index c4d274ab651e3..01b941a38eed2 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: High]
If the initial password write in i2c_mii_init_rollball() returns -ENODEV
(due to a slow-booting module or transient I2C errors), will this change
cause the SFP state machine to fail entirely?

In sfp_sm_event(), sfp_sm_add_mdio_bus() is called, and if it returns an
error, the module is forced into SFP_S_FAIL:

sfp_sm_event() {
...
        ret = sfp_sm_add_mdio_bus(sfp);
        if (ret < 0) {
                sfp_sm_next(sfp, SFP_S_FAIL, 0);
                break;
        }
...
}

Without the -ENODEV fallback to MDIO_I2C_NONE, it seems this will assert
TX_DISABLE and disable the network link instead of gracefully downgrading.

[ ... ]

> @@ -2215,6 +2208,19 @@ static int sfp_sm_probe_for_phy(struct sfp *sfp)
>  		break;
>  
>  	case MDIO_I2C_ROLLBALL:
> +		/* 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;
> +			err = 0;
> +			break;
> +		}

[Severity: High]
Does this bypass the PHY retry loop entirely for slow-booting genuine
RollBall modules?

The commit message notes that bridge initialization can take ~17s, but
looking at sfp_sm_event(), sfp_sm_probe_for_phy() is called immediately
after sfp_sm_add_mdio_bus() with no delay:

sfp_sm_event() {
...
        init_done:
                ret = sfp_sm_add_mdio_bus(sfp);
...
                goto phy_probe;
...
        phy_probe:
                ret = sfp_sm_probe_for_phy(sfp);
...
}

Since the probe happens immediately, a slow module that hasn't finished
initializing will return -ENODEV. This new code intercepts -ENODEV, sets
err = 0, and breaks out.

Because it returns 0, won't this tell the SFP state machine to skip
the PHY retry loop and proceed without PHY detection?

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

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

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

^ permalink raw reply