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