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

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

[Aleksander Jan Bajkowski]

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>
---
 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;
+	}
+}
+
 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",
+			    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;
 	}
-- 
2.53.0

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

[Maxime Chevallier]

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

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

[Aleksander Jan Bajkowski]

Hi Maxime,

>> --- 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.
Indeed. I think that all MMD C45 should have the same ID. Can you suggest a
better function name? :)
>
>> +
>>   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 ?
I followed the printk format. For u32, it should be %x instead of %lx.
Should I keep 0x%.8lx?
>> +			    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...
The world of 10Gbase-T SFP+ modules is quite messy. SFP modules with the 
same
part number use different PHYs (for example, an OEM SFP-10G-T might 
internally
use Aquantia AQC113C, Marvell CUX3610, Realtek RTL8211BE, RTL8261C...).
I think the PHY ID is very useful information here. It gives an idea 
which driver
package needs to be installed in the case of OpenWRT. Sometimes it also 
indicates
that a new chip doesn't have a driver in the kernel yet.


Best regarads,
Aleksander

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

[Andrew Lunn]

On Sat, Jun 20, 2026 at 03:11:13PM +0200, 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.

The problem with C45 is there is not one ID, but multiple IDs. And
they can be from different vendors, depending on who the different IP
blocks have been licensed from.

We came to the conclusion not to report any C45 IDs is the most
meaningful thing to do.

      Andrew