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

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

[Heiner Kallweit]

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.

Realtek PHY driver is the first user of this change.

Heiner Kallweit (2):
  net: phy: use phy_id_mask value zero for exact match
  net: phy: realtek: remove boilerplate code from PHY driver definitions

 drivers/net/phy/phy_device.c | 21 +++++++++++++++------
 drivers/net/phy/realtek.c    |  9 ---------
 include/linux/phy.h          |  2 +-
 3 files changed, 16 insertions(+), 16 deletions(-)

-- 
2.19.1

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

[Heiner Kallweit]

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.

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;
-- 
2.19.1

[PATCH net-next 2/2] net: phy: realtek: remove boilerplate code from driver configs

[Heiner Kallweit]

After a recent change phy_id_mask value 0 means "exact match". This
allows to remove setting phy_id_mask values from the driver configs.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/realtek.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index 7b1c89b38..abff4cdc9 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -215,13 +215,11 @@ static struct phy_driver realtek_drvs[] = {
 	{
 		.phy_id         = 0x00008201,
 		.name           = "RTL8201CP Ethernet",
-		.phy_id_mask    = 0x0000ffff,
 		.features       = PHY_BASIC_FEATURES,
 		.flags          = PHY_HAS_INTERRUPT,
 	}, {
 		.phy_id		= 0x001cc816,
 		.name		= "RTL8201F Fast Ethernet",
-		.phy_id_mask	= 0x001fffff,
 		.features	= PHY_BASIC_FEATURES,
 		.flags		= PHY_HAS_INTERRUPT,
 		.ack_interrupt	= &rtl8201_ack_interrupt,
@@ -233,7 +231,6 @@ static struct phy_driver realtek_drvs[] = {
 	}, {
 		.phy_id		= 0x001cc910,
 		.name		= "RTL8211 Gigabit Ethernet",
-		.phy_id_mask	= 0x001fffff,
 		.features	= PHY_GBIT_FEATURES,
 		.config_aneg	= rtl8211_config_aneg,
 		.read_mmd	= &genphy_read_mmd_unsupported,
@@ -241,7 +238,6 @@ static struct phy_driver realtek_drvs[] = {
 	}, {
 		.phy_id		= 0x001cc912,
 		.name		= "RTL8211B Gigabit Ethernet",
-		.phy_id_mask	= 0x001fffff,
 		.features	= PHY_GBIT_FEATURES,
 		.flags		= PHY_HAS_INTERRUPT,
 		.ack_interrupt	= &rtl821x_ack_interrupt,
@@ -253,7 +249,6 @@ static struct phy_driver realtek_drvs[] = {
 	}, {
 		.phy_id		= 0x001cc913,
 		.name		= "RTL8211C Gigabit Ethernet",
-		.phy_id_mask	= 0x001fffff,
 		.features	= PHY_GBIT_FEATURES,
 		.config_init	= rtl8211c_config_init,
 		.read_mmd	= &genphy_read_mmd_unsupported,
@@ -261,7 +256,6 @@ static struct phy_driver realtek_drvs[] = {
 	}, {
 		.phy_id		= 0x001cc914,
 		.name		= "RTL8211DN Gigabit Ethernet",
-		.phy_id_mask	= 0x001fffff,
 		.features	= PHY_GBIT_FEATURES,
 		.flags		= PHY_HAS_INTERRUPT,
 		.ack_interrupt	= rtl821x_ack_interrupt,
@@ -271,7 +265,6 @@ static struct phy_driver realtek_drvs[] = {
 	}, {
 		.phy_id		= 0x001cc915,
 		.name		= "RTL8211E Gigabit Ethernet",
-		.phy_id_mask	= 0x001fffff,
 		.features	= PHY_GBIT_FEATURES,
 		.flags		= PHY_HAS_INTERRUPT,
 		.ack_interrupt	= &rtl821x_ack_interrupt,
@@ -281,7 +274,6 @@ static struct phy_driver realtek_drvs[] = {
 	}, {
 		.phy_id		= 0x001cc916,
 		.name		= "RTL8211F Gigabit Ethernet",
-		.phy_id_mask	= 0x001fffff,
 		.features	= PHY_GBIT_FEATURES,
 		.flags		= PHY_HAS_INTERRUPT,
 		.config_init	= &rtl8211f_config_init,
@@ -294,7 +286,6 @@ static struct phy_driver realtek_drvs[] = {
 	}, {
 		.phy_id		= 0x001cc961,
 		.name		= "RTL8366RB Gigabit Ethernet",
-		.phy_id_mask	= 0x001fffff,
 		.features	= PHY_GBIT_FEATURES,
 		.flags		= PHY_HAS_INTERRUPT,
 		.config_init	= &rtl8366rb_config_init,
-- 
2.19.1

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

[Andrew Lunn]

On Wed, Nov 07, 2018 at 09:53:32PM +0100, Heiner Kallweit wrote:
> @@ -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 */

Hi Heiner

Please make this comment part of the kerneldoc which is just above:

 * phy_id: The result of reading the UID registers of this PHY
 *   type, and ANDing them with the phy_id_mask.

 Andrew

Re: [PATCH net-next 2/2] net: phy: realtek: remove boilerplate code from driver configs

[Andrew Lunn]

>  	{
>  		.phy_id         = 0x00008201,
>  		.name           = "RTL8201CP Ethernet",
> -		.phy_id_mask    = 0x0000ffff,
>  		.features       = PHY_BASIC_FEATURES,
>  		.flags          = PHY_HAS_INTERRUPT,
>  	}, {
>  		.phy_id		= 0x001cc816,
>  		.name		= "RTL8201F Fast Ethernet",
> -		.phy_id_mask	= 0x001fffff,

Hi Heiner

"RTL8201CP Ethernet" has a mask of 0x0000ffff, where as all the others
use 0x001fffff. Is this correct?

    Andrew

Re: [PATCH net-next 2/2] net: phy: realtek: remove boilerplate code from driver configs

[Heiner Kallweit]

On 08.11.2018 19:37, Andrew Lunn wrote:
>>  	{
>>  		.phy_id         = 0x00008201,
>>  		.name           = "RTL8201CP Ethernet",
>> -		.phy_id_mask    = 0x0000ffff,
>>  		.features       = PHY_BASIC_FEATURES,
>>  		.flags          = PHY_HAS_INTERRUPT,
>>  	}, {
>>  		.phy_id		= 0x001cc816,
>>  		.name		= "RTL8201F Fast Ethernet",
>> -		.phy_id_mask	= 0x001fffff,
> 
> Hi Heiner
> 
> "RTL8201CP Ethernet" has a mask of 0x0000ffff, where as all the others
> use 0x001fffff. Is this correct?
> 
IMO none of the masks is correct. All of them should be 0xffffffff.
Nowadays the 32 bit PHYID  is assembled from (MSB to LSB):
22 bits vendor OUI, 6 bit model number, 4 bit revision number
Just the old 8201 doesn't follow this scheme.

With the current masks, a PHYID 0x12348201 would be recognized as
Realtek 8201 too, what's obviously wrong.

>     Andrew
> 

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

[Florian Fainelli]

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.

Maybe we can find a clever way with a macro to specify only the PHY OUI
and compute a suitable mask automatically?

> 
> 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;
> 


-- 
Florian

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

[Heiner Kallweit]

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;
>>
> 
> 

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

[Andrew Lunn]

> > 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.

I'm crystal ball gazing, but i think Florian was meaning something like

#define PHY_ID_UNIQUE(_id) \
	.phy_id = _id_; \
	.phy_id_mask = 0xffffffff;

It is the boilerplate setting .phy_id_mask which you don't like. This removes that boilerplate.

	Andrew

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

[David Miller]

From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Wed, 7 Nov 2018 21:52:31 +0100

> 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.
> 
> Realtek PHY driver is the first user of this change.

It looks like there will be some changes to this series, so I will
wait for the next version.

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

[Florian Fainelli]

On 11/8/18 12:53 PM, Andrew Lunn wrote:
>>> 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.
> 
> I'm crystal ball gazing, but i think Florian was meaning something like
> 
> #define PHY_ID_UNIQUE(_id) \
> 	.phy_id = _id_; \
> 	.phy_id_mask = 0xffffffff;
> 
> It is the boilerplate setting .phy_id_mask which you don't like. This removes that boilerplate.

Your crystal ball gazing skills are good, that is what I meant, we could
also define another macro which does not match the revision bits, and
that would likely cover everything that is already out there.
-- 
Florian