Hi Maxime,

quoted

--- 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? :)

quoted

+
  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?

quoted

+			    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