Re: [PATCH net-next 1/2] net: phy: use phy_id_mask value zero for exact match

[Heiner Kallweit <hkallweit1@gmail.com>]

On 08.11.2018 20:44, Florian Fainelli wrote:
> On 11/7/18 12:53 PM, Heiner Kallweit wrote:
>> A phy_id_mask value zero means every PHYID matches, therefore
>> value zero isn't used. So we can safely redefine the semantics
>> of value zero to mean "exact match". This allows to avoid some
>> boilerplate code in PHY driver configs.
> 
> Having run recently into some ethtool quirkyness about how masks are
> supposed to be specified between ntuple/nfc, where the meaning of 0 is
> either don't care or match, I would rather we stick with the current
> behavior where every bit set to 0 is a don't care and bits set t 1 are not.
> 
I agree that using a mask value 0 as either exact match or don't care
can be confusing. However I think that the advantages here outweigh
this confusion aspect.
- We get a meaningful default in case a driver author misses to set
  the phy_id_mask.
- If a driver author wants to enforce an exact match, he has to do
  nothing and can rely on the core. This avoids mistakes like in the
  Realtek case where the driver author meant exact match but specified
  something completely different.
- Avoid boilerplate code

> Maybe we can find a clever way with a macro to specify only the PHY OUI
> and compute a suitable mask automatically?
> 
I don't think so. For Realtek each driver is specific even to a model
revision (therefore mask 0xffffffff). Same applies to intel-xway.
In broadcom.c we have masks 0xfffffff0, so for each model, independent
of revision number. There is no general rule.
Also we can't simply check for the first-bit-set to derive a mask.

>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/net/phy/phy_device.c | 21 +++++++++++++++------
>>  include/linux/phy.h          |  2 +-
>>  2 files changed, 16 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>> index ab33d1777..d165a2c82 100644
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
>> @@ -483,15 +483,24 @@ static int phy_bus_match(struct device *dev, struct device_driver *drv)
>>  			if (!(phydev->c45_ids.devices_in_package & (1 << i)))
>>  				continue;
>>  
>> -			if ((phydrv->phy_id & phydrv->phy_id_mask) ==
>> -			    (phydev->c45_ids.device_ids[i] &
>> -			     phydrv->phy_id_mask))
>> -				return 1;
>> +			if (!phydrv->phy_id_mask) {
>> +				if (phydrv->phy_id ==
>> +				    phydev->c45_ids.device_ids[i])
>> +					return 1;
>> +			} else {
>> +				if ((phydrv->phy_id & phydrv->phy_id_mask) ==
>> +				    (phydev->c45_ids.device_ids[i] &
>> +				     phydrv->phy_id_mask))
>> +					return 1;
>> +			}
>>  		}
>>  		return 0;
>>  	} else {
>> -		return (phydrv->phy_id & phydrv->phy_id_mask) ==
>> -			(phydev->phy_id & phydrv->phy_id_mask);
>> +		if (!phydrv->phy_id_mask)
>> +			return phydrv->phy_id == phydev->phy_id;
>> +		else
>> +			return (phydrv->phy_id & phydrv->phy_id_mask) ==
>> +			       (phydev->phy_id & phydrv->phy_id_mask);
>>  	}
>>  }
>>  
>> diff --git a/include/linux/phy.h b/include/linux/phy.h
>> index 2090277ea..e30ca2fdd 100644
>> --- a/include/linux/phy.h
>> +++ b/include/linux/phy.h
>> @@ -500,7 +500,7 @@ struct phy_driver {
>>  	struct mdio_driver_common mdiodrv;
>>  	u32 phy_id;
>>  	char *name;
>> -	u32 phy_id_mask;
>> +	u32 phy_id_mask; /* value 0 means exact match */
>>  	const unsigned long * const features;
>>  	u32 flags;
>>  	const void *driver_data;
>>
> 
>