[PATCH net-next] net: phy: add helpers for comparing phy IDs

[PATCH net-next] net: phy: add helpers for comparing phy IDs

[Russell King]

There are several places which open code comparing PHY IDs. Provide a
couple of helpers to assist with this, using a slightly simpler test
than the original:

- phy_id_compare() compares two arbitary PHY IDs and a mask of the
  significant bits in the ID.
- phydev_id_compare() compares the bound phydev with the specified
  PHY ID, using the bound driver's mask.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/micrel.c     |  6 +++---
 drivers/net/phy/phy_device.c | 16 +++++++---------
 drivers/net/phy/phylink.c    |  4 ++--
 include/linux/phy.h          | 28 ++++++++++++++++++++++++++++
 4 files changed, 40 insertions(+), 14 deletions(-)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 3f81bb8dac44..2094d49025a7 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -637,7 +637,7 @@ static int ksz8051_ksz8795_match_phy_device(struct phy_device *phydev,
 {
 	int ret;
 
-	if ((phydev->phy_id & MICREL_PHY_ID_MASK) != PHY_ID_KSZ8051)
+	if (!phy_id_compare(phydev->phy_id, PHY_ID_KSZ8051, MICREL_PHY_ID_MASK))
 		return 0;
 
 	ret = phy_read(phydev, MII_BMSR);
@@ -1566,7 +1566,7 @@ static int ksz9x31_cable_test_fault_length(struct phy_device *phydev, u16 stat)
 	 *
 	 * distance to fault = (VCT_DATA - 22) * 4 / cable propagation velocity
 	 */
-	if ((phydev->phy_id & MICREL_PHY_ID_MASK) == PHY_ID_KSZ9131)
+	if (phydev_id_compare(phydev, PHY_ID_KSZ9131))
 		dt = clamp(dt - 22, 0, 255);
 
 	return (dt * 400) / 10;
@@ -1998,7 +1998,7 @@ static __always_inline int ksz886x_cable_test_fault_length(struct phy_device *ph
 	 */
 	dt = FIELD_GET(data_mask, status);
 
-	if ((phydev->phy_id & MICREL_PHY_ID_MASK) == PHY_ID_LAN8814)
+	if (phydev_id_compare(phydev, PHY_ID_LAN8814))
 		return ((dt - 22) * 800) / 10;
 	else
 		return (dt * 400) / 10;
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 8852b0c53114..2cad9cc3f6b8 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -454,8 +454,7 @@ int phy_unregister_fixup(const char *bus_id, u32 phy_uid, u32 phy_uid_mask)
 		fixup = list_entry(pos, struct phy_fixup, list);
 
 		if ((!strcmp(fixup->bus_id, bus_id)) &&
-		    ((fixup->phy_uid & phy_uid_mask) ==
-		     (phy_uid & phy_uid_mask))) {
+		    phy_id_compare(fixup->phy_uid, phy_uid, phy_uid_mask)) {
 			list_del(&fixup->list);
 			kfree(fixup);
 			ret = 0;
@@ -491,8 +490,8 @@ static int phy_needs_fixup(struct phy_device *phydev, struct phy_fixup *fixup)
 		if (strcmp(fixup->bus_id, PHY_ANY_ID) != 0)
 			return 0;
 
-	if ((fixup->phy_uid & fixup->phy_uid_mask) !=
-	    (phydev->phy_id & fixup->phy_uid_mask))
+	if (!phy_id_compare(phydev->phy_id, fixup->phy_uid,
+			    fixup->phy_uid_mask))
 		if (fixup->phy_uid != PHY_ANY_UID)
 			return 0;
 
@@ -539,15 +538,14 @@ static int phy_bus_match(struct device *dev, struct device_driver *drv)
 			if (phydev->c45_ids.device_ids[i] == 0xffffffff)
 				continue;
 
-			if ((phydrv->phy_id & phydrv->phy_id_mask) ==
-			    (phydev->c45_ids.device_ids[i] &
-			     phydrv->phy_id_mask))
+			if (phy_id_compare(phydev->c45_ids.device_ids[i],
+					   phydrv->phy_id, phydrv->phy_id_mask))
 				return 1;
 		}
 		return 0;
 	} else {
-		return (phydrv->phy_id & phydrv->phy_id_mask) ==
-			(phydev->phy_id & phydrv->phy_id_mask);
+		return phy_id_compare(phydev->phy_id, phydrv->phy_id,
+				      phydrv->phy_id_mask);
 	}
 }
 
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index ee7e06718983..095c601985e7 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -3179,8 +3179,8 @@ static void phylink_sfp_link_up(void *upstream)
  */
 static bool phylink_phy_no_inband(struct phy_device *phy)
 {
-	return phy->is_c45 &&
-		(phy->c45_ids.device_ids[1] & 0xfffffff0) == 0xae025150;
+	return phy->is_c45 && phy_id_compare(phy->c45_ids.device_ids[1],
+					     0xae025150, 0xfffffff0);
 }
 
 static int phylink_sfp_connect_phy(void *upstream, struct phy_device *phy)
diff --git a/include/linux/phy.h b/include/linux/phy.h
index d8cd7115c773..2da87a36200d 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1112,6 +1112,34 @@ struct phy_driver {
 #define PHY_ID_MATCH_MODEL(id) .phy_id = (id), .phy_id_mask = GENMASK(31, 4)
 #define PHY_ID_MATCH_VENDOR(id) .phy_id = (id), .phy_id_mask = GENMASK(31, 10)
 
+/**
+ * phy_id_compare - compare @id1 with @id2 taking account of @mask
+ * @id1: first PHY ID
+ * @id2: second PHY ID
+ * @mask: the PHY ID mask, set bits are significant in matching
+ *
+ * Return true if the bits from @id1 and @id2 specified by @mask match.
+ * This uses an equivalent test to (@id & @mask) == (@phy_id & @mask).
+ */
+static inline bool phy_id_compare(u32 id1, u32 id2, u32 mask)
+{
+	return !((id1 ^ id2) & mask);
+}
+
+/**
+ * phydev_id_compare - compare @id with the PHY's Clause 22 ID
+ * @phydev: the PHY device
+ * @id: the PHY ID to be matched
+ *
+ * Compare the @phydev clause 22 ID with the provided @id and return true or
+ * false depending whether it matches, using the bound driver mask. The
+ * @phydev must be bound to a driver.
+ */
+static inline bool phydev_id_compare(struct phy_device *phydev, u32 id)
+{
+	return phy_id_compare(id, phydev->phy_id, phydev->drv->phy_id_mask);
+}
+
 /* A Structure for boards to register fixups with the PHY Lib */
 struct phy_fixup {
 	struct list_head list;
-- 
2.30.2

Re: [PATCH net-next] net: phy: add helpers for comparing phy IDs

[Simon Horman]

On Fri, May 19, 2023 at 02:03:59PM +0100, Russell King wrote:
> There are several places which open code comparing PHY IDs. Provide a
> couple of helpers to assist with this, using a slightly simpler test
> than the original:
> 
> - phy_id_compare() compares two arbitary PHY IDs and a mask of the
>   significant bits in the ID.
> - phydev_id_compare() compares the bound phydev with the specified
>   PHY ID, using the bound driver's mask.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Thanks Russell,

Reviewed-by: Simon Horman <simon.horman@corigine.com>

...

> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index d8cd7115c773..2da87a36200d 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -1112,6 +1112,34 @@ struct phy_driver {
>  #define PHY_ID_MATCH_MODEL(id) .phy_id = (id), .phy_id_mask = GENMASK(31, 4)
>  #define PHY_ID_MATCH_VENDOR(id) .phy_id = (id), .phy_id_mask = GENMASK(31, 10)
>  
> +/**
> + * phy_id_compare - compare @id1 with @id2 taking account of @mask
> + * @id1: first PHY ID
> + * @id2: second PHY ID
> + * @mask: the PHY ID mask, set bits are significant in matching
> + *
> + * Return true if the bits from @id1 and @id2 specified by @mask match.
> + * This uses an equivalent test to (@id & @mask) == (@phy_id & @mask).
> + */
> +static inline bool phy_id_compare(u32 id1, u32 id2, u32 mask)
> +{
> +	return !((id1 ^ id2) & mask);
> +}

I wonder if, at some point, it would be worth generalising this further.
It seems likely such an operation is used outside of the context of phy_id.

Re: [PATCH net-next] net: phy: add helpers for comparing phy IDs

[Andrew Lunn]

On Fri, May 19, 2023 at 02:03:59PM +0100, Russell King wrote:
> There are several places which open code comparing PHY IDs. Provide a
> couple of helpers to assist with this, using a slightly simpler test
> than the original:
> 
> - phy_id_compare() compares two arbitary PHY IDs and a mask of the
>   significant bits in the ID.
> - phydev_id_compare() compares the bound phydev with the specified
>   PHY ID, using the bound driver's mask.

Hi Russell

I think these are useful, but i'm wondering about naming. In the PHY
drivers we use these macros:

#define PHY_ID_MATCH_EXACT(id) .phy_id = (id), .phy_id_mask = GENMASK(31, 0)
#define PHY_ID_MATCH_MODEL(id) .phy_id = (id), .phy_id_mask = GENMASK(31, 4)
#define PHY_ID_MATCH_VENDOR(id) .phy_id = (id), .phy_id_mask = GENMASK(31, 10)

when creating the tables. 

> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> index 3f81bb8dac44..2094d49025a7 100644
> --- a/drivers/net/phy/micrel.c
> +++ b/drivers/net/phy/micrel.c
> @@ -637,7 +637,7 @@ static int ksz8051_ksz8795_match_phy_device(struct phy_device *phydev,
>  {
>  	int ret;
>  
> -	if ((phydev->phy_id & MICREL_PHY_ID_MASK) != PHY_ID_KSZ8051)
> +	if (!phy_id_compare(phydev->phy_id, PHY_ID_KSZ8051, MICREL_PHY_ID_MASK))
>  		return 0;

This could be phy_id_compare_model() 

phy_id_compare_exact() could be used in a number of places, eg.

vitesse.c:			(phydev->drv->phy_id == PHY_ID_VSC8234 ||
vitesse.c:			 phydev->drv->phy_id == PHY_ID_VSC8244 ||
vitesse.c:			 phydev->drv->phy_id == PHY_ID_VSC8572 ||
vitesse.c:			 phydev->drv->phy_id == PHY_ID_VSC8601) ?
motorcomm.c:	} else if (phydev->drv->phy_id == PHY_ID_YT8531S) {
marvell10g.c:	if (phydev->drv->phy_id != MARVELL_PHY_ID_88X3310)

etc.

	Andrew

Re: [PATCH net-next] net: phy: add helpers for comparing phy IDs

[Russell King (Oracle)]

On Fri, May 19, 2023 at 05:59:02PM +0200, Andrew Lunn wrote:
> On Fri, May 19, 2023 at 02:03:59PM +0100, Russell King wrote:
> > There are several places which open code comparing PHY IDs. Provide a
> > couple of helpers to assist with this, using a slightly simpler test
> > than the original:
> > 
> > - phy_id_compare() compares two arbitary PHY IDs and a mask of the
> >   significant bits in the ID.
> > - phydev_id_compare() compares the bound phydev with the specified
> >   PHY ID, using the bound driver's mask.
> 
> Hi Russell
> 
> I think these are useful, but i'm wondering about naming. In the PHY
> drivers we use these macros:
> 
> #define PHY_ID_MATCH_EXACT(id) .phy_id = (id), .phy_id_mask = GENMASK(31, 0)
> #define PHY_ID_MATCH_MODEL(id) .phy_id = (id), .phy_id_mask = GENMASK(31, 4)
> #define PHY_ID_MATCH_VENDOR(id) .phy_id = (id), .phy_id_mask = GENMASK(31, 10)
> 
> when creating the tables. 
> 
> > diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> > index 3f81bb8dac44..2094d49025a7 100644
> > --- a/drivers/net/phy/micrel.c
> > +++ b/drivers/net/phy/micrel.c
> > @@ -637,7 +637,7 @@ static int ksz8051_ksz8795_match_phy_device(struct phy_device *phydev,
> >  {
> >  	int ret;
> >  
> > -	if ((phydev->phy_id & MICREL_PHY_ID_MASK) != PHY_ID_KSZ8051)
> > +	if (!phy_id_compare(phydev->phy_id, PHY_ID_KSZ8051, MICREL_PHY_ID_MASK))
> >  		return 0;
> 
> This could be phy_id_compare_model() 

Note that this can't access phydev->drv, because this is a match method
and in all likelyhood, phydev->drv is NULL here.

> phy_id_compare_exact() could be used in a number of places, eg.
> 
> vitesse.c:			(phydev->drv->phy_id == PHY_ID_VSC8234 ||
> vitesse.c:			 phydev->drv->phy_id == PHY_ID_VSC8244 ||
> vitesse.c:			 phydev->drv->phy_id == PHY_ID_VSC8572 ||
> vitesse.c:			 phydev->drv->phy_id == PHY_ID_VSC8601) ?
> motorcomm.c:	} else if (phydev->drv->phy_id == PHY_ID_YT8531S) {
> marvell10g.c:	if (phydev->drv->phy_id != MARVELL_PHY_ID_88X3310)

However, these are not comparing the PHY ID, they're comparing the
driver's idea of the PHY ID. I would expect phy_id_compare_exact()
to be comparing phydev->phy_id!

Many places can use phydev->drv->phy_id == foo to identify which
driver was matched. If the driver's phy_id is set my a macro
definition, then that same macro identifier can be used with a
straight compare like the above, and I think open coding those
like that is entirely reasonable and proper.

However, phydev->phy_id is much more complex because it generally
needs the revision masking away, which is what these helpers are
achieving.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next] net: phy: add helpers for comparing phy IDs

[Andrew Lunn]

On Fri, May 19, 2023 at 02:03:59PM +0100, Russell King wrote:
> There are several places which open code comparing PHY IDs. Provide a
> couple of helpers to assist with this, using a slightly simpler test
> than the original:
> 
> - phy_id_compare() compares two arbitary PHY IDs and a mask of the
>   significant bits in the ID.
> - phydev_id_compare() compares the bound phydev with the specified
>   PHY ID, using the bound driver's mask.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next] net: phy: add helpers for comparing phy IDs

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by David S. Miller <davem@davemloft.net>:

On Fri, 19 May 2023 14:03:59 +0100 you wrote:
> There are several places which open code comparing PHY IDs. Provide a
> couple of helpers to assist with this, using a slightly simpler test
> than the original:
> 
> - phy_id_compare() compares two arbitary PHY IDs and a mask of the
>   significant bits in the ID.
> - phydev_id_compare() compares the bound phydev with the specified
>   PHY ID, using the bound driver's mask.
> 
> [...]

Here is the summary with links:
  - [net-next] net: phy: add helpers for comparing phy IDs
    https://git.kernel.org/netdev/net-next/c/4b159f5048b9

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html