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

[Aleksander Jan Bajkowski <olek2@wp.pl>]

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