Re: [PATCH net] net: phylink: print correct c45 phy id when missing PHY driver

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

Hi Aleksander,

On 6/20/26 15:11, Aleksander Jan Bajkowski wrote:
> If no PHY driver is found, `phy_id` is returned. `phy_id` holds the c22 ID.
> Modules with a rollball bridge support only c45 transfers. The c45 IDs are
> stored in the `c45_ids` structure. In the current code these modules report
> an ID 0x00000000. This may lead users to mistakenly conclude that the
> rollball bridge isn't properly implemented in their SFP module. This patch
> fixes the wrong IDs for c45 modules when a driver cannot be found.
> 
> Tested on Fiberstore SFP-GB-BE-T (C22) and ONTi ONT-C1TE-R05 (Rollball).
> 
> Before:
> [ 2440.373985] mtk_soc_eth 15100000.ethernet sfp-lan: PHY i2c:sfp2:11 (id 0x00000000) has no driver loaded
> [ 2440.383385] mtk_soc_eth 15100000.ethernet sfp-lan: Drivers which handle known common cases: CONFIG_BCM84881_PHY, CONFIG_MARVELL_PHY
> [ 2440.395274] sfp sfp2: sfp_add_phy failed: -EINVAL
> 
> After:
> [   82.573700] mtk_soc_eth 15100000.ethernet sfp-lan: PHY i2c:sfp2:11 (id 0x001cc898) has no driver loaded
> [   82.583098] mtk_soc_eth 15100000.ethernet sfp-lan: Drivers which handle known common cases: CONFIG_BCM84881_PHY, CONFIG_MARVELL_PHY
> [   82.594996] sfp sfp2: sfp_add_phy failed: -EINVAL
> 
> Fixes: ffcbfb5f9779 ("net: phylink: improve phylink_sfp_config_phy() error message with missing PHY driver")
> Signed-off-by: Aleksander Jan Bajkowski <olek2@wp.pl>

TBH I'm not convinced this counts as a net-worthy fix, I'd send that
to net-next when it reopens instead. Sure this is useful debug info,
but when you hit that error message you're already in for some digging
around in the code/config :)

> ---
>  drivers/net/phy/phylink.c | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 087ac63f9193..7d7595158bf9 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -3917,13 +3917,30 @@ static void phylink_sfp_link_up(void *upstream)
>  	phylink_enable_and_run_resolve(pl, PHYLINK_DISABLE_LINK);
>  }
>  
> +static u32 phylink_get_phy_id(struct phy_device *phy)
> +{
> +	if (phy->is_c45) {
> +		const int num_ids = ARRAY_SIZE(phy->c45_ids.device_ids);
> +		int i;
> +
> +		for (i = 1; i < num_ids; i++) {
> +			if (phy->c45_ids.mmds_present & BIT(i))
> +				return (phy->c45_ids.device_ids[i]);
> +		}
> +
> +		return 0;
> +	} else {
> +		return phy->phy_id;
> +	}
> +}

The function name is misleading, you don't really get the id, you get either
the c22 id or the first non-zero C45 id.

> +
>  static int phylink_sfp_connect_phy(void *upstream, struct phy_device *phy)
>  {
>  	struct phylink *pl = upstream;
>  
>  	if (!phy->drv) {
> -		phylink_err(pl, "PHY %s (id 0x%.8lx) has no driver loaded\n",
> -			    phydev_name(phy), (unsigned long)phy->phy_id);
> +		phylink_err(pl, "PHY %s (id 0x%.8x) has no driver loaded\n",

Why change the printk format from 0x%.8lx to 0x%.8x ?

> +			    phydev_name(phy), phylink_get_phy_id(phy));
>  		phylink_err(pl, "Drivers which handle known common cases: CONFIG_BCM84881_PHY, CONFIG_MARVELL_PHY\n");
>  		return -EINVAL;
>  	}


After reading all that, I'm actually not really convinced the overall patch
is the best approach. It's a lot of logic for a very niche case. This is really for
debug purposes, so why not instead print either the phy_id for a C22 PHY, or
just "C45 PHY" and no id at all for C45 ? This removes the confusion about the
id being 0, while still being cleat that the user needs to figure-out what's
going on with their module...

Maxime