[PATCH net-next v5 00/13] net: phy: Rework linkmodes handling in a dedicated file

[PATCH net-next v5 00/13] net: phy: Rework linkmodes handling in a dedicated file

[Maxime Chevallier]

Hello everyone,

This is V5 of the phy_caps series. In a nutshell, this series reworks the way
we maintain the list of speed/duplex capablities for each linkmode so that we
no longer have multiple definition of these associations.

That will help making sure that when people add new linkmodes in
include/uapi/linux/ethtool.h, they don't have to update phylib and phylink as
well, making the process more straightforward and less error-prone.

It also generalises the phy_caps interface to be able to lookup linkmodes
from phy_interface_t, which is needed for the multi-port work I've been working
on for a while.

This V5 addresse Russell's and Paolo's reviews, namely :

 - Error out when encountering an unknown SPEED_XXX setting

   It prints an error and fails to initialize phylib. I've tested by
   introducing a dummy 1.6T speed, I guess it's only a matter of time
   before that actually happens :)

 - Deal more gracefully with the fixed-link settings, keeping some level of
   compatibility with what we had before by making sure we report a
   single BaseT mode like before.

V1 : https://lore.kernel.org/netdev/20250222142727.894124-1-maxime.chevallier@bootlin.com/
V2 : https://lore.kernel.org/netdev/20250226100929.1646454-1-maxime.chevallier@bootlin.com/
V3 : https://lore.kernel.org/netdev/20250228145540.2209551-1-maxime.chevallier@bootlin.com/
V4 : https://lore.kernel.org/netdev/20250303090321.805785-1-maxime.chevallier@bootlin.com/

Maxime Chevallier (13):
  net: ethtool: Export the link_mode_params definitions
  net: phy: Use an internal, searchable storage for the linkmodes
  net: phy: phy_caps: Move phy_speeds to phy_caps
  net: phy: phy_caps: Move __set_linkmode_max_speed to phy_caps
  net: phy: phy_caps: Introduce phy_caps_valid
  net: phy: phy_caps: Implement link_capabilities lookup by linkmode
  net: phy: phy_caps: Allow looking-up link caps based on speed and
    duplex
  net: phy: phy_device: Use link_capabilities lookup for PHY aneg config
  net: phylink: Use phy_caps_lookup for fixed-link configuration
  net: phy: drop phy_settings and the associated lookup helpers
  net: phylink: Add a mapping between MAC_CAPS and LINK_CAPS
  net: phylink: Convert capabilities to linkmodes using phy_caps
  net: phylink: Use phy_caps to get an interface's capabilities and
    modes

 drivers/net/phy/Makefile     |   2 +-
 drivers/net/phy/phy-caps.h   |  63 ++++++
 drivers/net/phy/phy-core.c   | 253 ++----------------------
 drivers/net/phy/phy.c        |  37 +---
 drivers/net/phy/phy_caps.c   | 359 +++++++++++++++++++++++++++++++++++
 drivers/net/phy/phy_device.c |  16 +-
 drivers/net/phy/phylink.c    | 355 ++++++++++------------------------
 include/linux/ethtool.h      |   8 +
 include/linux/phy.h          |  15 --
 net/ethtool/common.c         |   1 +
 net/ethtool/common.h         |   7 -
 11 files changed, 571 insertions(+), 545 deletions(-)
 create mode 100644 drivers/net/phy/phy-caps.h
 create mode 100644 drivers/net/phy/phy_caps.c

-- 
2.48.1

[PATCH net-next v5 01/13] net: ethtool: Export the link_mode_params definitions

[Maxime Chevallier]

link_mode_params contains a lookup table of all 802.3 link modes that
are currently supported with structured data about each mode's speed,
duplex, number of lanes and mediums.

As a preparation for a port representation, export that table for the
rest of the net stack to use.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 include/linux/ethtool.h | 8 ++++++++
 net/ethtool/common.c    | 1 +
 net/ethtool/common.h    | 7 -------
 3 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 7f222dccc7d1..8210ece94fa6 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -210,6 +210,14 @@ static inline u8 *ethtool_rxfh_context_key(struct ethtool_rxfh_context *ctx)
 
 void ethtool_rxfh_context_lost(struct net_device *dev, u32 context_id);
 
+struct link_mode_info {
+	int                             speed;
+	u8                              lanes;
+	u8                              duplex;
+};
+
+extern const struct link_mode_info link_mode_params[];
+
 /* declare a link mode bitmap */
 #define __ETHTOOL_DECLARE_LINK_MODE_MASK(name)		\
 	DECLARE_BITMAP(name, __ETHTOOL_LINK_MODE_MASK_NBITS)
diff --git a/net/ethtool/common.c b/net/ethtool/common.c
index ac8b6107863e..c9d6302e88c9 100644
--- a/net/ethtool/common.c
+++ b/net/ethtool/common.c
@@ -423,6 +423,7 @@ const struct link_mode_info link_mode_params[] = {
 	__DEFINE_LINK_MODE_PARAMS(800000, VR4, Full),
 };
 static_assert(ARRAY_SIZE(link_mode_params) == __ETHTOOL_LINK_MODE_MASK_NBITS);
+EXPORT_SYMBOL_GPL(link_mode_params);
 
 const char netif_msg_class_names[][ETH_GSTRING_LEN] = {
 	[NETIF_MSG_DRV_BIT]		= "drv",
diff --git a/net/ethtool/common.h b/net/ethtool/common.h
index a1088c2441d0..b4683d286a5a 100644
--- a/net/ethtool/common.h
+++ b/net/ethtool/common.h
@@ -15,12 +15,6 @@
 #define __SOF_TIMESTAMPING_CNT (const_ilog2(SOF_TIMESTAMPING_LAST) + 1)
 #define __HWTSTAMP_FLAG_CNT (const_ilog2(HWTSTAMP_FLAG_LAST) + 1)
 
-struct link_mode_info {
-	int				speed;
-	u8				lanes;
-	u8				duplex;
-};
-
 struct genl_info;
 struct hwtstamp_provider_desc;
 
@@ -33,7 +27,6 @@ tunable_strings[__ETHTOOL_TUNABLE_COUNT][ETH_GSTRING_LEN];
 extern const char
 phy_tunable_strings[__ETHTOOL_PHY_TUNABLE_COUNT][ETH_GSTRING_LEN];
 extern const char link_mode_names[][ETH_GSTRING_LEN];
-extern const struct link_mode_info link_mode_params[];
 extern const char netif_msg_class_names[][ETH_GSTRING_LEN];
 extern const char wol_mode_names[][ETH_GSTRING_LEN];
 extern const char sof_timestamping_names[][ETH_GSTRING_LEN];
-- 
2.48.1

[PATCH net-next v5 02/13] net: phy: Use an internal, searchable storage for the linkmodes

[Maxime Chevallier]

The canonical definition for all the link modes is in linux/ethtool.h,
which is complemented by the link_mode_params array stored in
net/ethtool/common.h . That array contains all the metadata about each
of these modes, including the Speed and Duplex information.

Phylib and phylink needs that information as well for internal
management of the link, which was done by duplicating that information
in locally-stored arrays and lookup functions. This makes it easy for
developpers adding new modes to forget modifying phylib and phylink
accordingly.

However, the link_mode_params array in net/ethtool/common.c is fairly
inefficient to search through, as it isn't sorted in any manner. Phylib
and phylink perform a lot of lookup operations, mostly to filter modes
by speed and/or duplex.

We therefore introduce the link_caps private array in phy_caps.c, that
indexes linkmodes in a more efficient manner. Each element associated a
tuple <speed, duplex> to a bitfield of all the linkmodes runs at these
speed/duplex.

We end-up with an array that's fairly short, easily addressable and that
it optimised for the typical use-cases of phylib/phylink.

That array is initialized at the same time as phylib. As the
link_mode_params array is part of the net stack, which phylink depends
on, it should always be accessible from phylib.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
V5: Fail on unknown speed

 drivers/net/phy/Makefile     |  2 +-
 drivers/net/phy/phy-caps.h   | 43 +++++++++++++++++
 drivers/net/phy/phy_caps.c   | 90 ++++++++++++++++++++++++++++++++++++
 drivers/net/phy/phy_device.c |  5 ++
 4 files changed, 139 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/phy/phy-caps.h
 create mode 100644 drivers/net/phy/phy_caps.c

diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index 8f9ba5e8290d..23ce205ae91d 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -3,7 +3,7 @@
 
 libphy-y			:= phy.o phy-c45.o phy-core.o phy_device.o \
 				   linkmode.o phy_link_topology.o \
-				   phy_package.o
+				   phy_package.o phy_caps.o
 mdio-bus-y			+= mdio_bus.o mdio_device.o
 
 ifdef CONFIG_MDIO_DEVICE
diff --git a/drivers/net/phy/phy-caps.h b/drivers/net/phy/phy-caps.h
new file mode 100644
index 000000000000..6024f1d11a93
--- /dev/null
+++ b/drivers/net/phy/phy-caps.h
@@ -0,0 +1,43 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * link caps internal header, for link modes <-> capabilities <-> interfaces
+ * conversions.
+ */
+
+#ifndef __PHY_CAPS_H
+#define __PHY_CAPS_H
+
+#include <linux/ethtool.h>
+
+enum {
+	LINK_CAPA_10HD = 0,
+	LINK_CAPA_10FD,
+	LINK_CAPA_100HD,
+	LINK_CAPA_100FD,
+	LINK_CAPA_1000HD,
+	LINK_CAPA_1000FD,
+	LINK_CAPA_2500FD,
+	LINK_CAPA_5000FD,
+	LINK_CAPA_10000FD,
+	LINK_CAPA_20000FD,
+	LINK_CAPA_25000FD,
+	LINK_CAPA_40000FD,
+	LINK_CAPA_50000FD,
+	LINK_CAPA_56000FD,
+	LINK_CAPA_100000FD,
+	LINK_CAPA_200000FD,
+	LINK_CAPA_400000FD,
+	LINK_CAPA_800000FD,
+
+	__LINK_CAPA_MAX,
+};
+
+struct link_capabilities {
+	int speed;
+	unsigned int duplex;
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(linkmodes);
+};
+
+int phy_caps_init(void);
+
+#endif /* __PHY_CAPS_H */
diff --git a/drivers/net/phy/phy_caps.c b/drivers/net/phy/phy_caps.c
new file mode 100644
index 000000000000..6cb18e216d97
--- /dev/null
+++ b/drivers/net/phy/phy_caps.c
@@ -0,0 +1,90 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#include <linux/ethtool.h>
+#include <linux/linkmode.h>
+#include <linux/phy.h>
+
+#include "phy-caps.h"
+
+static struct link_capabilities link_caps[__LINK_CAPA_MAX] __ro_after_init = {
+	{ SPEED_10, DUPLEX_HALF, {0} }, /* LINK_CAPA_10HD */
+	{ SPEED_10, DUPLEX_FULL, {0} }, /* LINK_CAPA_10FD */
+	{ SPEED_100, DUPLEX_HALF, {0} }, /* LINK_CAPA_100HD */
+	{ SPEED_100, DUPLEX_FULL, {0} }, /* LINK_CAPA_100FD */
+	{ SPEED_1000, DUPLEX_HALF, {0} }, /* LINK_CAPA_1000HD */
+	{ SPEED_1000, DUPLEX_FULL, {0} }, /* LINK_CAPA_1000FD */
+	{ SPEED_2500, DUPLEX_FULL, {0} }, /* LINK_CAPA_2500FD */
+	{ SPEED_5000, DUPLEX_FULL, {0} }, /* LINK_CAPA_5000FD */
+	{ SPEED_10000, DUPLEX_FULL, {0} }, /* LINK_CAPA_10000FD */
+	{ SPEED_20000, DUPLEX_FULL, {0} }, /* LINK_CAPA_20000FD */
+	{ SPEED_25000, DUPLEX_FULL, {0} }, /* LINK_CAPA_25000FD */
+	{ SPEED_40000, DUPLEX_FULL, {0} }, /* LINK_CAPA_40000FD */
+	{ SPEED_50000, DUPLEX_FULL, {0} }, /* LINK_CAPA_50000FD */
+	{ SPEED_56000, DUPLEX_FULL, {0} }, /* LINK_CAPA_56000FD */
+	{ SPEED_100000, DUPLEX_FULL, {0} }, /* LINK_CAPA_100000FD */
+	{ SPEED_200000, DUPLEX_FULL, {0} }, /* LINK_CAPA_200000FD */
+	{ SPEED_400000, DUPLEX_FULL, {0} }, /* LINK_CAPA_400000FD */
+	{ SPEED_800000, DUPLEX_FULL, {0} }, /* LINK_CAPA_800000FD */
+};
+
+static int speed_duplex_to_capa(int speed, unsigned int duplex)
+{
+	if (duplex == DUPLEX_UNKNOWN ||
+	    (speed > SPEED_1000 && duplex != DUPLEX_FULL))
+		return -EINVAL;
+
+	switch (speed) {
+	case SPEED_10: return duplex == DUPLEX_FULL ?
+			      LINK_CAPA_10FD : LINK_CAPA_10HD;
+	case SPEED_100: return duplex == DUPLEX_FULL ?
+			       LINK_CAPA_100FD : LINK_CAPA_100HD;
+	case SPEED_1000: return duplex == DUPLEX_FULL ?
+				LINK_CAPA_1000FD : LINK_CAPA_1000HD;
+	case SPEED_2500: return LINK_CAPA_2500FD;
+	case SPEED_5000: return LINK_CAPA_5000FD;
+	case SPEED_10000: return LINK_CAPA_10000FD;
+	case SPEED_20000: return LINK_CAPA_20000FD;
+	case SPEED_25000: return LINK_CAPA_25000FD;
+	case SPEED_40000: return LINK_CAPA_40000FD;
+	case SPEED_50000: return LINK_CAPA_50000FD;
+	case SPEED_56000: return LINK_CAPA_56000FD;
+	case SPEED_100000: return LINK_CAPA_100000FD;
+	case SPEED_200000: return LINK_CAPA_200000FD;
+	case SPEED_400000: return LINK_CAPA_400000FD;
+	case SPEED_800000: return LINK_CAPA_800000FD;
+	}
+
+	return -EINVAL;
+}
+
+/**
+ * phy_caps_init() - Initializes the link_caps array from the link_mode_params.
+ *
+ * Returns: 0 if phy caps init was successful, -EINVAL if we found an
+ *	    unexpected linkmode setting that requires LINK_CAPS update.
+ *
+ */
+int phy_caps_init(void)
+{
+	const struct link_mode_info *linkmode;
+	int i, capa;
+
+	/* Fill the caps array from net/ethtool/common.c */
+	for (i = 0; i < __ETHTOOL_LINK_MODE_MASK_NBITS; i++) {
+		linkmode = &link_mode_params[i];
+		capa = speed_duplex_to_capa(linkmode->speed, linkmode->duplex);
+
+		if (capa < 0) {
+			if (linkmode->speed != SPEED_UNKNOWN) {
+				pr_err("Unknown speed %d, please update LINK_CAPS\n",
+				       linkmode->speed);
+				return -EINVAL;
+			}
+			continue;
+		}
+
+		__set_bit(i, link_caps[capa].linkmodes);
+	}
+
+	return 0;
+}
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index b2d32fbc8c85..4980862398d0 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -42,6 +42,7 @@
 #include <linux/unistd.h>
 
 #include "phylib-internal.h"
+#include "phy-caps.h"
 
 MODULE_DESCRIPTION("PHY library");
 MODULE_AUTHOR("Andy Fleming");
@@ -3558,6 +3559,10 @@ static int __init phy_init(void)
 	if (rc)
 		goto err_ethtool_phy_ops;
 
+	rc = phy_caps_init();
+	if (rc)
+		goto err_mdio_bus;
+
 	features_init();
 
 	rc = phy_driver_register(&genphy_c45_driver, THIS_MODULE);
-- 
2.48.1

[PATCH net-next v5 03/13] net: phy: phy_caps: Move phy_speeds to phy_caps

[Maxime Chevallier]

Use the newly introduced link_capabilities array to derive the list of
possible speeds when given a combination of linkmodes. As
link_capabilities is indexed by speed, we don't have to iterate the
whole phy_settings array.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/phy-caps.h |  3 +++
 drivers/net/phy/phy-core.c | 15 ---------------
 drivers/net/phy/phy.c      |  3 ++-
 drivers/net/phy/phy_caps.c | 33 +++++++++++++++++++++++++++++++++
 include/linux/phy.h        |  2 --
 5 files changed, 38 insertions(+), 18 deletions(-)

diff --git a/drivers/net/phy/phy-caps.h b/drivers/net/phy/phy-caps.h
index 6024f1d11a93..5a5f4ee3006b 100644
--- a/drivers/net/phy/phy-caps.h
+++ b/drivers/net/phy/phy-caps.h
@@ -40,4 +40,7 @@ struct link_capabilities {
 
 int phy_caps_init(void);
 
+size_t phy_caps_speeds(unsigned int *speeds, size_t size,
+		       unsigned long *linkmodes);
+
 #endif /* __PHY_CAPS_H */
diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
index 154d29be6293..542e9284d207 100644
--- a/drivers/net/phy/phy-core.c
+++ b/drivers/net/phy/phy-core.c
@@ -340,21 +340,6 @@ phy_lookup_setting(int speed, int duplex, const unsigned long *mask, bool exact)
 }
 EXPORT_SYMBOL_GPL(phy_lookup_setting);
 
-size_t phy_speeds(unsigned int *speeds, size_t size,
-		  unsigned long *mask)
-{
-	size_t count;
-	int i;
-
-	for (i = 0, count = 0; i < ARRAY_SIZE(settings) && count < size; i++)
-		if (settings[i].bit < __ETHTOOL_LINK_MODE_MASK_NBITS &&
-		    test_bit(settings[i].bit, mask) &&
-		    (count == 0 || speeds[count - 1] != settings[i].speed))
-			speeds[count++] = settings[i].speed;
-
-	return count;
-}
-
 static void __set_linkmode_max_speed(u32 max_speed, unsigned long *addr)
 {
 	const struct phy_setting *p;
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 16ffc00b419c..3128df03feda 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -37,6 +37,7 @@
 #include <net/sock.h>
 
 #include "phylib-internal.h"
+#include "phy-caps.h"
 
 #define PHY_STATE_TIME	HZ
 
@@ -245,7 +246,7 @@ unsigned int phy_supported_speeds(struct phy_device *phy,
 				  unsigned int *speeds,
 				  unsigned int size)
 {
-	return phy_speeds(speeds, size, phy->supported);
+	return phy_caps_speeds(speeds, size, phy->supported);
 }
 
 /**
diff --git a/drivers/net/phy/phy_caps.c b/drivers/net/phy/phy_caps.c
index 6cb18e216d97..8ce91257160f 100644
--- a/drivers/net/phy/phy_caps.c
+++ b/drivers/net/phy/phy_caps.c
@@ -57,6 +57,9 @@ static int speed_duplex_to_capa(int speed, unsigned int duplex)
 	return -EINVAL;
 }
 
+#define for_each_link_caps_asc_speed(cap) \
+	for (cap = link_caps; cap < &link_caps[__LINK_CAPA_MAX]; cap++)
+
 /**
  * phy_caps_init() - Initializes the link_caps array from the link_mode_params.
  *
@@ -88,3 +91,33 @@ int phy_caps_init(void)
 
 	return 0;
 }
+
+/**
+ * phy_caps_speeds() - Fill an array of supported SPEED_* values for given modes
+ * @speeds: Output array to store the speeds list into
+ * @size: Size of the output array
+ * @linkmodes: Linkmodes to get the speeds from
+ *
+ * Fills the speeds array with all possible speeds that can be achieved with
+ * the specified linkmodes.
+ *
+ * Returns: The number of speeds filled into the array. If the input array isn't
+ *	    big enough to store all speeds, fill it as much as possible.
+ */
+size_t phy_caps_speeds(unsigned int *speeds, size_t size,
+		       unsigned long *linkmodes)
+{
+	struct link_capabilities *lcap;
+	size_t count = 0;
+
+	for_each_link_caps_asc_speed(lcap) {
+		if (linkmode_intersects(lcap->linkmodes, linkmodes) &&
+		    (count == 0 || speeds[count - 1] != lcap->speed)) {
+			speeds[count++] = lcap->speed;
+			if (count >= size)
+				break;
+		}
+	}
+
+	return count;
+}
diff --git a/include/linux/phy.h b/include/linux/phy.h
index c4a6385faf41..99c195fa3695 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1287,8 +1287,6 @@ struct phy_setting {
 const struct phy_setting *
 phy_lookup_setting(int speed, int duplex, const unsigned long *mask,
 		   bool exact);
-size_t phy_speeds(unsigned int *speeds, size_t size,
-		  unsigned long *mask);
 
 /**
  * phy_is_started - Convenience function to check whether PHY is started
-- 
2.48.1

[PATCH net-next v5 04/13] net: phy: phy_caps: Move __set_linkmode_max_speed to phy_caps

[Maxime Chevallier]

Convert the __set_linkmode_max_speed to use the link_capabilities array.
This makes it easy to clamp the linkmodes to a given max speed.
Introduce a new helper phy_caps_linkmode_max_speed to replace the
previous one that used phy_settings.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/phy-caps.h |  2 ++
 drivers/net/phy/phy-core.c | 18 +++---------------
 drivers/net/phy/phy_caps.c | 19 +++++++++++++++++++
 3 files changed, 24 insertions(+), 15 deletions(-)

diff --git a/drivers/net/phy/phy-caps.h b/drivers/net/phy/phy-caps.h
index 5a5f4ee3006b..1bef9cca62c5 100644
--- a/drivers/net/phy/phy-caps.h
+++ b/drivers/net/phy/phy-caps.h
@@ -42,5 +42,7 @@ int phy_caps_init(void);
 
 size_t phy_caps_speeds(unsigned int *speeds, size_t size,
 		       unsigned long *linkmodes);
+void phy_caps_linkmode_max_speed(u32 max_speed, unsigned long *linkmodes);
+
 
 #endif /* __PHY_CAPS_H */
diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
index 542e9284d207..af2463e67cea 100644
--- a/drivers/net/phy/phy-core.c
+++ b/drivers/net/phy/phy-core.c
@@ -8,6 +8,7 @@
 
 #include "phylib.h"
 #include "phylib-internal.h"
+#include "phy-caps.h"
 
 /**
  * phy_speed_to_str - Return a string representing the PHY link speed
@@ -340,22 +341,9 @@ phy_lookup_setting(int speed, int duplex, const unsigned long *mask, bool exact)
 }
 EXPORT_SYMBOL_GPL(phy_lookup_setting);
 
-static void __set_linkmode_max_speed(u32 max_speed, unsigned long *addr)
-{
-	const struct phy_setting *p;
-	int i;
-
-	for (i = 0, p = settings; i < ARRAY_SIZE(settings); i++, p++) {
-		if (p->speed > max_speed)
-			linkmode_clear_bit(p->bit, addr);
-		else
-			break;
-	}
-}
-
 static void __set_phy_supported(struct phy_device *phydev, u32 max_speed)
 {
-	__set_linkmode_max_speed(max_speed, phydev->supported);
+	phy_caps_linkmode_max_speed(max_speed, phydev->supported);
 }
 
 /**
@@ -558,7 +546,7 @@ int phy_speed_down_core(struct phy_device *phydev)
 	if (min_common_speed == SPEED_UNKNOWN)
 		return -EINVAL;
 
-	__set_linkmode_max_speed(min_common_speed, phydev->advertising);
+	phy_caps_linkmode_max_speed(min_common_speed, phydev->advertising);
 
 	return 0;
 }
diff --git a/drivers/net/phy/phy_caps.c b/drivers/net/phy/phy_caps.c
index 8ce91257160f..d43493884ff7 100644
--- a/drivers/net/phy/phy_caps.c
+++ b/drivers/net/phy/phy_caps.c
@@ -60,6 +60,9 @@ static int speed_duplex_to_capa(int speed, unsigned int duplex)
 #define for_each_link_caps_asc_speed(cap) \
 	for (cap = link_caps; cap < &link_caps[__LINK_CAPA_MAX]; cap++)
 
+#define for_each_link_caps_desc_speed(cap) \
+	for (cap = &link_caps[__LINK_CAPA_MAX - 1]; cap >= link_caps; cap--)
+
 /**
  * phy_caps_init() - Initializes the link_caps array from the link_mode_params.
  *
@@ -121,3 +124,19 @@ size_t phy_caps_speeds(unsigned int *speeds, size_t size,
 
 	return count;
 }
+
+/**
+ * phy_caps_linkmode_max_speed() - Clamp a linkmodes set to a max speed
+ * @max_speed: Speed limit for the linkmode set
+ * @linkmodes: Linkmodes to limit
+ */
+void phy_caps_linkmode_max_speed(u32 max_speed, unsigned long *linkmodes)
+{
+	struct link_capabilities *lcap;
+
+	for_each_link_caps_desc_speed(lcap)
+		if (lcap->speed > max_speed)
+			linkmode_andnot(linkmodes, linkmodes, lcap->linkmodes);
+		else
+			break;
+}
-- 
2.48.1

[PATCH net-next v5 05/13] net: phy: phy_caps: Introduce phy_caps_valid

[Maxime Chevallier]

With the link_capabilities array, it's trivial to validate a given mask
againts a <speed, duplex> tuple. Create a helper for that purpose, and
use it to replace a phy_settings lookup in phy_check_valid();

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/phy-caps.h |  1 +
 drivers/net/phy/phy.c      |  2 +-
 drivers/net/phy/phy_caps.c | 19 +++++++++++++++++++
 3 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy-caps.h b/drivers/net/phy/phy-caps.h
index 1bef9cca62c5..1217686f1f91 100644
--- a/drivers/net/phy/phy-caps.h
+++ b/drivers/net/phy/phy-caps.h
@@ -43,6 +43,7 @@ int phy_caps_init(void);
 size_t phy_caps_speeds(unsigned int *speeds, size_t size,
 		       unsigned long *linkmodes);
 void phy_caps_linkmode_max_speed(u32 max_speed, unsigned long *linkmodes);
+bool phy_caps_valid(int speed, int duplex, const unsigned long *linkmodes);
 
 
 #endif /* __PHY_CAPS_H */
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 3128df03feda..8df37d221fba 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -260,7 +260,7 @@ unsigned int phy_supported_speeds(struct phy_device *phy,
  */
 bool phy_check_valid(int speed, int duplex, unsigned long *features)
 {
-	return !!phy_lookup_setting(speed, duplex, features, true);
+	return phy_caps_valid(speed, duplex, features);
 }
 EXPORT_SYMBOL(phy_check_valid);
 
diff --git a/drivers/net/phy/phy_caps.c b/drivers/net/phy/phy_caps.c
index d43493884ff7..4ee8c25c8521 100644
--- a/drivers/net/phy/phy_caps.c
+++ b/drivers/net/phy/phy_caps.c
@@ -140,3 +140,22 @@ void phy_caps_linkmode_max_speed(u32 max_speed, unsigned long *linkmodes)
 		else
 			break;
 }
+
+/**
+ * phy_caps_valid() - Validate a linkmodes set agains given speed and duplex
+ * @speed: input speed to validate
+ * @duplex: input duplex to validate. Passing DUPLEX_UNKNOWN is always not valid
+ * @linkmodes: The linkmodes to validate
+ *
+ * Returns: True if at least one of the linkmodes in @linkmodes can function at
+ *          the given speed and duplex, false otherwise.
+ */
+bool phy_caps_valid(int speed, int duplex, const unsigned long *linkmodes)
+{
+	int capa = speed_duplex_to_capa(speed, duplex);
+
+	if (capa < 0)
+		return false;
+
+	return linkmode_intersects(link_caps[capa].linkmodes, linkmodes);
+}
-- 
2.48.1

[PATCH net-next v5 06/13] net: phy: phy_caps: Implement link_capabilities lookup by linkmode

[Maxime Chevallier]

In several occasions, phylib needs to lookup a set of matching speed and
duplex against a given linkmode set. Instead of relying on the
phy_settings array and thus iterate over the whole linkmodes list, use
the link_capabilities array to lookup these matches, as we aren't
interested in the actual link setting that matches but rather the speed
and duplex for that setting.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/phy-caps.h |  5 +++++
 drivers/net/phy/phy-core.c | 36 +++++++++++++-----------------
 drivers/net/phy/phy_caps.c | 45 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 65 insertions(+), 21 deletions(-)

diff --git a/drivers/net/phy/phy-caps.h b/drivers/net/phy/phy-caps.h
index 1217686f1f91..8833798f141f 100644
--- a/drivers/net/phy/phy-caps.h
+++ b/drivers/net/phy/phy-caps.h
@@ -45,5 +45,10 @@ size_t phy_caps_speeds(unsigned int *speeds, size_t size,
 void phy_caps_linkmode_max_speed(u32 max_speed, unsigned long *linkmodes);
 bool phy_caps_valid(int speed, int duplex, const unsigned long *linkmodes);
 
+const struct link_capabilities *
+phy_caps_lookup_by_linkmode(const unsigned long *linkmodes);
+
+const struct link_capabilities *
+phy_caps_lookup_by_linkmode_rev(const unsigned long *linkmodes, bool fdx_only);
 
 #endif /* __PHY_CAPS_H */
diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
index af2463e67cea..bfba03f208fd 100644
--- a/drivers/net/phy/phy-core.c
+++ b/drivers/net/phy/phy-core.c
@@ -469,16 +469,15 @@ EXPORT_SYMBOL_GPL(phy_resolve_aneg_pause);
 void phy_resolve_aneg_linkmode(struct phy_device *phydev)
 {
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(common);
-	int i;
+	const struct link_capabilities *c;
 
 	linkmode_and(common, phydev->lp_advertising, phydev->advertising);
 
-	for (i = 0; i < ARRAY_SIZE(settings); i++)
-		if (test_bit(settings[i].bit, common)) {
-			phydev->speed = settings[i].speed;
-			phydev->duplex = settings[i].duplex;
-			break;
-		}
+	c = phy_caps_lookup_by_linkmode(common);
+	if (c) {
+		phydev->speed = c->speed;
+		phydev->duplex = c->duplex;
+	}
 
 	phy_resolve_aneg_pause(phydev);
 }
@@ -496,7 +495,8 @@ EXPORT_SYMBOL_GPL(phy_resolve_aneg_linkmode);
 void phy_check_downshift(struct phy_device *phydev)
 {
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(common);
-	int i, speed = SPEED_UNKNOWN;
+	const struct link_capabilities *c;
+	int speed = SPEED_UNKNOWN;
 
 	phydev->downshifted_rate = 0;
 
@@ -506,11 +506,9 @@ void phy_check_downshift(struct phy_device *phydev)
 
 	linkmode_and(common, phydev->lp_advertising, phydev->advertising);
 
-	for (i = 0; i < ARRAY_SIZE(settings); i++)
-		if (test_bit(settings[i].bit, common)) {
-			speed = settings[i].speed;
-			break;
-		}
+	c = phy_caps_lookup_by_linkmode(common);
+	if (c)
+		speed = c->speed;
 
 	if (speed == SPEED_UNKNOWN || phydev->speed >= speed)
 		return;
@@ -524,17 +522,13 @@ void phy_check_downshift(struct phy_device *phydev)
 static int phy_resolve_min_speed(struct phy_device *phydev, bool fdx_only)
 {
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(common);
-	int i = ARRAY_SIZE(settings);
+	const struct link_capabilities *c;
 
 	linkmode_and(common, phydev->lp_advertising, phydev->advertising);
 
-	while (--i >= 0) {
-		if (test_bit(settings[i].bit, common)) {
-			if (fdx_only && settings[i].duplex != DUPLEX_FULL)
-				continue;
-			return settings[i].speed;
-		}
-	}
+	c = phy_caps_lookup_by_linkmode_rev(common, fdx_only);
+	if (c)
+		return c->speed;
 
 	return SPEED_UNKNOWN;
 }
diff --git a/drivers/net/phy/phy_caps.c b/drivers/net/phy/phy_caps.c
index 4ee8c25c8521..c39f38c12ef2 100644
--- a/drivers/net/phy/phy_caps.c
+++ b/drivers/net/phy/phy_caps.c
@@ -125,6 +125,51 @@ size_t phy_caps_speeds(unsigned int *speeds, size_t size,
 	return count;
 }
 
+/**
+ * phy_caps_lookup_by_linkmode() - Lookup the fastest matching link_capabilities
+ * @linkmodes: Linkmodes to match against
+ *
+ * Returns: The highest-speed link_capabilities that intersects the given
+ *	    linkmodes. In case several DUPLEX_ options exist at that speed,
+ *	    DUPLEX_FULL is matched first. NULL is returned if no match.
+ */
+const struct link_capabilities *
+phy_caps_lookup_by_linkmode(const unsigned long *linkmodes)
+{
+	struct link_capabilities *lcap;
+
+	for_each_link_caps_desc_speed(lcap)
+		if (linkmode_intersects(lcap->linkmodes, linkmodes))
+			return lcap;
+
+	return NULL;
+}
+
+/**
+ * phy_caps_lookup_by_linkmode_rev() - Lookup the slowest matching link_capabilities
+ * @linkmodes: Linkmodes to match against
+ * @fdx_only: Full duplex match only when set
+ *
+ * Returns: The lowest-speed link_capabilities that intersects the given
+ *	    linkmodes. When set, fdx_only will ignore half-duplex matches.
+ *	    NULL is returned if no match.
+ */
+const struct link_capabilities *
+phy_caps_lookup_by_linkmode_rev(const unsigned long *linkmodes, bool fdx_only)
+{
+	struct link_capabilities *lcap;
+
+	for_each_link_caps_asc_speed(lcap) {
+		if (fdx_only && lcap->duplex != DUPLEX_FULL)
+			continue;
+
+		if (linkmode_intersects(lcap->linkmodes, linkmodes))
+			return lcap;
+	}
+
+	return NULL;
+}
+
 /**
  * phy_caps_linkmode_max_speed() - Clamp a linkmodes set to a max speed
  * @max_speed: Speed limit for the linkmode set
-- 
2.48.1

[PATCH net-next v5 07/13] net: phy: phy_caps: Allow looking-up link caps based on speed and duplex

[Maxime Chevallier]

As the link_caps array is efficient for <speed,duplex> lookups,
implement a function for speed/duplex lookups that matches a given
mask. This replicates to some extent the phy_lookup_settings()
behaviour, matching full link_capabilities instead of a single linkmode.

phy.c's phy_santize_settings() and phylink's
phylink_ethtool_ksettings_set() performs such lookup using the
phy_settings table, but are only interested in the actual speed/duplex
that were matched, rathet than the individual linkmode.

Similar to phy_lookup_settings(), the newly introduced phy_caps_lookup()
will run through the link_caps[] array by descending speed/duplex order.

If the link_capabilities for a given <speed/duplex> tuple intersects the
passed linkmodes, we consider that a match.

Similar to phy_lookup_settings(), we also allow passing an 'exact'
boolean, allowing non-exact match. Here, we MUST always match the
linkmodes mask, but we allow matching on lower speed settings.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/phy-caps.h |  4 ++++
 drivers/net/phy/phy.c      | 32 ++++++--------------------
 drivers/net/phy/phy_caps.c | 47 ++++++++++++++++++++++++++++++++++++++
 drivers/net/phy/phylink.c  | 17 +++++++-------
 4 files changed, 67 insertions(+), 33 deletions(-)

diff --git a/drivers/net/phy/phy-caps.h b/drivers/net/phy/phy-caps.h
index 8833798f141f..aef4b54a8429 100644
--- a/drivers/net/phy/phy-caps.h
+++ b/drivers/net/phy/phy-caps.h
@@ -51,4 +51,8 @@ phy_caps_lookup_by_linkmode(const unsigned long *linkmodes);
 const struct link_capabilities *
 phy_caps_lookup_by_linkmode_rev(const unsigned long *linkmodes, bool fdx_only);
 
+const struct link_capabilities *
+phy_caps_lookup(int speed, unsigned int duplex, const unsigned long *supported,
+		bool exact);
+
 #endif /* __PHY_CAPS_H */
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 8df37d221fba..562acde89224 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -213,25 +213,6 @@ int phy_aneg_done(struct phy_device *phydev)
 }
 EXPORT_SYMBOL(phy_aneg_done);
 
-/**
- * phy_find_valid - find a PHY setting that matches the requested parameters
- * @speed: desired speed
- * @duplex: desired duplex
- * @supported: mask of supported link modes
- *
- * Locate a supported phy setting that is, in priority order:
- * - an exact match for the specified speed and duplex mode
- * - a match for the specified speed, or slower speed
- * - the slowest supported speed
- * Returns the matched phy_setting entry, or %NULL if no supported phy
- * settings were found.
- */
-static const struct phy_setting *
-phy_find_valid(int speed, int duplex, unsigned long *supported)
-{
-	return phy_lookup_setting(speed, duplex, supported, false);
-}
-
 /**
  * phy_supported_speeds - return all speeds currently supported by a phy device
  * @phy: The phy device to return supported speeds of.
@@ -274,13 +255,14 @@ EXPORT_SYMBOL(phy_check_valid);
  */
 static void phy_sanitize_settings(struct phy_device *phydev)
 {
-	const struct phy_setting *setting;
+	const struct link_capabilities *c;
+
+	c = phy_caps_lookup(phydev->speed, phydev->duplex, phydev->supported,
+			    false);
 
-	setting = phy_find_valid(phydev->speed, phydev->duplex,
-				 phydev->supported);
-	if (setting) {
-		phydev->speed = setting->speed;
-		phydev->duplex = setting->duplex;
+	if (c) {
+		phydev->speed = c->speed;
+		phydev->duplex = c->duplex;
 	} else {
 		/* We failed to find anything (no supported speeds?) */
 		phydev->speed = SPEED_UNKNOWN;
diff --git a/drivers/net/phy/phy_caps.c b/drivers/net/phy/phy_caps.c
index c39f38c12ef2..0366feee2912 100644
--- a/drivers/net/phy/phy_caps.c
+++ b/drivers/net/phy/phy_caps.c
@@ -170,6 +170,53 @@ phy_caps_lookup_by_linkmode_rev(const unsigned long *linkmodes, bool fdx_only)
 	return NULL;
 }
 
+/**
+ * phy_caps_lookup() - Lookup capabilities by speed/duplex that matches a mask
+ * @speed: Speed to match
+ * @duplex: Duplex to match
+ * @supported: Mask of linkmodes to match
+ * @exact: Perform an exact match or not.
+ *
+ * Lookup a link_capabilities entry that intersect the supported linkmodes mask,
+ * and that matches the passed speed and duplex.
+ *
+ * When @exact is set, an exact match is performed on speed and duplex, meaning
+ * that if the linkmodes for the given speed and duplex intersect the supported
+ * mask, this capability is returned, otherwise we don't have a match and return
+ * NULL.
+ *
+ * When @exact is not set, we return either an exact match, or matching capabilities
+ * at lower speed, or the lowest matching speed, or NULL.
+ *
+ * Returns: a matched link_capabilities according to the above process, NULL
+ *	    otherwise.
+ */
+const struct link_capabilities *
+phy_caps_lookup(int speed, unsigned int duplex, const unsigned long *supported,
+		bool exact)
+{
+	const struct link_capabilities *lcap, *last = NULL;
+
+	for_each_link_caps_desc_speed(lcap) {
+		if (linkmode_intersects(lcap->linkmodes, supported)) {
+			last = lcap;
+			/* exact match on speed and duplex*/
+			if (lcap->speed == speed && lcap->duplex == duplex) {
+				return lcap;
+			} else if (!exact) {
+				if (lcap->speed <= speed)
+					return lcap;
+			}
+		}
+	}
+
+	if (!exact)
+		return last;
+
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(phy_caps_lookup);
+
 /**
  * phy_caps_linkmode_max_speed() - Clamp a linkmodes set to a max speed
  * @max_speed: Speed limit for the linkmode set
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index a3f64b6d2d34..cf9f019382ad 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -20,6 +20,7 @@
 #include <linux/timer.h>
 #include <linux/workqueue.h>
 
+#include "phy-caps.h"
 #include "sfp.h"
 #include "swphy.h"
 
@@ -2852,8 +2853,8 @@ int phylink_ethtool_ksettings_set(struct phylink *pl,
 				  const struct ethtool_link_ksettings *kset)
 {
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(support);
+	const struct link_capabilities *c;
 	struct phylink_link_state config;
-	const struct phy_setting *s;
 
 	ASSERT_RTNL();
 
@@ -2896,23 +2897,23 @@ int phylink_ethtool_ksettings_set(struct phylink *pl,
 		/* Autonegotiation disabled, select a suitable speed and
 		 * duplex.
 		 */
-		s = phy_lookup_setting(kset->base.speed, kset->base.duplex,
-				       pl->supported, false);
-		if (!s)
+		c = phy_caps_lookup(kset->base.speed, kset->base.duplex,
+				    pl->supported, false);
+		if (!c)
 			return -EINVAL;
 
 		/* If we have a fixed link, refuse to change link parameters.
 		 * If the link parameters match, accept them but do nothing.
 		 */
 		if (pl->req_link_an_mode == MLO_AN_FIXED) {
-			if (s->speed != pl->link_config.speed ||
-			    s->duplex != pl->link_config.duplex)
+			if (c->speed != pl->link_config.speed ||
+			    c->duplex != pl->link_config.duplex)
 				return -EINVAL;
 			return 0;
 		}
 
-		config.speed = s->speed;
-		config.duplex = s->duplex;
+		config.speed = c->speed;
+		config.duplex = c->duplex;
 		break;
 
 	case AUTONEG_ENABLE:
-- 
2.48.1

[PATCH net-next v5 08/13] net: phy: phy_device: Use link_capabilities lookup for PHY aneg config

[Maxime Chevallier]

When configuring PHY advertising with autoneg disabled, we lookd for an
exact linkmode to advertise and configure for the requested Speed and
Duplex, specially at or over 1G.

Using phy_caps_lookup allows us to build a list of the supported
linkmodes at that speed that we can advertise instead of the first mode
that matches.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/phy_device.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 4980862398d0..3f734e847e8e 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -2120,7 +2120,7 @@ EXPORT_SYMBOL(genphy_check_and_restart_aneg);
 int __genphy_config_aneg(struct phy_device *phydev, bool changed)
 {
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(fixed_advert);
-	const struct phy_setting *set;
+	const struct link_capabilities *c;
 	unsigned long *advert;
 	int err;
 
@@ -2146,10 +2146,11 @@ int __genphy_config_aneg(struct phy_device *phydev, bool changed)
 	} else {
 		linkmode_zero(fixed_advert);
 
-		set = phy_lookup_setting(phydev->speed, phydev->duplex,
-					 phydev->supported, true);
-		if (set)
-			linkmode_set_bit(set->bit, fixed_advert);
+		c = phy_caps_lookup(phydev->speed, phydev->duplex,
+				    phydev->supported, true);
+		if (c)
+			linkmode_and(fixed_advert, phydev->supported,
+				     c->linkmodes);
 
 		advert = fixed_advert;
 	}
-- 
2.48.1

[PATCH net-next v5 09/13] net: phylink: Use phy_caps_lookup for fixed-link configuration

[Maxime Chevallier]

When phylink creates a fixed-link configuration, it finds a matching
linkmode to set as the advertised, lp_advertising and supported modes
based on the speed and duplex of the fixed link.

Use the newly introduced phy_caps_lookup to get these modes instead of
phy_lookup_settings(). This has the side effect that the matched
settings and configured linkmodes may now contain several linkmodes (the
intersection of supported linkmodes from the phylink settings and the
linkmodes that match speed/duplex) instead of the one from
phy_lookup_settings().

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/phylink.c | 44 +++++++++++++++++++++++++++------------
 1 file changed, 31 insertions(+), 13 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index cf9f019382ad..8e2b7d647a92 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -802,12 +802,26 @@ static int phylink_validate(struct phylink *pl, unsigned long *supported,
 	return phylink_validate_mac_and_pcs(pl, supported, state);
 }
 
+static void phylink_fill_fixedlink_supported(unsigned long *supported)
+{
+	linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT, supported);
+	linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT, supported);
+	linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT, supported);
+	linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT, supported);
+	linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT, supported);
+	linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, supported);
+	linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, supported);
+	linkmode_set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, supported);
+	linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, supported);
+}
+
 static int phylink_parse_fixedlink(struct phylink *pl,
 				   const struct fwnode_handle *fwnode)
 {
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(match) = { 0, };
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
+	const struct link_capabilities *c;
 	struct fwnode_handle *fixed_node;
-	const struct phy_setting *s;
 	struct gpio_desc *desc;
 	u32 speed;
 	int ret;
@@ -875,12 +889,16 @@ static int phylink_parse_fixedlink(struct phylink *pl,
 		phylink_warn(pl, "fixed link specifies half duplex for %dMbps link?\n",
 			     pl->link_config.speed);
 
-	linkmode_fill(pl->supported);
+	linkmode_zero(pl->supported);
+	phylink_fill_fixedlink_supported(pl->supported);
+
 	linkmode_copy(pl->link_config.advertising, pl->supported);
 	phylink_validate(pl, pl->supported, &pl->link_config);
 
-	s = phy_lookup_setting(pl->link_config.speed, pl->link_config.duplex,
-			       pl->supported, true);
+	c = phy_caps_lookup(pl->link_config.speed, pl->link_config.duplex,
+			    pl->supported, true);
+	if (c)
+		linkmode_and(match, pl->supported, c->linkmodes);
 
 	linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT, mask);
 	linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, mask);
@@ -889,9 +907,10 @@ static int phylink_parse_fixedlink(struct phylink *pl,
 
 	phylink_set(pl->supported, MII);
 
-	if (s) {
-		__set_bit(s->bit, pl->supported);
-		__set_bit(s->bit, pl->link_config.lp_advertising);
+	if (c) {
+		linkmode_or(pl->supported, pl->supported, match);
+		linkmode_or(pl->link_config.lp_advertising,
+			    pl->link_config.lp_advertising, match);
 	} else {
 		phylink_warn(pl, "fixed link %s duplex %dMbps not recognised\n",
 			     pl->link_config.duplex == DUPLEX_FULL ? "full" : "half",
@@ -1879,21 +1898,20 @@ static int phylink_register_sfp(struct phylink *pl,
 int phylink_set_fixed_link(struct phylink *pl,
 			   const struct phylink_link_state *state)
 {
-	const struct phy_setting *s;
+	const struct link_capabilities *c;
 	unsigned long *adv;
 
 	if (pl->cfg_link_an_mode != MLO_AN_PHY || !state ||
 	    !test_bit(PHYLINK_DISABLE_STOPPED, &pl->phylink_disable_state))
 		return -EINVAL;
 
-	s = phy_lookup_setting(state->speed, state->duplex,
-			       pl->supported, true);
-	if (!s)
+	c = phy_caps_lookup(state->speed, state->duplex,
+			    pl->supported, true);
+	if (!c)
 		return -EINVAL;
 
 	adv = pl->link_config.advertising;
-	linkmode_zero(adv);
-	linkmode_set_bit(s->bit, adv);
+	linkmode_and(adv, pl->supported, c->linkmodes);
 	linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, adv);
 
 	pl->link_config.speed = state->speed;
-- 
2.48.1

[PATCH net-next v5 10/13] net: phy: drop phy_settings and the associated lookup helpers

[Maxime Chevallier]

The phy_settings array is no longer relevant as it has now been replaced
by the link_caps array and associated phy_caps helpers.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/phy-core.c | 184 -------------------------------------
 include/linux/phy.h        |  13 ---
 2 files changed, 197 deletions(-)

diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
index bfba03f208fd..c064c49f971c 100644
--- a/drivers/net/phy/phy-core.c
+++ b/drivers/net/phy/phy-core.c
@@ -157,190 +157,6 @@ int phy_interface_num_ports(phy_interface_t interface)
 }
 EXPORT_SYMBOL_GPL(phy_interface_num_ports);
 
-/* A mapping of all SUPPORTED settings to speed/duplex.  This table
- * must be grouped by speed and sorted in descending match priority
- * - iow, descending speed.
- */
-
-#define PHY_SETTING(s, d, b) { .speed = SPEED_ ## s, .duplex = DUPLEX_ ## d, \
-			       .bit = ETHTOOL_LINK_MODE_ ## b ## _BIT}
-
-static const struct phy_setting settings[] = {
-	/* 800G */
-	PHY_SETTING( 800000, FULL, 800000baseCR8_Full		),
-	PHY_SETTING( 800000, FULL, 800000baseKR8_Full		),
-	PHY_SETTING( 800000, FULL, 800000baseDR8_Full		),
-	PHY_SETTING( 800000, FULL, 800000baseDR8_2_Full		),
-	PHY_SETTING( 800000, FULL, 800000baseSR8_Full		),
-	PHY_SETTING( 800000, FULL, 800000baseVR8_Full		),
-	PHY_SETTING( 800000, FULL, 800000baseCR4_Full		),
-	PHY_SETTING( 800000, FULL, 800000baseKR4_Full		),
-	PHY_SETTING( 800000, FULL, 800000baseDR4_Full		),
-	PHY_SETTING( 800000, FULL, 800000baseDR4_2_Full		),
-	PHY_SETTING( 800000, FULL, 800000baseSR4_Full		),
-	PHY_SETTING( 800000, FULL, 800000baseVR4_Full		),
-	/* 400G */
-	PHY_SETTING( 400000, FULL, 400000baseCR8_Full		),
-	PHY_SETTING( 400000, FULL, 400000baseKR8_Full		),
-	PHY_SETTING( 400000, FULL, 400000baseLR8_ER8_FR8_Full	),
-	PHY_SETTING( 400000, FULL, 400000baseDR8_Full		),
-	PHY_SETTING( 400000, FULL, 400000baseSR8_Full		),
-	PHY_SETTING( 400000, FULL, 400000baseCR4_Full		),
-	PHY_SETTING( 400000, FULL, 400000baseKR4_Full		),
-	PHY_SETTING( 400000, FULL, 400000baseLR4_ER4_FR4_Full	),
-	PHY_SETTING( 400000, FULL, 400000baseDR4_Full		),
-	PHY_SETTING( 400000, FULL, 400000baseSR4_Full		),
-	PHY_SETTING( 400000, FULL, 400000baseCR2_Full		),
-	PHY_SETTING( 400000, FULL, 400000baseKR2_Full		),
-	PHY_SETTING( 400000, FULL, 400000baseDR2_Full		),
-	PHY_SETTING( 400000, FULL, 400000baseDR2_2_Full		),
-	PHY_SETTING( 400000, FULL, 400000baseSR2_Full		),
-	PHY_SETTING( 400000, FULL, 400000baseVR2_Full		),
-	/* 200G */
-	PHY_SETTING( 200000, FULL, 200000baseCR4_Full		),
-	PHY_SETTING( 200000, FULL, 200000baseKR4_Full		),
-	PHY_SETTING( 200000, FULL, 200000baseLR4_ER4_FR4_Full	),
-	PHY_SETTING( 200000, FULL, 200000baseDR4_Full		),
-	PHY_SETTING( 200000, FULL, 200000baseSR4_Full		),
-	PHY_SETTING( 200000, FULL, 200000baseCR2_Full		),
-	PHY_SETTING( 200000, FULL, 200000baseKR2_Full		),
-	PHY_SETTING( 200000, FULL, 200000baseLR2_ER2_FR2_Full	),
-	PHY_SETTING( 200000, FULL, 200000baseDR2_Full		),
-	PHY_SETTING( 200000, FULL, 200000baseSR2_Full		),
-	PHY_SETTING( 200000, FULL, 200000baseCR_Full		),
-	PHY_SETTING( 200000, FULL, 200000baseKR_Full		),
-	PHY_SETTING( 200000, FULL, 200000baseDR_Full		),
-	PHY_SETTING( 200000, FULL, 200000baseDR_2_Full		),
-	PHY_SETTING( 200000, FULL, 200000baseSR_Full		),
-	PHY_SETTING( 200000, FULL, 200000baseVR_Full		),
-	/* 100G */
-	PHY_SETTING( 100000, FULL, 100000baseCR4_Full		),
-	PHY_SETTING( 100000, FULL, 100000baseKR4_Full		),
-	PHY_SETTING( 100000, FULL, 100000baseLR4_ER4_Full	),
-	PHY_SETTING( 100000, FULL, 100000baseSR4_Full		),
-	PHY_SETTING( 100000, FULL, 100000baseCR2_Full		),
-	PHY_SETTING( 100000, FULL, 100000baseKR2_Full		),
-	PHY_SETTING( 100000, FULL, 100000baseLR2_ER2_FR2_Full	),
-	PHY_SETTING( 100000, FULL, 100000baseDR2_Full		),
-	PHY_SETTING( 100000, FULL, 100000baseSR2_Full		),
-	PHY_SETTING( 100000, FULL, 100000baseCR_Full		),
-	PHY_SETTING( 100000, FULL, 100000baseKR_Full		),
-	PHY_SETTING( 100000, FULL, 100000baseLR_ER_FR_Full	),
-	PHY_SETTING( 100000, FULL, 100000baseDR_Full		),
-	PHY_SETTING( 100000, FULL, 100000baseSR_Full		),
-	/* 56G */
-	PHY_SETTING(  56000, FULL,  56000baseCR4_Full	  	),
-	PHY_SETTING(  56000, FULL,  56000baseKR4_Full	  	),
-	PHY_SETTING(  56000, FULL,  56000baseLR4_Full	  	),
-	PHY_SETTING(  56000, FULL,  56000baseSR4_Full	  	),
-	/* 50G */
-	PHY_SETTING(  50000, FULL,  50000baseCR2_Full		),
-	PHY_SETTING(  50000, FULL,  50000baseKR2_Full		),
-	PHY_SETTING(  50000, FULL,  50000baseSR2_Full		),
-	PHY_SETTING(  50000, FULL,  50000baseCR_Full		),
-	PHY_SETTING(  50000, FULL,  50000baseKR_Full		),
-	PHY_SETTING(  50000, FULL,  50000baseLR_ER_FR_Full	),
-	PHY_SETTING(  50000, FULL,  50000baseDR_Full		),
-	PHY_SETTING(  50000, FULL,  50000baseSR_Full		),
-	/* 40G */
-	PHY_SETTING(  40000, FULL,  40000baseCR4_Full		),
-	PHY_SETTING(  40000, FULL,  40000baseKR4_Full		),
-	PHY_SETTING(  40000, FULL,  40000baseLR4_Full		),
-	PHY_SETTING(  40000, FULL,  40000baseSR4_Full		),
-	/* 25G */
-	PHY_SETTING(  25000, FULL,  25000baseCR_Full		),
-	PHY_SETTING(  25000, FULL,  25000baseKR_Full		),
-	PHY_SETTING(  25000, FULL,  25000baseSR_Full		),
-	/* 20G */
-	PHY_SETTING(  20000, FULL,  20000baseKR2_Full		),
-	PHY_SETTING(  20000, FULL,  20000baseMLD2_Full		),
-	/* 10G */
-	PHY_SETTING(  10000, FULL,  10000baseCR_Full		),
-	PHY_SETTING(  10000, FULL,  10000baseER_Full		),
-	PHY_SETTING(  10000, FULL,  10000baseKR_Full		),
-	PHY_SETTING(  10000, FULL,  10000baseKX4_Full		),
-	PHY_SETTING(  10000, FULL,  10000baseLR_Full		),
-	PHY_SETTING(  10000, FULL,  10000baseLRM_Full		),
-	PHY_SETTING(  10000, FULL,  10000baseR_FEC		),
-	PHY_SETTING(  10000, FULL,  10000baseSR_Full		),
-	PHY_SETTING(  10000, FULL,  10000baseT_Full		),
-	/* 5G */
-	PHY_SETTING(   5000, FULL,   5000baseT_Full		),
-	/* 2.5G */
-	PHY_SETTING(   2500, FULL,   2500baseT_Full		),
-	PHY_SETTING(   2500, FULL,   2500baseX_Full		),
-	/* 1G */
-	PHY_SETTING(   1000, FULL,   1000baseT_Full		),
-	PHY_SETTING(   1000, HALF,   1000baseT_Half		),
-	PHY_SETTING(   1000, FULL,   1000baseT1_Full		),
-	PHY_SETTING(   1000, FULL,   1000baseX_Full		),
-	PHY_SETTING(   1000, FULL,   1000baseKX_Full		),
-	/* 100M */
-	PHY_SETTING(    100, FULL,    100baseT_Full		),
-	PHY_SETTING(    100, FULL,    100baseT1_Full		),
-	PHY_SETTING(    100, HALF,    100baseT_Half		),
-	PHY_SETTING(    100, HALF,    100baseFX_Half		),
-	PHY_SETTING(    100, FULL,    100baseFX_Full		),
-	/* 10M */
-	PHY_SETTING(     10, FULL,     10baseT_Full		),
-	PHY_SETTING(     10, HALF,     10baseT_Half		),
-	PHY_SETTING(     10, FULL,     10baseT1L_Full		),
-	PHY_SETTING(     10, FULL,     10baseT1S_Full		),
-	PHY_SETTING(     10, HALF,     10baseT1S_Half		),
-	PHY_SETTING(     10, HALF,     10baseT1S_P2MP_Half	),
-	PHY_SETTING(     10, FULL,     10baseT1BRR_Full		),
-};
-#undef PHY_SETTING
-
-/**
- * phy_lookup_setting - lookup a PHY setting
- * @speed: speed to match
- * @duplex: duplex to match
- * @mask: allowed link modes
- * @exact: an exact match is required
- *
- * Search the settings array for a setting that matches the speed and
- * duplex, and which is supported.
- *
- * If @exact is unset, either an exact match or %NULL for no match will
- * be returned.
- *
- * If @exact is set, an exact match, the fastest supported setting at
- * or below the specified speed, the slowest supported setting, or if
- * they all fail, %NULL will be returned.
- */
-const struct phy_setting *
-phy_lookup_setting(int speed, int duplex, const unsigned long *mask, bool exact)
-{
-	const struct phy_setting *p, *match = NULL, *last = NULL;
-	int i;
-
-	for (i = 0, p = settings; i < ARRAY_SIZE(settings); i++, p++) {
-		if (p->bit < __ETHTOOL_LINK_MODE_MASK_NBITS &&
-		    test_bit(p->bit, mask)) {
-			last = p;
-			if (p->speed == speed && p->duplex == duplex) {
-				/* Exact match for speed and duplex */
-				match = p;
-				break;
-			} else if (!exact) {
-				if (!match && p->speed <= speed)
-					/* Candidate */
-					match = p;
-
-				if (p->speed < speed)
-					break;
-			}
-		}
-	}
-
-	if (!match && !exact)
-		match = last;
-
-	return match;
-}
-EXPORT_SYMBOL_GPL(phy_lookup_setting);
-
 static void __set_phy_supported(struct phy_device *phydev, u32 max_speed)
 {
 	phy_caps_linkmode_max_speed(max_speed, phydev->supported);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 99c195fa3695..ee4cc6a4ac3f 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1275,19 +1275,6 @@ const char *phy_rate_matching_to_str(int rate_matching);
 
 int phy_interface_num_ports(phy_interface_t interface);
 
-/* A structure for mapping a particular speed and duplex
- * combination to a particular SUPPORTED and ADVERTISED value
- */
-struct phy_setting {
-	u32 speed;
-	u8 duplex;
-	u8 bit;
-};
-
-const struct phy_setting *
-phy_lookup_setting(int speed, int duplex, const unsigned long *mask,
-		   bool exact);
-
 /**
  * phy_is_started - Convenience function to check whether PHY is started
  * @phydev: The phy_device struct
-- 
2.48.1

[PATCH net-next v5 11/13] net: phylink: Add a mapping between MAC_CAPS and LINK_CAPS

[Maxime Chevallier]

phylink allows MAC drivers to report the capabilities in terms of speed,
duplex and pause support. This is done through a dedicated set of enum
values in the form of the MAC_ capabilities. They are very close to what
the LINK_CAPA_xxx can express, with the difference that LINK_CAPA don't
have any information about Pause/Asym Pause support.

To prepare converting phylink to using the phy_caps, add the mapping
between MAC capabilities and phy_caps. While doing so, we move the
phylink_caps_params array up a bit to simplify future commits.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/phylink.c | 49 ++++++++++++++++++++-------------------
 1 file changed, 25 insertions(+), 24 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 8e2b7d647a92..c6fa3f003833 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -292,6 +292,31 @@ static int phylink_interface_max_speed(phy_interface_t interface)
 	return SPEED_UNKNOWN;
 }
 
+static struct {
+	unsigned long mask;
+	int speed;
+	unsigned int duplex;
+	unsigned int caps_bit;
+} phylink_caps_params[] = {
+	{ MAC_400000FD, SPEED_400000, DUPLEX_FULL, BIT(LINK_CAPA_400000FD) },
+	{ MAC_200000FD, SPEED_200000, DUPLEX_FULL, BIT(LINK_CAPA_200000FD) },
+	{ MAC_100000FD, SPEED_100000, DUPLEX_FULL, BIT(LINK_CAPA_100000FD) },
+	{ MAC_56000FD,  SPEED_56000,  DUPLEX_FULL, BIT(LINK_CAPA_56000FD) },
+	{ MAC_50000FD,  SPEED_50000,  DUPLEX_FULL, BIT(LINK_CAPA_50000FD) },
+	{ MAC_40000FD,  SPEED_40000,  DUPLEX_FULL, BIT(LINK_CAPA_40000FD) },
+	{ MAC_25000FD,  SPEED_25000,  DUPLEX_FULL, BIT(LINK_CAPA_25000FD) },
+	{ MAC_20000FD,  SPEED_20000,  DUPLEX_FULL, BIT(LINK_CAPA_20000FD) },
+	{ MAC_10000FD,  SPEED_10000,  DUPLEX_FULL, BIT(LINK_CAPA_10000FD) },
+	{ MAC_5000FD,   SPEED_5000,   DUPLEX_FULL, BIT(LINK_CAPA_5000FD) },
+	{ MAC_2500FD,   SPEED_2500,   DUPLEX_FULL, BIT(LINK_CAPA_2500FD) },
+	{ MAC_1000FD,   SPEED_1000,   DUPLEX_FULL, BIT(LINK_CAPA_1000FD) },
+	{ MAC_1000HD,   SPEED_1000,   DUPLEX_HALF, BIT(LINK_CAPA_1000HD) },
+	{ MAC_100FD,    SPEED_100,    DUPLEX_FULL, BIT(LINK_CAPA_100FD) },
+	{ MAC_100HD,    SPEED_100,    DUPLEX_HALF, BIT(LINK_CAPA_100HD) },
+	{ MAC_10FD,     SPEED_10,     DUPLEX_FULL, BIT(LINK_CAPA_10FD) },
+	{ MAC_10HD,     SPEED_10,     DUPLEX_HALF, BIT(LINK_CAPA_10HD) },
+};
+
 /**
  * phylink_caps_to_linkmodes() - Convert capabilities to ethtool link modes
  * @linkmodes: ethtool linkmode mask (must be already initialised)
@@ -445,30 +470,6 @@ static void phylink_caps_to_linkmodes(unsigned long *linkmodes,
 	}
 }
 
-static struct {
-	unsigned long mask;
-	int speed;
-	unsigned int duplex;
-} phylink_caps_params[] = {
-	{ MAC_400000FD, SPEED_400000, DUPLEX_FULL },
-	{ MAC_200000FD, SPEED_200000, DUPLEX_FULL },
-	{ MAC_100000FD, SPEED_100000, DUPLEX_FULL },
-	{ MAC_56000FD,  SPEED_56000,  DUPLEX_FULL },
-	{ MAC_50000FD,  SPEED_50000,  DUPLEX_FULL },
-	{ MAC_40000FD,  SPEED_40000,  DUPLEX_FULL },
-	{ MAC_25000FD,  SPEED_25000,  DUPLEX_FULL },
-	{ MAC_20000FD,  SPEED_20000,  DUPLEX_FULL },
-	{ MAC_10000FD,  SPEED_10000,  DUPLEX_FULL },
-	{ MAC_5000FD,   SPEED_5000,   DUPLEX_FULL },
-	{ MAC_2500FD,   SPEED_2500,   DUPLEX_FULL },
-	{ MAC_1000FD,   SPEED_1000,   DUPLEX_FULL },
-	{ MAC_1000HD,   SPEED_1000,   DUPLEX_HALF },
-	{ MAC_100FD,    SPEED_100,    DUPLEX_FULL },
-	{ MAC_100HD,    SPEED_100,    DUPLEX_HALF },
-	{ MAC_10FD,     SPEED_10,     DUPLEX_FULL },
-	{ MAC_10HD,     SPEED_10,     DUPLEX_HALF },
-};
-
 /**
  * phylink_limit_mac_speed - limit the phylink_config to a maximum speed
  * @config: pointer to a &struct phylink_config
-- 
2.48.1

[PATCH net-next v5 12/13] net: phylink: Convert capabilities to linkmodes using phy_caps

[Maxime Chevallier]

phylink_caps_to_linkmodes() is used to derive a list of linkmodes that
can be conceivably exposed using a given set of speeds and duplex
through phylink's MAC capabilities.

This list can be derived from the link_caps array in phy_caps, provided
we convert the MAC capabilities into a LINK_CAPA bitmask first.

Introduce an internal phylink helper phylink_caps_to_link_caps() to
convert from MAC capabilities into phy_caps, then  phy_caps_linkmodes()
to do the link_caps -> linkmodes conversion.

This avoids having to update phylink for every new linkmode.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/phy-caps.h |   1 +
 drivers/net/phy/phy_caps.c |  14 ++++
 drivers/net/phy/phylink.c  | 155 +++++--------------------------------
 3 files changed, 36 insertions(+), 134 deletions(-)

diff --git a/drivers/net/phy/phy-caps.h b/drivers/net/phy/phy-caps.h
index aef4b54a8429..7fc930876e33 100644
--- a/drivers/net/phy/phy-caps.h
+++ b/drivers/net/phy/phy-caps.h
@@ -44,6 +44,7 @@ size_t phy_caps_speeds(unsigned int *speeds, size_t size,
 		       unsigned long *linkmodes);
 void phy_caps_linkmode_max_speed(u32 max_speed, unsigned long *linkmodes);
 bool phy_caps_valid(int speed, int duplex, const unsigned long *linkmodes);
+void phy_caps_linkmodes(unsigned long caps, unsigned long *linkmodes);
 
 const struct link_capabilities *
 phy_caps_lookup_by_linkmode(const unsigned long *linkmodes);
diff --git a/drivers/net/phy/phy_caps.c b/drivers/net/phy/phy_caps.c
index 0366feee2912..fc1568f83c1a 100644
--- a/drivers/net/phy/phy_caps.c
+++ b/drivers/net/phy/phy_caps.c
@@ -251,3 +251,17 @@ bool phy_caps_valid(int speed, int duplex, const unsigned long *linkmodes)
 
 	return linkmode_intersects(link_caps[capa].linkmodes, linkmodes);
 }
+
+/**
+ * phy_caps_linkmodes() - Convert a bitfield of capabilities into linkmodes
+ * @caps: The list of caps, each bit corresponding to a LINK_CAPA value
+ * @linkmodes: The set of linkmodes to fill. Must be previously initialized.
+ */
+void phy_caps_linkmodes(unsigned long caps, unsigned long *linkmodes)
+{
+	unsigned long capa;
+
+	for_each_set_bit(capa, &caps, __LINK_CAPA_MAX)
+		linkmode_or(linkmodes, linkmodes, link_caps[capa].linkmodes);
+}
+EXPORT_SYMBOL_GPL(phy_caps_linkmodes);
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index c6fa3f003833..a75aad5e4563 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -317,6 +317,24 @@ static struct {
 	{ MAC_10HD,     SPEED_10,     DUPLEX_HALF, BIT(LINK_CAPA_10HD) },
 };
 
+/**
+ * phylink_caps_to_link_caps() - Convert a set of MAC capabilities LINK caps
+ * @caps: A set of MAC capabilities
+ *
+ * Returns: The corresponding set of LINK_CAPA as defined in phy-caps.h
+ */
+static unsigned long phylink_caps_to_link_caps(unsigned long caps)
+{
+	unsigned long link_caps = 0;
+	int i;
+
+	for (i = 0; i <  ARRAY_SIZE(phylink_caps_params); i++)
+		if (caps & phylink_caps_params[i].mask)
+			link_caps |= phylink_caps_params[i].caps_bit;
+
+	return link_caps;
+}
+
 /**
  * phylink_caps_to_linkmodes() - Convert capabilities to ethtool link modes
  * @linkmodes: ethtool linkmode mask (must be already initialised)
@@ -328,146 +346,15 @@ static struct {
 static void phylink_caps_to_linkmodes(unsigned long *linkmodes,
 				      unsigned long caps)
 {
+	unsigned long link_caps = phylink_caps_to_link_caps(caps);
+
 	if (caps & MAC_SYM_PAUSE)
 		__set_bit(ETHTOOL_LINK_MODE_Pause_BIT, linkmodes);
 
 	if (caps & MAC_ASYM_PAUSE)
 		__set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, linkmodes);
 
-	if (caps & MAC_10HD) {
-		__set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_10baseT1S_Half_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_10baseT1S_P2MP_Half_BIT, linkmodes);
-	}
-
-	if (caps & MAC_10FD) {
-		__set_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_10baseT1L_Full_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_10baseT1S_Full_BIT, linkmodes);
-	}
-
-	if (caps & MAC_100HD) {
-		__set_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_100baseFX_Half_BIT, linkmodes);
-	}
-
-	if (caps & MAC_100FD) {
-		__set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_100baseT1_Full_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_100baseFX_Full_BIT, linkmodes);
-	}
-
-	if (caps & MAC_1000HD)
-		__set_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT, linkmodes);
-
-	if (caps & MAC_1000FD) {
-		__set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_1000baseKX_Full_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_1000baseT1_Full_BIT, linkmodes);
-	}
-
-	if (caps & MAC_2500FD) {
-		__set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_2500baseX_Full_BIT, linkmodes);
-	}
-
-	if (caps & MAC_5000FD)
-		__set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, linkmodes);
-
-	if (caps & MAC_10000FD) {
-		__set_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_10000baseKX4_Full_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_10000baseKR_Full_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_10000baseR_FEC_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_10000baseCR_Full_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_10000baseSR_Full_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_10000baseLR_Full_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_10000baseLRM_Full_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_10000baseER_Full_BIT, linkmodes);
-	}
-
-	if (caps & MAC_25000FD) {
-		__set_bit(ETHTOOL_LINK_MODE_25000baseCR_Full_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_25000baseKR_Full_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_25000baseSR_Full_BIT, linkmodes);
-	}
-
-	if (caps & MAC_40000FD) {
-		__set_bit(ETHTOOL_LINK_MODE_40000baseKR4_Full_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_40000baseCR4_Full_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_40000baseSR4_Full_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_40000baseLR4_Full_BIT, linkmodes);
-	}
-
-	if (caps & MAC_50000FD) {
-		__set_bit(ETHTOOL_LINK_MODE_50000baseCR2_Full_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_50000baseKR2_Full_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_50000baseSR2_Full_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_50000baseKR_Full_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_50000baseSR_Full_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_50000baseCR_Full_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_50000baseLR_ER_FR_Full_BIT,
-			  linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_50000baseDR_Full_BIT, linkmodes);
-	}
-
-	if (caps & MAC_56000FD) {
-		__set_bit(ETHTOOL_LINK_MODE_56000baseKR4_Full_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_56000baseCR4_Full_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_56000baseSR4_Full_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_56000baseLR4_Full_BIT, linkmodes);
-	}
-
-	if (caps & MAC_100000FD) {
-		__set_bit(ETHTOOL_LINK_MODE_100000baseKR4_Full_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_100000baseSR4_Full_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_100000baseCR4_Full_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_100000baseLR4_ER4_Full_BIT,
-			  linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_100000baseKR2_Full_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_100000baseSR2_Full_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_100000baseCR2_Full_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_100000baseLR2_ER2_FR2_Full_BIT,
-			  linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_100000baseDR2_Full_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_100000baseKR_Full_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_100000baseSR_Full_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_100000baseLR_ER_FR_Full_BIT,
-			  linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_100000baseCR_Full_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_100000baseDR_Full_BIT, linkmodes);
-	}
-
-	if (caps & MAC_200000FD) {
-		__set_bit(ETHTOOL_LINK_MODE_200000baseKR4_Full_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_200000baseSR4_Full_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_200000baseLR4_ER4_FR4_Full_BIT,
-			  linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_200000baseDR4_Full_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_200000baseCR4_Full_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_200000baseKR2_Full_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_200000baseSR2_Full_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_200000baseLR2_ER2_FR2_Full_BIT,
-			  linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_200000baseDR2_Full_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_200000baseCR2_Full_BIT, linkmodes);
-	}
-
-	if (caps & MAC_400000FD) {
-		__set_bit(ETHTOOL_LINK_MODE_400000baseKR8_Full_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_400000baseSR8_Full_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_400000baseLR8_ER8_FR8_Full_BIT,
-			  linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_400000baseDR8_Full_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_400000baseCR8_Full_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_400000baseKR4_Full_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_400000baseSR4_Full_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_400000baseLR4_ER4_FR4_Full_BIT,
-			  linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_400000baseDR4_Full_BIT, linkmodes);
-		__set_bit(ETHTOOL_LINK_MODE_400000baseCR4_Full_BIT, linkmodes);
-	}
+	phy_caps_linkmodes(link_caps, linkmodes);
 }
 
 /**
-- 
2.48.1

[PATCH net-next v5 13/13] net: phylink: Use phy_caps to get an interface's capabilities and modes

[Maxime Chevallier]

Phylink has internal code to get the MAC capabilities of a given PHY
interface (what are the supported speed and duplex).

Extract that into phy_caps, but use the link_capa for conversion. Add an
internal phylink helper for the link caps -> mac caps conversion, and
use this in phylink_caps_to_linkmodes().

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
V5: Rename the patch with the phylink prefix

 drivers/net/phy/phy-caps.h |  4 ++
 drivers/net/phy/phy_caps.c | 92 ++++++++++++++++++++++++++++++++++++++
 drivers/net/phy/phylink.c  | 90 ++++++-------------------------------
 3 files changed, 110 insertions(+), 76 deletions(-)

diff --git a/drivers/net/phy/phy-caps.h b/drivers/net/phy/phy-caps.h
index 7fc930876e33..157759966650 100644
--- a/drivers/net/phy/phy-caps.h
+++ b/drivers/net/phy/phy-caps.h
@@ -8,6 +8,7 @@
 #define __PHY_CAPS_H
 
 #include <linux/ethtool.h>
+#include <linux/phy.h>
 
 enum {
 	LINK_CAPA_10HD = 0,
@@ -32,6 +33,8 @@ enum {
 	__LINK_CAPA_MAX,
 };
 
+#define LINK_CAPA_ALL	GENMASK((__LINK_CAPA_MAX - 1), 0)
+
 struct link_capabilities {
 	int speed;
 	unsigned int duplex;
@@ -45,6 +48,7 @@ size_t phy_caps_speeds(unsigned int *speeds, size_t size,
 void phy_caps_linkmode_max_speed(u32 max_speed, unsigned long *linkmodes);
 bool phy_caps_valid(int speed, int duplex, const unsigned long *linkmodes);
 void phy_caps_linkmodes(unsigned long caps, unsigned long *linkmodes);
+unsigned long phy_caps_from_interface(phy_interface_t interface);
 
 const struct link_capabilities *
 phy_caps_lookup_by_linkmode(const unsigned long *linkmodes);
diff --git a/drivers/net/phy/phy_caps.c b/drivers/net/phy/phy_caps.c
index fc1568f83c1a..703321689726 100644
--- a/drivers/net/phy/phy_caps.c
+++ b/drivers/net/phy/phy_caps.c
@@ -265,3 +265,95 @@ void phy_caps_linkmodes(unsigned long caps, unsigned long *linkmodes)
 		linkmode_or(linkmodes, linkmodes, link_caps[capa].linkmodes);
 }
 EXPORT_SYMBOL_GPL(phy_caps_linkmodes);
+
+/**
+ * phy_caps_from_interface() - Get the link capa from a given PHY interface
+ * @interface: The PHY interface we want to get the possible Speed/Duplex from
+ *
+ * Returns: A bitmask of LINK_CAPA_xxx values that can be achieved with the
+ *          provided interface.
+ */
+unsigned long phy_caps_from_interface(phy_interface_t interface)
+{
+	unsigned long link_caps = 0;
+
+	switch (interface) {
+	case PHY_INTERFACE_MODE_USXGMII:
+		link_caps |= BIT(LINK_CAPA_10000FD) | BIT(LINK_CAPA_5000FD);
+		fallthrough;
+
+	case PHY_INTERFACE_MODE_10G_QXGMII:
+		link_caps |= BIT(LINK_CAPA_2500FD);
+		fallthrough;
+
+	case PHY_INTERFACE_MODE_RGMII_TXID:
+	case PHY_INTERFACE_MODE_RGMII_RXID:
+	case PHY_INTERFACE_MODE_RGMII_ID:
+	case PHY_INTERFACE_MODE_RGMII:
+	case PHY_INTERFACE_MODE_PSGMII:
+	case PHY_INTERFACE_MODE_QSGMII:
+	case PHY_INTERFACE_MODE_QUSGMII:
+	case PHY_INTERFACE_MODE_SGMII:
+	case PHY_INTERFACE_MODE_GMII:
+		link_caps |= BIT(LINK_CAPA_1000HD) | BIT(LINK_CAPA_1000FD);
+		fallthrough;
+
+	case PHY_INTERFACE_MODE_REVRMII:
+	case PHY_INTERFACE_MODE_RMII:
+	case PHY_INTERFACE_MODE_SMII:
+	case PHY_INTERFACE_MODE_REVMII:
+	case PHY_INTERFACE_MODE_MII:
+		link_caps |= BIT(LINK_CAPA_10HD) | BIT(LINK_CAPA_10FD);
+		fallthrough;
+
+	case PHY_INTERFACE_MODE_100BASEX:
+		link_caps |= BIT(LINK_CAPA_100HD) | BIT(LINK_CAPA_100FD);
+		break;
+
+	case PHY_INTERFACE_MODE_TBI:
+	case PHY_INTERFACE_MODE_MOCA:
+	case PHY_INTERFACE_MODE_RTBI:
+	case PHY_INTERFACE_MODE_1000BASEX:
+		link_caps |= BIT(LINK_CAPA_1000HD);
+		fallthrough;
+	case PHY_INTERFACE_MODE_1000BASEKX:
+	case PHY_INTERFACE_MODE_TRGMII:
+		link_caps |= BIT(LINK_CAPA_1000FD);
+		break;
+
+	case PHY_INTERFACE_MODE_2500BASEX:
+		link_caps |= BIT(LINK_CAPA_2500FD);
+		break;
+
+	case PHY_INTERFACE_MODE_5GBASER:
+		link_caps |= BIT(LINK_CAPA_5000FD);
+		break;
+
+	case PHY_INTERFACE_MODE_XGMII:
+	case PHY_INTERFACE_MODE_RXAUI:
+	case PHY_INTERFACE_MODE_XAUI:
+	case PHY_INTERFACE_MODE_10GBASER:
+	case PHY_INTERFACE_MODE_10GKR:
+		link_caps |= BIT(LINK_CAPA_10000FD);
+		break;
+
+	case PHY_INTERFACE_MODE_25GBASER:
+		link_caps |= BIT(LINK_CAPA_25000FD);
+		break;
+
+	case PHY_INTERFACE_MODE_XLGMII:
+		link_caps |= BIT(LINK_CAPA_40000FD);
+		break;
+
+	case PHY_INTERFACE_MODE_INTERNAL:
+		link_caps |= LINK_CAPA_ALL;
+		break;
+
+	case PHY_INTERFACE_MODE_NA:
+	case PHY_INTERFACE_MODE_MAX:
+		break;
+	}
+
+	return link_caps;
+}
+EXPORT_SYMBOL_GPL(phy_caps_from_interface);
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index a75aad5e4563..08a2f0f7c08e 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -335,6 +335,18 @@ static unsigned long phylink_caps_to_link_caps(unsigned long caps)
 	return link_caps;
 }
 
+static unsigned long phylink_link_caps_to_mac_caps(unsigned long link_caps)
+{
+	unsigned long caps = 0;
+	int i;
+
+	for (i = 0; i <  ARRAY_SIZE(phylink_caps_params); i++)
+		if (link_caps & phylink_caps_params[i].caps_bit)
+			caps |= phylink_caps_params[i].mask;
+
+	return caps;
+}
+
 /**
  * phylink_caps_to_linkmodes() - Convert capabilities to ethtool link modes
  * @linkmodes: ethtool linkmode mask (must be already initialised)
@@ -412,86 +424,12 @@ static unsigned long phylink_get_capabilities(phy_interface_t interface,
 					      unsigned long mac_capabilities,
 					      int rate_matching)
 {
+	unsigned long link_caps = phy_caps_from_interface(interface);
 	int max_speed = phylink_interface_max_speed(interface);
 	unsigned long caps = MAC_SYM_PAUSE | MAC_ASYM_PAUSE;
 	unsigned long matched_caps = 0;
 
-	switch (interface) {
-	case PHY_INTERFACE_MODE_USXGMII:
-		caps |= MAC_10000FD | MAC_5000FD;
-		fallthrough;
-
-	case PHY_INTERFACE_MODE_10G_QXGMII:
-		caps |= MAC_2500FD;
-		fallthrough;
-
-	case PHY_INTERFACE_MODE_RGMII_TXID:
-	case PHY_INTERFACE_MODE_RGMII_RXID:
-	case PHY_INTERFACE_MODE_RGMII_ID:
-	case PHY_INTERFACE_MODE_RGMII:
-	case PHY_INTERFACE_MODE_PSGMII:
-	case PHY_INTERFACE_MODE_QSGMII:
-	case PHY_INTERFACE_MODE_QUSGMII:
-	case PHY_INTERFACE_MODE_SGMII:
-	case PHY_INTERFACE_MODE_GMII:
-		caps |= MAC_1000HD | MAC_1000FD;
-		fallthrough;
-
-	case PHY_INTERFACE_MODE_REVRMII:
-	case PHY_INTERFACE_MODE_RMII:
-	case PHY_INTERFACE_MODE_SMII:
-	case PHY_INTERFACE_MODE_REVMII:
-	case PHY_INTERFACE_MODE_MII:
-		caps |= MAC_10HD | MAC_10FD;
-		fallthrough;
-
-	case PHY_INTERFACE_MODE_100BASEX:
-		caps |= MAC_100HD | MAC_100FD;
-		break;
-
-	case PHY_INTERFACE_MODE_TBI:
-	case PHY_INTERFACE_MODE_MOCA:
-	case PHY_INTERFACE_MODE_RTBI:
-	case PHY_INTERFACE_MODE_1000BASEX:
-		caps |= MAC_1000HD;
-		fallthrough;
-	case PHY_INTERFACE_MODE_1000BASEKX:
-	case PHY_INTERFACE_MODE_TRGMII:
-		caps |= MAC_1000FD;
-		break;
-
-	case PHY_INTERFACE_MODE_2500BASEX:
-		caps |= MAC_2500FD;
-		break;
-
-	case PHY_INTERFACE_MODE_5GBASER:
-		caps |= MAC_5000FD;
-		break;
-
-	case PHY_INTERFACE_MODE_XGMII:
-	case PHY_INTERFACE_MODE_RXAUI:
-	case PHY_INTERFACE_MODE_XAUI:
-	case PHY_INTERFACE_MODE_10GBASER:
-	case PHY_INTERFACE_MODE_10GKR:
-		caps |= MAC_10000FD;
-		break;
-
-	case PHY_INTERFACE_MODE_25GBASER:
-		caps |= MAC_25000FD;
-		break;
-
-	case PHY_INTERFACE_MODE_XLGMII:
-		caps |= MAC_40000FD;
-		break;
-
-	case PHY_INTERFACE_MODE_INTERNAL:
-		caps |= ~0;
-		break;
-
-	case PHY_INTERFACE_MODE_NA:
-	case PHY_INTERFACE_MODE_MAX:
-		break;
-	}
+	caps |= phylink_link_caps_to_mac_caps(link_caps);
 
 	switch (rate_matching) {
 	case RATE_MATCH_OPEN_LOOP:
-- 
2.48.1

Re: [PATCH net-next v5 00/13] net: phy: Rework linkmodes handling in a dedicated file

[Paolo Abeni]

On 3/7/25 6:35 PM, Maxime Chevallier wrote:
> Hello everyone,
> 
> This is V5 of the phy_caps series. In a nutshell, this series reworks the way
> we maintain the list of speed/duplex capablities for each linkmode so that we
> no longer have multiple definition of these associations.
> 
> That will help making sure that when people add new linkmodes in
> include/uapi/linux/ethtool.h, they don't have to update phylib and phylink as
> well, making the process more straightforward and less error-prone.
> 
> It also generalises the phy_caps interface to be able to lookup linkmodes
> from phy_interface_t, which is needed for the multi-port work I've been working
> on for a while.
> 
> This V5 addresse Russell's and Paolo's reviews, namely :
> 
>  - Error out when encountering an unknown SPEED_XXX setting
> 
>    It prints an error and fails to initialize phylib. I've tested by
>    introducing a dummy 1.6T speed, I guess it's only a matter of time
>    before that actually happens :)
> 
>  - Deal more gracefully with the fixed-link settings, keeping some level of
>    compatibility with what we had before by making sure we report a
>    single BaseT mode like before.
> 
> V1 : https://lore.kernel.org/netdev/20250222142727.894124-1-maxime.chevallier@bootlin.com/
> V2 : https://lore.kernel.org/netdev/20250226100929.1646454-1-maxime.chevallier@bootlin.com/
> V3 : https://lore.kernel.org/netdev/20250228145540.2209551-1-maxime.chevallier@bootlin.com/
> V4 : https://lore.kernel.org/netdev/20250303090321.805785-1-maxime.chevallier@bootlin.com/

LGTM, waiting an extra bit to allow explicit acks from the phy crew.

Thanks,

Paolo

Re: [PATCH net-next v5 00/13] net: phy: Rework linkmodes handling in a dedicated file

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Fri,  7 Mar 2025 18:35:57 +0100 you wrote:
> Hello everyone,
> 
> This is V5 of the phy_caps series. In a nutshell, this series reworks the way
> we maintain the list of speed/duplex capablities for each linkmode so that we
> no longer have multiple definition of these associations.
> 
> That will help making sure that when people add new linkmodes in
> include/uapi/linux/ethtool.h, they don't have to update phylib and phylink as
> well, making the process more straightforward and less error-prone.
> 
> [...]

Here is the summary with links:
  - [net-next,v5,01/13] net: ethtool: Export the link_mode_params definitions
    https://git.kernel.org/netdev/net-next/c/79f88a584e35
  - [net-next,v5,02/13] net: phy: Use an internal, searchable storage for the linkmodes
    https://git.kernel.org/netdev/net-next/c/d8c838a57ce2
  - [net-next,v5,03/13] net: phy: phy_caps: Move phy_speeds to phy_caps
    https://git.kernel.org/netdev/net-next/c/8c8c4a87933d
  - [net-next,v5,04/13] net: phy: phy_caps: Move __set_linkmode_max_speed to phy_caps
    https://git.kernel.org/netdev/net-next/c/4823ed060919
  - [net-next,v5,05/13] net: phy: phy_caps: Introduce phy_caps_valid
    https://git.kernel.org/netdev/net-next/c/87b22ce31235
  - [net-next,v5,06/13] net: phy: phy_caps: Implement link_capabilities lookup by linkmode
    https://git.kernel.org/netdev/net-next/c/dbcd85b05c5b
  - [net-next,v5,07/13] net: phy: phy_caps: Allow looking-up link caps based on speed and duplex
    https://git.kernel.org/netdev/net-next/c/fc81e257d19f
  - [net-next,v5,08/13] net: phy: phy_device: Use link_capabilities lookup for PHY aneg config
    https://git.kernel.org/netdev/net-next/c/c7ae89c6b4d5
  - [net-next,v5,09/13] net: phylink: Use phy_caps_lookup for fixed-link configuration
    https://git.kernel.org/netdev/net-next/c/de7d3f87be3c
  - [net-next,v5,10/13] net: phy: drop phy_settings and the associated lookup helpers
    https://git.kernel.org/netdev/net-next/c/ce60fef7fecc
  - [net-next,v5,11/13] net: phylink: Add a mapping between MAC_CAPS and LINK_CAPS
    https://git.kernel.org/netdev/net-next/c/3bea75002a05
  - [net-next,v5,12/13] net: phylink: Convert capabilities to linkmodes using phy_caps
    https://git.kernel.org/netdev/net-next/c/4ca5b8a258b6
  - [net-next,v5,13/13] net: phylink: Use phy_caps to get an interface's capabilities and modes
    https://git.kernel.org/netdev/net-next/c/3bd87f3b4405

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

Re: [PATCH net-next v5 09/13] net: phylink: Use phy_caps_lookup for fixed-link configuration

[Alexander H Duyck]

On Fri, 2025-03-07 at 18:36 +0100, Maxime Chevallier wrote:
> When phylink creates a fixed-link configuration, it finds a matching
> linkmode to set as the advertised, lp_advertising and supported modes
> based on the speed and duplex of the fixed link.
> 
> Use the newly introduced phy_caps_lookup to get these modes instead of
> phy_lookup_settings(). This has the side effect that the matched
> settings and configured linkmodes may now contain several linkmodes (the
> intersection of supported linkmodes from the phylink settings and the
> linkmodes that match speed/duplex) instead of the one from
> phy_lookup_settings().
> 
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> ---
>  drivers/net/phy/phylink.c | 44 +++++++++++++++++++++++++++------------
>  1 file changed, 31 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index cf9f019382ad..8e2b7d647a92 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -802,12 +802,26 @@ static int phylink_validate(struct phylink *pl, unsigned long *supported,
>  	return phylink_validate_mac_and_pcs(pl, supported, state);
>  }
>  
> +static void phylink_fill_fixedlink_supported(unsigned long *supported)
> +{
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT, supported);
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT, supported);
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT, supported);
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT, supported);
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT, supported);
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, supported);
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, supported);
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, supported);
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, supported);
> +}
> +

Any chance we can go with a different route here than just locking
fixed mode to being only for BaseT configurations?

I am currently working on getting the fbnic driver up and running and I
am using fixed-link mode as a crutch until I can finish up enabling
QSFP module support for phylink and this just knocked out the supported
link modes as I was using 25CR, 50CR, 50CR2, and 100CR2.

Seems like this should really be something handled by some sort of
validation function rather than just forcing all devices using fixed
link to assume that they are BaseT. I know in our direct attached
copper case we aren't running autoneg so that plan was to use fixed
link until we can add more flexibility by getting QSFP support going.

Re: [PATCH net-next v5 09/13] net: phylink: Use phy_caps_lookup for fixed-link configuration

[Maxime Chevallier]

On Thu, 27 Mar 2025 18:16:00 -0700
Alexander H Duyck <alexander.duyck@gmail.com> wrote:

> On Fri, 2025-03-07 at 18:36 +0100, Maxime Chevallier wrote:
> > When phylink creates a fixed-link configuration, it finds a matching
> > linkmode to set as the advertised, lp_advertising and supported modes
> > based on the speed and duplex of the fixed link.
> > 
> > Use the newly introduced phy_caps_lookup to get these modes instead of
> > phy_lookup_settings(). This has the side effect that the matched
> > settings and configured linkmodes may now contain several linkmodes (the
> > intersection of supported linkmodes from the phylink settings and the
> > linkmodes that match speed/duplex) instead of the one from
> > phy_lookup_settings().
> > 
> > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > ---
> >  drivers/net/phy/phylink.c | 44 +++++++++++++++++++++++++++------------
> >  1 file changed, 31 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > index cf9f019382ad..8e2b7d647a92 100644
> > --- a/drivers/net/phy/phylink.c
> > +++ b/drivers/net/phy/phylink.c
> > @@ -802,12 +802,26 @@ static int phylink_validate(struct phylink *pl, unsigned long *supported,
> >  	return phylink_validate_mac_and_pcs(pl, supported, state);
> >  }
> >  
> > +static void phylink_fill_fixedlink_supported(unsigned long *supported)
> > +{
> > +	linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT, supported);
> > +	linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT, supported);
> > +	linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT, supported);
> > +	linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT, supported);
> > +	linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT, supported);
> > +	linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, supported);
> > +	linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, supported);
> > +	linkmode_set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, supported);
> > +	linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, supported);
> > +}
> > +  
> 
> Any chance we can go with a different route here than just locking
> fixed mode to being only for BaseT configurations?
> 
> I am currently working on getting the fbnic driver up and running and I
> am using fixed-link mode as a crutch until I can finish up enabling
> QSFP module support for phylink and this just knocked out the supported
> link modes as I was using 25CR, 50CR, 50CR2, and 100CR2.
> 
> Seems like this should really be something handled by some sort of
> validation function rather than just forcing all devices using fixed
> link to assume that they are BaseT. I know in our direct attached
> copper case we aren't running autoneg so that plan was to use fixed
> link until we can add more flexibility by getting QSFP support going.

These baseT mode were for compatibility with the previous way to deal
with fixed links, but we can extend what's being report above 10G with
other modes. Indeed the above code unnecessarily restricts the
linkmodes. Can you tell me if the following patch works for you ?

Maxime

-----------

From 595888afb23f209f2b1002d0b0406380195d53c1 Mon Sep 17 00:00:00 2001
From: Maxime Chevallier <maxime.chevallier@bootlin.com>
Date: Fri, 28 Mar 2025 08:53:00 +0100
Subject: [PATCH] net: phylink: Allow fixed-link registration above 10G

The blamed commit introduced a helper to keep the linkmodes reported by
fixed links identical to what they were before phy_caps. This means
filtering linkmodes only to report BaseT modes.

Doing so, it also filtered out any linkmode above 10G. Reinstate the
reporting of linkmodes above 10G, where we report any linkmodes that
exist at these speeds.

Reported-by: Alexander H Duyck <alexander.duyck@gmail.com>
Closes: https://lore.kernel.org/netdev/8d3a9c9bb76b1c6bc27d2bd01f4831b2cac83f7f.camel@gmail.com/
Fixes: de7d3f87be3c ("net: phylink: Use phy_caps_lookup for fixed-link configuration")
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/phylink.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 69ca765485db..de90fed9c207 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -715,16 +715,25 @@ static int phylink_parse_fixedlink(struct phylink *pl,
 		phylink_warn(pl, "fixed link specifies half duplex for %dMbps link?\n",
 			     pl->link_config.speed);
 
-	linkmode_zero(pl->supported);
-	phylink_fill_fixedlink_supported(pl->supported);
+	linkmode_fill(pl->supported);
 
 	linkmode_copy(pl->link_config.advertising, pl->supported);
 	phylink_validate(pl, pl->supported, &pl->link_config);
 
+	phylink_fill_fixedlink_supported(match);
+
 	c = phy_caps_lookup(pl->link_config.speed, pl->link_config.duplex,
 			    pl->supported, true);
-	if (c)
-		linkmode_and(match, pl->supported, c->linkmodes);
+	if (c) {
+		/* Compatbility with the legacy behaviour : Report one single
+		 * BaseT mode for fixed-link speeds under or at 10G, or all
+		 * linkmodes at the speed/duplex for speeds over 10G
+		 */
+		if (linkmode_intersects(match, c->linkmodes))
+			linkmode_and(match, match, c->linkmodes);
+		else
+			linkmode_copy(match, c->linkmodes);
+	}
 
 	linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT, mask);
 	linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, mask);
-- 
2.48.1

Re: [PATCH net-next v5 09/13] net: phylink: Use phy_caps_lookup for fixed-link configuration

[Alexander Duyck]

On Fri, Mar 28, 2025 at 1:06 AM Maxime Chevallier
<maxime.chevallier@bootlin.com> wrote:
>
> On Thu, 27 Mar 2025 18:16:00 -0700
> Alexander H Duyck <alexander.duyck@gmail.com> wrote:
>
> > On Fri, 2025-03-07 at 18:36 +0100, Maxime Chevallier wrote:
> > > When phylink creates a fixed-link configuration, it finds a matching
> > > linkmode to set as the advertised, lp_advertising and supported modes
> > > based on the speed and duplex of the fixed link.
> > >
> > > Use the newly introduced phy_caps_lookup to get these modes instead of
> > > phy_lookup_settings(). This has the side effect that the matched
> > > settings and configured linkmodes may now contain several linkmodes (the
> > > intersection of supported linkmodes from the phylink settings and the
> > > linkmodes that match speed/duplex) instead of the one from
> > > phy_lookup_settings().
> > >
> > > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > > ---
> > >  drivers/net/phy/phylink.c | 44 +++++++++++++++++++++++++++------------
> > >  1 file changed, 31 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > > index cf9f019382ad..8e2b7d647a92 100644
> > > --- a/drivers/net/phy/phylink.c
> > > +++ b/drivers/net/phy/phylink.c
> > > @@ -802,12 +802,26 @@ static int phylink_validate(struct phylink *pl, unsigned long *supported,
> > >     return phylink_validate_mac_and_pcs(pl, supported, state);
> > >  }
> > >
> > > +static void phylink_fill_fixedlink_supported(unsigned long *supported)
> > > +{
> > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT, supported);
> > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT, supported);
> > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT, supported);
> > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT, supported);
> > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT, supported);
> > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, supported);
> > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, supported);
> > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, supported);
> > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, supported);
> > > +}
> > > +
> >
> > Any chance we can go with a different route here than just locking
> > fixed mode to being only for BaseT configurations?
> >
> > I am currently working on getting the fbnic driver up and running and I
> > am using fixed-link mode as a crutch until I can finish up enabling
> > QSFP module support for phylink and this just knocked out the supported
> > link modes as I was using 25CR, 50CR, 50CR2, and 100CR2.
> >
> > Seems like this should really be something handled by some sort of
> > validation function rather than just forcing all devices using fixed
> > link to assume that they are BaseT. I know in our direct attached
> > copper case we aren't running autoneg so that plan was to use fixed
> > link until we can add more flexibility by getting QSFP support going.
>
> These baseT mode were for compatibility with the previous way to deal
> with fixed links, but we can extend what's being report above 10G with
> other modes. Indeed the above code unnecessarily restricts the
> linkmodes. Can you tell me if the following patch works for you ?
>
> Maxime
>
> -----------
>
> From 595888afb23f209f2b1002d0b0406380195d53c1 Mon Sep 17 00:00:00 2001
> From: Maxime Chevallier <maxime.chevallier@bootlin.com>
> Date: Fri, 28 Mar 2025 08:53:00 +0100
> Subject: [PATCH] net: phylink: Allow fixed-link registration above 10G
>
> The blamed commit introduced a helper to keep the linkmodes reported by
> fixed links identical to what they were before phy_caps. This means
> filtering linkmodes only to report BaseT modes.
>
> Doing so, it also filtered out any linkmode above 10G. Reinstate the
> reporting of linkmodes above 10G, where we report any linkmodes that
> exist at these speeds.
>
> Reported-by: Alexander H Duyck <alexander.duyck@gmail.com>
> Closes: https://lore.kernel.org/netdev/8d3a9c9bb76b1c6bc27d2bd01f4831b2cac83f7f.camel@gmail.com/
> Fixes: de7d3f87be3c ("net: phylink: Use phy_caps_lookup for fixed-link configuration")
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> ---
>  drivers/net/phy/phylink.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 69ca765485db..de90fed9c207 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -715,16 +715,25 @@ static int phylink_parse_fixedlink(struct phylink *pl,
>                 phylink_warn(pl, "fixed link specifies half duplex for %dMbps link?\n",
>                              pl->link_config.speed);
>
> -       linkmode_zero(pl->supported);
> -       phylink_fill_fixedlink_supported(pl->supported);
> +       linkmode_fill(pl->supported);
>
>         linkmode_copy(pl->link_config.advertising, pl->supported);
>         phylink_validate(pl, pl->supported, &pl->link_config);
>
> +       phylink_fill_fixedlink_supported(match);
> +

I worry that this might put you back into the same problem again with
the older drivers. In addition it is filling in modes that shouldn't
be present after the validation.

One thought on this. You might look at checking to see if the TP bit
is set in the supported modes after validation and then use your
fixedlink_supported as a mask if it is supporting twisted pair
connections. One possibility would be to go through and filter the
modes based on the media type where you could basically filter out TP,
Fiber, and Backplane connection types and use that as a second pass
based on the settings by basically doing a set of AND and OR
operations.

Also I am not sure it makes sense to say we can't support multiple
modes on a fixed connection. For example in the case of SerDes links
and the like it isn't unusual to see support for CR/KR advertised at
the same speed on the same link and use the exact same configuration
so a fixed config could support both and advertise both at the same
time if I am not mistaken.

Re: [PATCH net-next v5 09/13] net: phylink: Use phy_caps_lookup for fixed-link configuration

[Andrew Lunn]

> Also I am not sure it makes sense to say we can't support multiple
> modes on a fixed connection. For example in the case of SerDes links
> and the like it isn't unusual to see support for CR/KR advertised at
> the same speed on the same link and use the exact same configuration
> so a fixed config could support both and advertise both at the same
> time if I am not mistaken.

Traditionally, fixed link has only supported one mode. The combination
of speed and duplex fully describes a base-T link. Even more
traditionally, it was implemented as an emulated C22 PHY, using the
genphy driver, so limited to just 1G. With multigige PHY we needed to
be a bit more flexible, so phylink gained its own fixed link
implementation which did not emulate a PHY, just the results of
talking to a multigige PHY.

But i don't think you are actually talking about a PHY. I think you
mean the PCS advertises CR/KR, and you want to emulate a fixed-link
PCS? That is a different beast.

	Andrew

Re: [PATCH net-next v5 09/13] net: phylink: Use phy_caps_lookup for fixed-link configuration

[Alexander Duyck]

On Fri, Mar 28, 2025 at 2:45 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > Also I am not sure it makes sense to say we can't support multiple
> > modes on a fixed connection. For example in the case of SerDes links
> > and the like it isn't unusual to see support for CR/KR advertised at
> > the same speed on the same link and use the exact same configuration
> > so a fixed config could support both and advertise both at the same
> > time if I am not mistaken.
>
> Traditionally, fixed link has only supported one mode. The combination
> of speed and duplex fully describes a base-T link. Even more
> traditionally, it was implemented as an emulated C22 PHY, using the
> genphy driver, so limited to just 1G. With multigige PHY we needed to
> be a bit more flexible, so phylink gained its own fixed link
> implementation which did not emulate a PHY, just the results of
> talking to a multigige PHY.
>
> But i don't think you are actually talking about a PHY. I think you
> mean the PCS advertises CR/KR, and you want to emulate a fixed-link
> PCS? That is a different beast.
>
>         Andrew

A serdes PHY is part of it, but not a traditional twisted pair PHY as
we are talking about 25R, 50R(50GAUI & LAUI), and 100P interfaces. I
agree it is a different beast, but are we saying that the fixed-link
is supposed to be a twisted pair PHY only? That is the part I am
confused with as there are multiple scenarios where you might end up
with a fixed link configuration at a specific speed for something not
twisted pair. For example in our case the firmware provides us with
the fixed modulation/lanes/fec configuration so we can essentially
take that and treat it as a fixed-link configuration.

In addition one advantage is that it makes it possible to support
speeds that don't yet have a type in the phy_interface_t, so as I was
enabling things it allowed some backwards compatibility with older
kernels. In the case of fbnic I was planning to use pcs_validate to
strip down the supported features and essentially limit things based
on the bitrate per lane and the number of lanes. We were only using CR
so for us the result should only be 1 regardless based on the speed
match.

The general idea I had in mind for upstreaming the support for fbnic
was to initially bring it up as a fixed-link setup using
PHY_INTERFACE_MODE_INTERNAL as that is essentially what we have now
and I can get rid of the extraneous 40G stuff that we aren't using.
Then we pivot into enabling additional PHY interface modes and
QSFP+/28 support in the kernel. Then I would add a mailbox based i2c
and gpio to enable SFP/QSPF on fbnic. After that we could switch fbnic
back to the inband setup with support for the higher speed interfaces.

One option I would be open to is to have me take on addressing the
issue in net-next instead of net since I would need to sort it out to
enable my patches anyway. I was mostly bringing it up as I was
concerned that I may have not been the only one impacted. I was using
the fixed-link/internal combination to basically signal to the phylink
interface that we were locked in and weren't going to change it, as
such the only impact for me is it seemed to result in a warning
message about the link configuration not being recognized and the
supported/advertised modes being empty.

Re: [PATCH net-next v5 09/13] net: phylink: Use phy_caps_lookup for fixed-link configuration

[Maxime Chevallier]

On Fri, 28 Mar 2025 14:03:54 -0700
Alexander Duyck <alexander.duyck@gmail.com> wrote:

> On Fri, Mar 28, 2025 at 1:06 AM Maxime Chevallier
> <maxime.chevallier@bootlin.com> wrote:
> >
> > On Thu, 27 Mar 2025 18:16:00 -0700
> > Alexander H Duyck <alexander.duyck@gmail.com> wrote:
> >  
> > > On Fri, 2025-03-07 at 18:36 +0100, Maxime Chevallier wrote:  
> > > > When phylink creates a fixed-link configuration, it finds a matching
> > > > linkmode to set as the advertised, lp_advertising and supported modes
> > > > based on the speed and duplex of the fixed link.
> > > >
> > > > Use the newly introduced phy_caps_lookup to get these modes instead of
> > > > phy_lookup_settings(). This has the side effect that the matched
> > > > settings and configured linkmodes may now contain several linkmodes (the
> > > > intersection of supported linkmodes from the phylink settings and the
> > > > linkmodes that match speed/duplex) instead of the one from
> > > > phy_lookup_settings().
> > > >
> > > > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > > > ---
> > > >  drivers/net/phy/phylink.c | 44 +++++++++++++++++++++++++++------------
> > > >  1 file changed, 31 insertions(+), 13 deletions(-)
> > > >
> > > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > > > index cf9f019382ad..8e2b7d647a92 100644
> > > > --- a/drivers/net/phy/phylink.c
> > > > +++ b/drivers/net/phy/phylink.c
> > > > @@ -802,12 +802,26 @@ static int phylink_validate(struct phylink *pl, unsigned long *supported,
> > > >     return phylink_validate_mac_and_pcs(pl, supported, state);
> > > >  }
> > > >
> > > > +static void phylink_fill_fixedlink_supported(unsigned long *supported)
> > > > +{
> > > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT, supported);
> > > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT, supported);
> > > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT, supported);
> > > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT, supported);
> > > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT, supported);
> > > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, supported);
> > > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, supported);
> > > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, supported);
> > > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, supported);
> > > > +}
> > > > +  
> > >
> > > Any chance we can go with a different route here than just locking
> > > fixed mode to being only for BaseT configurations?
> > >
> > > I am currently working on getting the fbnic driver up and running and I
> > > am using fixed-link mode as a crutch until I can finish up enabling
> > > QSFP module support for phylink and this just knocked out the supported
> > > link modes as I was using 25CR, 50CR, 50CR2, and 100CR2.
> > >
> > > Seems like this should really be something handled by some sort of
> > > validation function rather than just forcing all devices using fixed
> > > link to assume that they are BaseT. I know in our direct attached
> > > copper case we aren't running autoneg so that plan was to use fixed
> > > link until we can add more flexibility by getting QSFP support going.  
> >
> > These baseT mode were for compatibility with the previous way to deal
> > with fixed links, but we can extend what's being report above 10G with
> > other modes. Indeed the above code unnecessarily restricts the
> > linkmodes. Can you tell me if the following patch works for you ?
> >
> > Maxime
> >
> > -----------
> >
> > From 595888afb23f209f2b1002d0b0406380195d53c1 Mon Sep 17 00:00:00 2001
> > From: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > Date: Fri, 28 Mar 2025 08:53:00 +0100
> > Subject: [PATCH] net: phylink: Allow fixed-link registration above 10G
> >
> > The blamed commit introduced a helper to keep the linkmodes reported by
> > fixed links identical to what they were before phy_caps. This means
> > filtering linkmodes only to report BaseT modes.
> >
> > Doing so, it also filtered out any linkmode above 10G. Reinstate the
> > reporting of linkmodes above 10G, where we report any linkmodes that
> > exist at these speeds.
> >
> > Reported-by: Alexander H Duyck <alexander.duyck@gmail.com>
> > Closes: https://lore.kernel.org/netdev/8d3a9c9bb76b1c6bc27d2bd01f4831b2cac83f7f.camel@gmail.com/
> > Fixes: de7d3f87be3c ("net: phylink: Use phy_caps_lookup for fixed-link configuration")
> > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > ---
> >  drivers/net/phy/phylink.c | 17 +++++++++++++----
> >  1 file changed, 13 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > index 69ca765485db..de90fed9c207 100644
> > --- a/drivers/net/phy/phylink.c
> > +++ b/drivers/net/phy/phylink.c
> > @@ -715,16 +715,25 @@ static int phylink_parse_fixedlink(struct phylink *pl,
> >                 phylink_warn(pl, "fixed link specifies half duplex for %dMbps link?\n",
> >                              pl->link_config.speed);
> >
> > -       linkmode_zero(pl->supported);
> > -       phylink_fill_fixedlink_supported(pl->supported);
> > +       linkmode_fill(pl->supported);
> >
> >         linkmode_copy(pl->link_config.advertising, pl->supported);
> >         phylink_validate(pl, pl->supported, &pl->link_config);
> >
> > +       phylink_fill_fixedlink_supported(match);
> > +  
> 
> I worry that this might put you back into the same problem again with
> the older drivers. In addition it is filling in modes that shouldn't
> be present after the validation.

Note that I'm not directly filling pl->supported here.

[...]

 	c = phy_caps_lookup(pl->link_config.speed, pl->link_config.duplex,
 			    pl->supported, true);
-	if (c)
-		linkmode_and(match, pl->supported, c->linkmodes);
+	if (c) {
+		/* Compatbility with the legacy behaviour : Report one single
+		 * BaseT mode for fixed-link speeds under or at 10G, or all
+		 * linkmodes at the speed/duplex for speeds over 10G
+		 */
+		if (linkmode_intersects(match, c->linkmodes))
+			linkmode_and(match, match, c->linkmodes);
+		else
+			linkmode_copy(match, c->linkmodes);
+	}

[...]

	if (c) {
		linkmode_or(pl->supported, pl->supported, match);
		linkmode_or(pl->link_config.lp_advertising,
			    pl->link_config.lp_advertising, match);
	}

For speeds above 10G, you will get all the modes for the requested
speed, so it should solve the issue in the next steps of your code
where you need matching linkmodes for your settings ? Did you give it a
try ?

You may end up with too many linkmodes, but for fixed-link we don't
really expect these modes to precisely represent any real linkmodes

> One thought on this. You might look at checking to see if the TP bit
> is set in the supported modes after validation and then use your
> fixedlink_supported as a mask if it is supporting twisted pair
> connections.

But the TP bit doesn't really mean much here, at least as of today.
What matters at that point really is the supported phy_interface_t and
your requested speed/duplex

> One possibility would be to go through and filter the
> modes based on the media type where you could basically filter out TP,
> Fiber, and Backplane connection types and use that as a second pass
> based on the settings by basically doing a set of AND and OR
> operations.

At that point, the linkmodes aren't very relevant, even if you look at
the old version of that same function, the linkmodes are built only
based on speed/duplex and do not represent any physical mode.

The media-specific filtering will come in a next series, I sent a few
iterations in last cycle to be able to filter out based on medium, I'll
CC you for the next round

> Also I am not sure it makes sense to say we can't support multiple
> modes on a fixed connection. For example in the case of SerDes links
> and the like it isn't unusual to see support for CR/KR advertised at
> the same speed on the same link and use the exact same configuration
> so a fixed config could support both and advertise both at the same
> time if I am not mistaken.

The use-cases for fixed links have mostly been about describing a link
that we know exists, but can't really control or query. In that case,
we don't even know what's the physical medium so we report pretty much
bogus values (as Andrew says, that used to be done by emulating a
copper PHY). As use-cases are evolving, I think we can consider indeed
a better coverage of your use-case :)

Maxime

Re: [PATCH net-next v5 09/13] net: phylink: Use phy_caps_lookup for fixed-link configuration

[Maxime Chevallier]

On Fri, 28 Mar 2025 16:26:04 -0700
Alexander Duyck <alexander.duyck@gmail.com> wrote:


> The general idea I had in mind for upstreaming the support for fbnic
> was to initially bring it up as a fixed-link setup using
> PHY_INTERFACE_MODE_INTERNAL as that is essentially what we have now
> and I can get rid of the extraneous 40G stuff that we aren't using.
> Then we pivot into enabling additional PHY interface modes and
> QSFP+/28 support in the kernel. Then I would add a mailbox based i2c
> and gpio to enable SFP/QSPF on fbnic. After that we could switch fbnic
> back to the inband setup with support for the higher speed interfaces.
> 
> One option I would be open to is to have me take on addressing the
> issue in net-next instead of net since I would need to sort it out to
> enable my patches anyway.

If that's OK for you and the proposed patch fixes your problem, I'd
still like for the patch to get in, as right now fixed links will
not work at all for >10G links.

Then we can get better linkmodes reporting for your usecase ?

Maxime

Re: [PATCH net-next v5 09/13] net: phylink: Use phy_caps_lookup for fixed-link configuration

[Russell King (Oracle)]

On Thu, Mar 27, 2025 at 06:16:00PM -0700, Alexander H Duyck wrote:
> On Fri, 2025-03-07 at 18:36 +0100, Maxime Chevallier wrote:
> > When phylink creates a fixed-link configuration, it finds a matching
> > linkmode to set as the advertised, lp_advertising and supported modes
> > based on the speed and duplex of the fixed link.
> > 
> > Use the newly introduced phy_caps_lookup to get these modes instead of
> > phy_lookup_settings(). This has the side effect that the matched
> > settings and configured linkmodes may now contain several linkmodes (the
> > intersection of supported linkmodes from the phylink settings and the
> > linkmodes that match speed/duplex) instead of the one from
> > phy_lookup_settings().
> > 
> > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > ---
> >  drivers/net/phy/phylink.c | 44 +++++++++++++++++++++++++++------------
> >  1 file changed, 31 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > index cf9f019382ad..8e2b7d647a92 100644
> > --- a/drivers/net/phy/phylink.c
> > +++ b/drivers/net/phy/phylink.c
> > @@ -802,12 +802,26 @@ static int phylink_validate(struct phylink *pl, unsigned long *supported,
> >  	return phylink_validate_mac_and_pcs(pl, supported, state);
> >  }
> >  
> > +static void phylink_fill_fixedlink_supported(unsigned long *supported)
> > +{
> > +	linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT, supported);
> > +	linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT, supported);
> > +	linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT, supported);
> > +	linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT, supported);
> > +	linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT, supported);
> > +	linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, supported);
> > +	linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, supported);
> > +	linkmode_set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, supported);
> > +	linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, supported);
> > +}
> > +
> 
> Any chance we can go with a different route here than just locking
> fixed mode to being only for BaseT configurations?
> 
> I am currently working on getting the fbnic driver up and running and I
> am using fixed-link mode as a crutch until I can finish up enabling
> QSFP module support for phylink and this just knocked out the supported
> link modes as I was using 25CR, 50CR, 50CR2, and 100CR2.
> 
> Seems like this should really be something handled by some sort of
> validation function rather than just forcing all devices using fixed
> link to assume that they are BaseT. I know in our direct attached
> copper case we aren't running autoneg so that plan was to use fixed
> link until we can add more flexibility by getting QSFP support going.

Please look back at phylink's historical behaviour. Phylink used to
use phy_lookup_setting() to locate the linkmode for the speed and
duplex. That is the behaviour that we should be aiming to preserve.

We were getting:

speed	duplex	linkmode
10M	Half	10baseT_Half
10M	Full	10baseT_Full
100M	Half	100baseT_Half
100M	Full	100baseT_Full
1G	Half	1000baseT_Half
1G	Full	1000baseT_Full (this changed over time)
2.5G	Full	2500baseT_Full
5G	Full	5000baseT_Full

At this point, things get weird because of the way linkmodes were
added, as we return the _first_ match. Before commit 3c6b59d6f07c
("net: phy: Add more link modes to the settings table"):

10G	Full	10000baseKR_Full
Faster speeds not supported

After the commit:
10G	Full	10000baseCR_Full
20G	Full	20000baseKR2_Full
25G	Full	25000baseCR_Full
40G	Full	40000baseCR4_Full
50G	Full	50000baseCR2_Full
56G	Full	56000baseCR4_Full
100G	Full	100000baseCR4_Full

It's all a bit random. :(

-- 
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 v5 09/13] net: phylink: Use phy_caps_lookup for fixed-link configuration

[Andrew Lunn]

On Fri, Mar 28, 2025 at 04:26:04PM -0700, Alexander Duyck wrote:
> On Fri, Mar 28, 2025 at 2:45 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > > Also I am not sure it makes sense to say we can't support multiple
> > > modes on a fixed connection. For example in the case of SerDes links
> > > and the like it isn't unusual to see support for CR/KR advertised at
> > > the same speed on the same link and use the exact same configuration
> > > so a fixed config could support both and advertise both at the same
> > > time if I am not mistaken.
> >
> > Traditionally, fixed link has only supported one mode. The combination
> > of speed and duplex fully describes a base-T link. Even more
> > traditionally, it was implemented as an emulated C22 PHY, using the
> > genphy driver, so limited to just 1G. With multigige PHY we needed to
> > be a bit more flexible, so phylink gained its own fixed link
> > implementation which did not emulate a PHY, just the results of
> > talking to a multigige PHY.
> >
> > But i don't think you are actually talking about a PHY. I think you
> > mean the PCS advertises CR/KR, and you want to emulate a fixed-link
> > PCS? That is a different beast.
> >
> >         Andrew
> 
> A serdes PHY is part of it, but not a traditional twisted pair PHY as
> we are talking about 25R, 50R(50GAUI & LAUI), and 100P interfaces. I
> agree it is a different beast, but are we saying that the fixed-link
> is supposed to be a twisted pair PHY only?

With phylink, the PCS enumerates its capabilities, the PHY enumerates
its capabilities, and the MAC enumerates it capabilities. phylink then
finds the subset which all support.

As i said, historically, fixed_link was used in place of a PHY, since
it emulated a PHY. phylinks implementation of fixed_link is however
different. Can it be used in place of both a PCS and a PHY? I don't
know.

You are pushing the envelope here, and maybe we need to take a step
back and consider what is a fixed link, how does it fit into the MAC,
PCS, PHY model of enumeration? Maybe fixed link should only represent
the PHY and we need a second sort of fixed_link object to represent
the PCS? I don't know?

> In addition one advantage is that it makes it possible to support
> speeds that don't yet have a type in the phy_interface_t, so as I was
> enabling things it allowed some backwards compatibility with older
> kernels.

I don't like the sound of that. I would simply not support the older
kernels, rather than do some hacks.

	 Andrew

Re: [PATCH net-next v5 09/13] net: phylink: Use phy_caps_lookup for fixed-link configuration

[Russell King (Oracle)]

On Mon, Mar 31, 2025 at 04:17:02PM +0200, Andrew Lunn wrote:
> On Fri, Mar 28, 2025 at 04:26:04PM -0700, Alexander Duyck wrote:
> > A serdes PHY is part of it, but not a traditional twisted pair PHY as
> > we are talking about 25R, 50R(50GAUI & LAUI), and 100P interfaces. I
> > agree it is a different beast, but are we saying that the fixed-link
> > is supposed to be a twisted pair PHY only?
> 
> With phylink, the PCS enumerates its capabilities, the PHY enumerates
> its capabilities, and the MAC enumerates it capabilities. phylink then
> finds the subset which all support.
> 
> As i said, historically, fixed_link was used in place of a PHY, since
> it emulated a PHY. phylinks implementation of fixed_link is however
> different. Can it be used in place of both a PCS and a PHY? I don't
> know.

In fixed-link mode, phylink will use a PCS if the MAC driver says there
is one, but it will not look for a PHY.

> You are pushing the envelope here, and maybe we need to take a step
> back and consider what is a fixed link, how does it fit into the MAC,
> PCS, PHY model of enumeration? Maybe fixed link should only represent
> the PHY and we need a second sort of fixed_link object to represent
> the PCS? I don't know?

As I previously wrote today in response to an earlier email, the
link modes that phylink used were the first-match from the old
settings[] array in phylib which is now gone. This would only ever
return _one_ link mode, which invariably was a baseT link mode for
the slower speeds.

Maxime's first approach at adapting this to his new system was to
set every single link mode that corresponded with the speed. I
objected to that, because it quickly gets rediculous when we end
up with lots of link modes being indicated for e.g. 10, 100M, 1G
but the emulated PHY for these speeds only indicates baseT. That's
just back-compatibility but... in principle changing the link modes
that are reported to userspace for a fixed link is something we
should not be doing - we don't know if userspace tooling has come
to rely on that.

Yes, it's a bit weird to be reporting 1000baseT for a 1000BASE-X
interface mode, but that's what we've always done in the past and
phylink was coded to maintain that (following the principle that
we shouldn't do gratuitous changes to the information exposed to
userspace.)

Maxime's replacement approach is to just expose baseT, which
means that for the speeds which do not have a baseT mode, we go
from supporting it but with a weird link mode (mostly baseCR*)
based on first-match in the settings[] table, to not supporting the
speed.

> > In addition one advantage is that it makes it possible to support
> > speeds that don't yet have a type in the phy_interface_t, so as I was
> > enabling things it allowed some backwards compatibility with older
> > kernels.
> 
> I don't like the sound of that. I would simply not support the older
> kernels, rather than do some hacks.

I think Alexander is referring to having a PHY interface modes that
supports the speed - for example, if we didn't have a PHY interface
mode that supported 100G speeds, then 100G speed would not be
supported.

Phylink has already restricted this and has done for quite some time.
We only allow speeds that the selected interface mode can support,
with the exception of PHY_INTERFACE_MODE_INTERNAL which supports
everything.

-- 
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 v5 09/13] net: phylink: Use phy_caps_lookup for fixed-link configuration

[Maxime Chevallier]

On Mon, 31 Mar 2025 15:54:20 +0100
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:

> On Mon, Mar 31, 2025 at 04:17:02PM +0200, Andrew Lunn wrote:
> > On Fri, Mar 28, 2025 at 04:26:04PM -0700, Alexander Duyck wrote:  
> > > A serdes PHY is part of it, but not a traditional twisted pair PHY as
> > > we are talking about 25R, 50R(50GAUI & LAUI), and 100P interfaces. I
> > > agree it is a different beast, but are we saying that the fixed-link
> > > is supposed to be a twisted pair PHY only?  
> > 
> > With phylink, the PCS enumerates its capabilities, the PHY enumerates
> > its capabilities, and the MAC enumerates it capabilities. phylink then
> > finds the subset which all support.
> > 
> > As i said, historically, fixed_link was used in place of a PHY, since
> > it emulated a PHY. phylinks implementation of fixed_link is however
> > different. Can it be used in place of both a PCS and a PHY? I don't
> > know.  
> 
> In fixed-link mode, phylink will use a PCS if the MAC driver says there
> is one, but it will not look for a PHY.
> 
> > You are pushing the envelope here, and maybe we need to take a step
> > back and consider what is a fixed link, how does it fit into the MAC,
> > PCS, PHY model of enumeration? Maybe fixed link should only represent
> > the PHY and we need a second sort of fixed_link object to represent
> > the PCS? I don't know?  
> 
> As I previously wrote today in response to an earlier email, the
> link modes that phylink used were the first-match from the old
> settings[] array in phylib which is now gone. This would only ever
> return _one_ link mode, which invariably was a baseT link mode for
> the slower speeds.
> 
> Maxime's first approach at adapting this to his new system was to
> set every single link mode that corresponded with the speed. I
> objected to that, because it quickly gets rediculous when we end
> up with lots of link modes being indicated for e.g. 10, 100M, 1G
> but the emulated PHY for these speeds only indicates baseT. That's
> just back-compatibility but... in principle changing the link modes
> that are reported to userspace for a fixed link is something we
> should not be doing - we don't know if userspace tooling has come
> to rely on that.
> 
> Yes, it's a bit weird to be reporting 1000baseT for a 1000BASE-X
> interface mode, but that's what we've always done in the past and
> phylink was coded to maintain that (following the principle that
> we shouldn't do gratuitous changes to the information exposed to
> userspace.)
> 
> Maxime's replacement approach is to just expose baseT, which
> means that for the speeds which do not have a baseT mode, we go
> from supporting it but with a weird link mode (mostly baseCR*)
> based on first-match in the settings[] table, to not supporting the
> speed.

I very wrongfully considered that there was no >10G fixed-link users, I
plan to fix that with something like the proposed patch in the
discussion, that reports all linkmodes for speeds above 10G (looks less
like a randomly selected mode, you can kind-of see what's going on as
you get all the linkmodes) but is a change in what we expose to
userspace.

Or maybe simpler, I could extend the list of compat fixed-link linkmodes
to all speeds with the previous arbitrary values that Russell listed in
the other mail (that way, no user-visible changes :) )

I was hoping Alexander could give option 1 a try, but let me know if
you think we should instead adopt option 2, which is probably the safer
on.

Maxime

Re: [PATCH net-next v5 09/13] net: phylink: Use phy_caps_lookup for fixed-link configuration

[Alexander Duyck]

On Mon, Mar 31, 2025 at 9:20 AM Maxime Chevallier
<maxime.chevallier@bootlin.com> wrote:
>
> On Mon, 31 Mar 2025 15:54:20 +0100
> "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
>
> > On Mon, Mar 31, 2025 at 04:17:02PM +0200, Andrew Lunn wrote:
> > > On Fri, Mar 28, 2025 at 04:26:04PM -0700, Alexander Duyck wrote:
> > > > A serdes PHY is part of it, but not a traditional twisted pair PHY as
> > > > we are talking about 25R, 50R(50GAUI & LAUI), and 100P interfaces. I
> > > > agree it is a different beast, but are we saying that the fixed-link
> > > > is supposed to be a twisted pair PHY only?
> > >
> > > With phylink, the PCS enumerates its capabilities, the PHY enumerates
> > > its capabilities, and the MAC enumerates it capabilities. phylink then
> > > finds the subset which all support.
> > >
> > > As i said, historically, fixed_link was used in place of a PHY, since
> > > it emulated a PHY. phylinks implementation of fixed_link is however
> > > different. Can it be used in place of both a PCS and a PHY? I don't
> > > know.
> >
> > In fixed-link mode, phylink will use a PCS if the MAC driver says there
> > is one, but it will not look for a PHY.

Admittedly the documentation does reference much lower speeds as being
the use case. I was a bit of an eager beaver and started assembling
things without really reading the directions. I just kind of assumed
what I could or couldn't get away with within the interface.

> > > You are pushing the envelope here, and maybe we need to take a step
> > > back and consider what is a fixed link, how does it fit into the MAC,
> > > PCS, PHY model of enumeration? Maybe fixed link should only represent
> > > the PHY and we need a second sort of fixed_link object to represent
> > > the PCS? I don't know?
> >
> > As I previously wrote today in response to an earlier email, the
> > link modes that phylink used were the first-match from the old
> > settings[] array in phylib which is now gone. This would only ever
> > return _one_ link mode, which invariably was a baseT link mode for
> > the slower speeds.
> >
> > Maxime's first approach at adapting this to his new system was to
> > set every single link mode that corresponded with the speed. I
> > objected to that, because it quickly gets rediculous when we end
> > up with lots of link modes being indicated for e.g. 10, 100M, 1G
> > but the emulated PHY for these speeds only indicates baseT. That's
> > just back-compatibility but... in principle changing the link modes
> > that are reported to userspace for a fixed link is something we
> > should not be doing - we don't know if userspace tooling has come
> > to rely on that.
> >
> > Yes, it's a bit weird to be reporting 1000baseT for a 1000BASE-X
> > interface mode, but that's what we've always done in the past and
> > phylink was coded to maintain that (following the principle that
> > we shouldn't do gratuitous changes to the information exposed to
> > userspace.)
> >
> > Maxime's replacement approach is to just expose baseT, which
> > means that for the speeds which do not have a baseT mode, we go
> > from supporting it but with a weird link mode (mostly baseCR*)
> > based on first-match in the settings[] table, to not supporting the
> > speed.
>
> I very wrongfully considered that there was no >10G fixed-link users, I
> plan to fix that with something like the proposed patch in the
> discussion, that reports all linkmodes for speeds above 10G (looks less
> like a randomly selected mode, you can kind-of see what's going on as
> you get all the linkmodes) but is a change in what we expose to
> userspace.

I am not sure if there are any >10G users. I haven't landed anything
in the kernel yet and like I said what I was doing was more of a hack
to enable backwards compatibility on older kernels w/ the correct
supported and advertised modes. If I have to patch one kernel to make
it work for me that would be manageable.

One thing I was thinking about that it looks like this code might
prevent would be reinterpreting the meaning of duplex. Currently we
only have 3 values for it 0 (half), 1 (Full), and ~0 (Unknown). One
thought I had is that once we are over 1G we don't really care about
that anymore as everything is Full duplex and instead care about
lanes. As it turns out the duplex values currently used would work
well to be extended out to lanes. Essentially 0 would still be half, 1
would be 1 lane full duplex, 2-8 could be the number of full duplex
lanes the interface is using, and unknown lane count would still be ~0
since it is unlikely we will end up with anything other than a power
of 2 number of lanes anyway. With that you could greatly sort out a
number of modes in your setup. We would then have to do some cleanups
here and there to do something like "duplex == DUPLEX_UNKNOWN ? duplex
: !!duplex" to clean up any cases where the legacy values are
expected.

Likewise if you were to look at adding the port type that might allow
for further division and cleanup. With that someone could specify the
speed, duplex, and port type and they would be able to pretty
precisely pick out a specific fixed mode.

> Or maybe simpler, I could extend the list of compat fixed-link linkmodes
> to all speeds with the previous arbitrary values that Russell listed in
> the other mail (that way, no user-visible changes :) )
>
> I was hoping Alexander could give option 1 a try, but let me know if
> you think we should instead adopt option 2, which is probably the safer
> on.

I can try to get to it, but I have a number of meetings today so I may
not be able to get to it until tomorrow morning.

Also I suspect this may have an impact outside of just the fixed link
setup. I will have to try some other spots to see if I see anything
odd pop up as I suspect that I will have issues with 50R2/50R running
over top of each other after these changes.

Thanks,

- Alex

Re: [PATCH net-next v5 09/13] net: phylink: Use phy_caps_lookup for fixed-link configuration

[Alexander Duyck]

On Mon, Mar 31, 2025 at 5:51 AM Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
>
> On Thu, Mar 27, 2025 at 06:16:00PM -0700, Alexander H Duyck wrote:
> > On Fri, 2025-03-07 at 18:36 +0100, Maxime Chevallier wrote:
> > > When phylink creates a fixed-link configuration, it finds a matching
> > > linkmode to set as the advertised, lp_advertising and supported modes
> > > based on the speed and duplex of the fixed link.
> > >
> > > Use the newly introduced phy_caps_lookup to get these modes instead of
> > > phy_lookup_settings(). This has the side effect that the matched
> > > settings and configured linkmodes may now contain several linkmodes (the
> > > intersection of supported linkmodes from the phylink settings and the
> > > linkmodes that match speed/duplex) instead of the one from
> > > phy_lookup_settings().
> > >
> > > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > > ---
> > >  drivers/net/phy/phylink.c | 44 +++++++++++++++++++++++++++------------
> > >  1 file changed, 31 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > > index cf9f019382ad..8e2b7d647a92 100644
> > > --- a/drivers/net/phy/phylink.c
> > > +++ b/drivers/net/phy/phylink.c
> > > @@ -802,12 +802,26 @@ static int phylink_validate(struct phylink *pl, unsigned long *supported,
> > >     return phylink_validate_mac_and_pcs(pl, supported, state);
> > >  }
> > >
> > > +static void phylink_fill_fixedlink_supported(unsigned long *supported)
> > > +{
> > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT, supported);
> > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT, supported);
> > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT, supported);
> > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT, supported);
> > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT, supported);
> > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, supported);
> > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, supported);
> > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, supported);
> > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, supported);
> > > +}
> > > +
> >
> > Any chance we can go with a different route here than just locking
> > fixed mode to being only for BaseT configurations?
> >
> > I am currently working on getting the fbnic driver up and running and I
> > am using fixed-link mode as a crutch until I can finish up enabling
> > QSFP module support for phylink and this just knocked out the supported
> > link modes as I was using 25CR, 50CR, 50CR2, and 100CR2.
> >
> > Seems like this should really be something handled by some sort of
> > validation function rather than just forcing all devices using fixed
> > link to assume that they are BaseT. I know in our direct attached
> > copper case we aren't running autoneg so that plan was to use fixed
> > link until we can add more flexibility by getting QSFP support going.
>
> Please look back at phylink's historical behaviour. Phylink used to
> use phy_lookup_setting() to locate the linkmode for the speed and
> duplex. That is the behaviour that we should be aiming to preserve.
>
> We were getting:
>
> speed   duplex  linkmode
> 10M     Half    10baseT_Half
> 10M     Full    10baseT_Full
> 100M    Half    100baseT_Half
> 100M    Full    100baseT_Full
> 1G      Half    1000baseT_Half
> 1G      Full    1000baseT_Full (this changed over time)
> 2.5G    Full    2500baseT_Full
> 5G      Full    5000baseT_Full
>
> At this point, things get weird because of the way linkmodes were
> added, as we return the _first_ match. Before commit 3c6b59d6f07c
> ("net: phy: Add more link modes to the settings table"):
>
> 10G     Full    10000baseKR_Full
> Faster speeds not supported
>
> After the commit:
> 10G     Full    10000baseCR_Full
> 20G     Full    20000baseKR2_Full
> 25G     Full    25000baseCR_Full
> 40G     Full    40000baseCR4_Full
> 50G     Full    50000baseCR2_Full
> 56G     Full    56000baseCR4_Full
> 100G    Full    100000baseCR4_Full
>
> It's all a bit random. :(

I agree. I was using pcs_validate to overcome some of that by limiting
myself to *mostly* one link type at each speed. The only spot that got
a bit iffy was the 50G as I support 50GAUI and LAUI-2. I was
overcoming that by tracking the number of lanes expected and filtering
for one or the other.

One thought I had proposed in another email was to look at adding more
data to the fields. In the case of duplex we could overload it to also
be the number of lanes as they represent full duplex lanes anyway.
With that you could sort out some of the CR vs CR2 noise.

In addition I wonder if we shouldn't also look at including port type
in the data. Using that you could essentially go through the
post-validated supported values and OR in the types that belong to the
various link modes for TP, FIBER, DA, ect. That would sort out some of
the KR vs CR vs SR vs LR noise.

Re: [PATCH net-next v5 09/13] net: phylink: Use phy_caps_lookup for fixed-link configuration

[Alexander H Duyck]

On Mon, 2025-03-31 at 18:20 +0200, Maxime Chevallier wrote:
> On Mon, 31 Mar 2025 15:54:20 +0100
> "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:

...

> I was hoping Alexander could give option 1 a try, but let me know if
> you think we should instead adopt option 2, which is probably the safer
> on.
> 
> Maxime

So I gave it a try, but the results weren't promising. I ended up
getting the lp_advertised spammed with all the modes:

    Link partner advertised link modes:  100000baseKR4/Full
                                         100000baseSR4/Full
                                         100000baseCR4/Full
                                         100000baseLR4_ER4/Full
                                         100000baseKR2/Full
                                         100000baseSR2/Full
                                         100000baseCR2/Full
                                         100000baseLR2_ER2_FR2/Full
                                         100000baseDR2/Full
                                         100000baseKR/Full
                                         100000baseSR/Full
                                         100000baseLR_ER_FR/Full
                                         100000baseCR/Full
                                         100000baseDR/Full


In order to resolve it I just made the following change:
@@ -713,9 +700,7 @@ static int phylink_parse_fixedlink(struct phylink
*pl,
                phylink_warn(pl, "fixed link specifies half duplex for
%dMbps link?\n",
                             pl->link_config.speed);
 
-       linkmode_zero(pl->supported);
-       phylink_fill_fixedlink_supported(pl->supported);
-
+       linkmode_fill(pl->supported);
        linkmode_copy(pl->link_config.advertising, pl->supported);
        phylink_validate(pl, pl->supported, &pl->link_config);



Basically the issue is that I am using the pcs_validate to cleanup my
link modes. So the code below this point worked correctly for me. The
only issue was the dropping of the other bits.

That is why I mentioned the possibility of maybe adding some sort of
follow-on filter function that would go through the upper bits and or
them into the filter being run after the original one.

For example there is mask which is used to filter out everything but
the pause and autoneg bits. Perhaps we should assemble bits there
depending on the TP, FIBER, and BACKPLANE bits to clean out everything
but CR, KR, and TP types if those bits are set.

Re: [PATCH net-next v5 09/13] net: phylink: Use phy_caps_lookup for fixed-link configuration

[Maxime Chevallier]

On Mon, 31 Mar 2025 09:38:34 -0700
Alexander Duyck <alexander.duyck@gmail.com> wrote:

> On Mon, Mar 31, 2025 at 9:20 AM Maxime Chevallier
> <maxime.chevallier@bootlin.com> wrote:
> >
> > On Mon, 31 Mar 2025 15:54:20 +0100
> > "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> >  
> > > On Mon, Mar 31, 2025 at 04:17:02PM +0200, Andrew Lunn wrote:  
> > > > On Fri, Mar 28, 2025 at 04:26:04PM -0700, Alexander Duyck wrote:  
> > > > > A serdes PHY is part of it, but not a traditional twisted pair PHY as
> > > > > we are talking about 25R, 50R(50GAUI & LAUI), and 100P interfaces. I
> > > > > agree it is a different beast, but are we saying that the fixed-link
> > > > > is supposed to be a twisted pair PHY only?  
> > > >
> > > > With phylink, the PCS enumerates its capabilities, the PHY enumerates
> > > > its capabilities, and the MAC enumerates it capabilities. phylink then
> > > > finds the subset which all support.
> > > >
> > > > As i said, historically, fixed_link was used in place of a PHY, since
> > > > it emulated a PHY. phylinks implementation of fixed_link is however
> > > > different. Can it be used in place of both a PCS and a PHY? I don't
> > > > know.  
> > >
> > > In fixed-link mode, phylink will use a PCS if the MAC driver says there
> > > is one, but it will not look for a PHY.  
> 
> Admittedly the documentation does reference much lower speeds as being
> the use case. I was a bit of an eager beaver and started assembling
> things without really reading the directions. I just kind of assumed
> what I could or couldn't get away with within the interface.
> 
> > > > You are pushing the envelope here, and maybe we need to take a step
> > > > back and consider what is a fixed link, how does it fit into the MAC,
> > > > PCS, PHY model of enumeration? Maybe fixed link should only represent
> > > > the PHY and we need a second sort of fixed_link object to represent
> > > > the PCS? I don't know?  
> > >
> > > As I previously wrote today in response to an earlier email, the
> > > link modes that phylink used were the first-match from the old
> > > settings[] array in phylib which is now gone. This would only ever
> > > return _one_ link mode, which invariably was a baseT link mode for
> > > the slower speeds.
> > >
> > > Maxime's first approach at adapting this to his new system was to
> > > set every single link mode that corresponded with the speed. I
> > > objected to that, because it quickly gets rediculous when we end
> > > up with lots of link modes being indicated for e.g. 10, 100M, 1G
> > > but the emulated PHY for these speeds only indicates baseT. That's
> > > just back-compatibility but... in principle changing the link modes
> > > that are reported to userspace for a fixed link is something we
> > > should not be doing - we don't know if userspace tooling has come
> > > to rely on that.
> > >
> > > Yes, it's a bit weird to be reporting 1000baseT for a 1000BASE-X
> > > interface mode, but that's what we've always done in the past and
> > > phylink was coded to maintain that (following the principle that
> > > we shouldn't do gratuitous changes to the information exposed to
> > > userspace.)
> > >
> > > Maxime's replacement approach is to just expose baseT, which
> > > means that for the speeds which do not have a baseT mode, we go
> > > from supporting it but with a weird link mode (mostly baseCR*)
> > > based on first-match in the settings[] table, to not supporting the
> > > speed.  
> >
> > I very wrongfully considered that there was no >10G fixed-link users, I
> > plan to fix that with something like the proposed patch in the
> > discussion, that reports all linkmodes for speeds above 10G (looks less
> > like a randomly selected mode, you can kind-of see what's going on as
> > you get all the linkmodes) but is a change in what we expose to
> > userspace.  
> 
> I am not sure if there are any >10G users. I haven't landed anything
> in the kernel yet and like I said what I was doing was more of a hack
> to enable backwards compatibility on older kernels w/ the correct
> supported and advertised modes. If I have to patch one kernel to make
> it work for me that would be manageable.
> 
> One thing I was thinking about that it looks like this code might
> prevent would be reinterpreting the meaning of duplex. Currently we
> only have 3 values for it 0 (half), 1 (Full), and ~0 (Unknown). One
> thought I had is that once we are over 1G we don't really care about
> that anymore as everything is Full duplex and instead care about
> lanes. As it turns out the duplex values currently used would work
> well to be extended out to lanes. Essentially 0 would still be half, 1
> would be 1 lane full duplex, 2-8 could be the number of full duplex
> lanes the interface is using, and unknown lane count would still be ~0
> since it is unlikely we will end up with anything other than a power
> of 2 number of lanes anyway. With that you could greatly sort out a
> number of modes in your setup. We would then have to do some cleanups
> here and there to do something like "duplex == DUPLEX_UNKNOWN ? duplex
> : !!duplex" to clean up any cases where the legacy values are
> expected.

Funny you say that, the phy_port work I was referring to with the
mediums introduction also tracks the lanes for a given port 

previous outdated iteration here
: https://lore.kernel.org/netdev/20250213101606.1154014-4-maxime.chevallier@bootlin.com/

The idea is to represent physical interfaces to a NIC, as NICs can have
multiple ports for different types of connectors. Ports would be driven
by either a PHY, a NIC directly or an SFP.

The port contains, among other things, speed, duplex, n_lanes, and the
medium (with more granularity than just TP/Backplane/Fiber, as we list
the 802.3 media : BaseS / BaseC / BaseK / BaseT ...)

We already have information about whickh linkmode uses how many lanes,
but we don't do much with that information besides reporting it to
userspace.

I'll revamp the whole thing for the next iteration, however I really
only have a deep-ish understanding of embedded usecases, and I lack
insights on other classes of devices such as what your working on.

I'll be very happy to get your feedback on it, I plan to send that when
net-next reopens.

Maxime

Re: [PATCH net-next v5 09/13] net: phylink: Use phy_caps_lookup for fixed-link configuration

[Maxime Chevallier]

Hi Alexander,

On Mon, 31 Mar 2025 15:31:23 -0700
Alexander H Duyck <alexander.duyck@gmail.com> wrote:

> On Mon, 2025-03-31 at 18:20 +0200, Maxime Chevallier wrote:
> > On Mon, 31 Mar 2025 15:54:20 +0100
> > "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:  
> 
> ...
> 
> > I was hoping Alexander could give option 1 a try, but let me know if
> > you think we should instead adopt option 2, which is probably the safer
> > on.
> > 
> > Maxime  
> 
> So I gave it a try, but the results weren't promising. I ended up
> getting the lp_advertised spammed with all the modes:
> 
>     Link partner advertised link modes:  100000baseKR4/Full
>                                          100000baseSR4/Full
>                                          100000baseCR4/Full
>                                          100000baseLR4_ER4/Full
>                                          100000baseKR2/Full
>                                          100000baseSR2/Full
>                                          100000baseCR2/Full
>                                          100000baseLR2_ER2_FR2/Full
>                                          100000baseDR2/Full
>                                          100000baseKR/Full
>                                          100000baseSR/Full
>                                          100000baseLR_ER_FR/Full
>                                          100000baseCR/Full
>                                          100000baseDR/Full

Thanks for testing, that's the expected outcome for the patch though. Is
that really an issue ? For fixed links, what report is bogus
anyways :/ I guess here as you mentioned, the problem is that you don't
actually have a real fixed link :)

> 
> In order to resolve it I just made the following change:
> @@ -713,9 +700,7 @@ static int phylink_parse_fixedlink(struct phylink
> *pl,
>                 phylink_warn(pl, "fixed link specifies half duplex for
> %dMbps link?\n",
>                              pl->link_config.speed);
>  
> -       linkmode_zero(pl->supported);
> -       phylink_fill_fixedlink_supported(pl->supported);
> -
> +       linkmode_fill(pl->supported);
>         linkmode_copy(pl->link_config.advertising, pl->supported);
>         phylink_validate(pl, pl->supported, &pl->link_config);
> 

So this goes back to the <10G modes reporting multiple modes then ?

> 
> Basically the issue is that I am using the pcs_validate to cleanup my
> link modes. So the code below this point worked correctly for me. The
> only issue was the dropping of the other bits.
> 
> That is why I mentioned the possibility of maybe adding some sort of
> follow-on filter function that would go through the upper bits and or
> them into the filter being run after the original one.
> 
> For example there is mask which is used to filter out everything but
> the pause and autoneg bits. Perhaps we should assemble bits there
> depending on the TP, FIBER, and BACKPLANE bits to clean out everything
> but CR, KR, and TP types if those bits are set.

Yeah but where do we get these TP / Fiber / Backplane bits from ? We
build that list of linkmodes from scratch in that function, only based
on speed/duplex, we don't know anything about the physical medium at
that point.

What you are suggesting is something I'm adding in the phy_port work
actually. You'll be able to say : "This port is a BaseK port" or
"BaseT" or "it may be baseC or baseL or baseS" and there'll be some
filtering based on that, although only in what we report to userspace,
at least for now :)

Maxime

Re: [PATCH net-next v5 09/13] net: phylink: Use phy_caps_lookup for fixed-link configuration

[Alexander H Duyck]

On Mon, 2025-03-31 at 09:14 +0200, Maxime Chevallier wrote:
> On Fri, 28 Mar 2025 14:03:54 -0700
> Alexander Duyck <alexander.duyck@gmail.com> wrote:
> 
> > On Fri, Mar 28, 2025 at 1:06 AM Maxime Chevallier
> > <maxime.chevallier@bootlin.com> wrote:
> > > 
> > > On Thu, 27 Mar 2025 18:16:00 -0700
> > > Alexander H Duyck <alexander.duyck@gmail.com> wrote:
> > >  
> > > > On Fri, 2025-03-07 at 18:36 +0100, Maxime Chevallier wrote:  
> > > > > When phylink creates a fixed-link configuration, it finds a matching
> > > > > linkmode to set as the advertised, lp_advertising and supported modes
> > > > > based on the speed and duplex of the fixed link.
> > > > > 
> > > > > Use the newly introduced phy_caps_lookup to get these modes instead of
> > > > > phy_lookup_settings(). This has the side effect that the matched
> > > > > settings and configured linkmodes may now contain several linkmodes (the
> > > > > intersection of supported linkmodes from the phylink settings and the
> > > > > linkmodes that match speed/duplex) instead of the one from
> > > > > phy_lookup_settings().
> > > > > 
> > > > > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > > > > ---
> > > > >  drivers/net/phy/phylink.c | 44 +++++++++++++++++++++++++++------------
> > > > >  1 file changed, 31 insertions(+), 13 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > > > > index cf9f019382ad..8e2b7d647a92 100644
> > > > > --- a/drivers/net/phy/phylink.c
> > > > > +++ b/drivers/net/phy/phylink.c
> > > > > @@ -802,12 +802,26 @@ static int phylink_validate(struct phylink *pl, unsigned long *supported,
> > > > >     return phylink_validate_mac_and_pcs(pl, supported, state);
> > > > >  }
> > > > > 
> > > > > +static void phylink_fill_fixedlink_supported(unsigned long *supported)
> > > > > +{
> > > > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT, supported);
> > > > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT, supported);
> > > > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT, supported);
> > > > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT, supported);
> > > > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT, supported);
> > > > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, supported);
> > > > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, supported);
> > > > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, supported);
> > > > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, supported);
> > > > > +}
> > > > > +  
> > > > 
> > > > Any chance we can go with a different route here than just locking
> > > > fixed mode to being only for BaseT configurations?
> > > > 
> > > > I am currently working on getting the fbnic driver up and running and I
> > > > am using fixed-link mode as a crutch until I can finish up enabling
> > > > QSFP module support for phylink and this just knocked out the supported
> > > > link modes as I was using 25CR, 50CR, 50CR2, and 100CR2.
> > > > 
> > > > Seems like this should really be something handled by some sort of
> > > > validation function rather than just forcing all devices using fixed
> > > > link to assume that they are BaseT. I know in our direct attached
> > > > copper case we aren't running autoneg so that plan was to use fixed
> > > > link until we can add more flexibility by getting QSFP support going.  
> > > 
> > > These baseT mode were for compatibility with the previous way to deal
> > > with fixed links, but we can extend what's being report above 10G with
> > > other modes. Indeed the above code unnecessarily restricts the
> > > linkmodes. Can you tell me if the following patch works for you ?
> > > 
> > > Maxime
> > > 
> > > -----------
> > > 
> > > From 595888afb23f209f2b1002d0b0406380195d53c1 Mon Sep 17 00:00:00 2001
> > > From: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > > Date: Fri, 28 Mar 2025 08:53:00 +0100
> > > Subject: [PATCH] net: phylink: Allow fixed-link registration above 10G
> > > 
> > > The blamed commit introduced a helper to keep the linkmodes reported by
> > > fixed links identical to what they were before phy_caps. This means
> > > filtering linkmodes only to report BaseT modes.
> > > 
> > > Doing so, it also filtered out any linkmode above 10G. Reinstate the
> > > reporting of linkmodes above 10G, where we report any linkmodes that
> > > exist at these speeds.
> > > 
> > > Reported-by: Alexander H Duyck <alexander.duyck@gmail.com>
> > > Closes: https://lore.kernel.org/netdev/8d3a9c9bb76b1c6bc27d2bd01f4831b2cac83f7f.camel@gmail.com/
> > > Fixes: de7d3f87be3c ("net: phylink: Use phy_caps_lookup for fixed-link configuration")
> > > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > > ---
> > >  drivers/net/phy/phylink.c | 17 +++++++++++++----
> > >  1 file changed, 13 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > > index 69ca765485db..de90fed9c207 100644
> > > --- a/drivers/net/phy/phylink.c
> > > +++ b/drivers/net/phy/phylink.c
> > > @@ -715,16 +715,25 @@ static int phylink_parse_fixedlink(struct phylink *pl,
> > >                 phylink_warn(pl, "fixed link specifies half duplex for %dMbps link?\n",
> > >                              pl->link_config.speed);
> > > 
> > > -       linkmode_zero(pl->supported);
> > > -       phylink_fill_fixedlink_supported(pl->supported);
> > > +       linkmode_fill(pl->supported);
> > > 
> > >         linkmode_copy(pl->link_config.advertising, pl->supported);
> > >         phylink_validate(pl, pl->supported, &pl->link_config);
> > > 
> > > +       phylink_fill_fixedlink_supported(match);
> > > +  
> > 
> > I worry that this might put you back into the same problem again with
> > the older drivers. In addition it is filling in modes that shouldn't
> > be present after the validation.
> 
> Note that I'm not directly filling pl->supported here.
> 
> [...]
> 
>  	c = phy_caps_lookup(pl->link_config.speed, pl->link_config.duplex,
>  			    pl->supported, true);
> -	if (c)
> -		linkmode_and(match, pl->supported, c->linkmodes);
> +	if (c) {
> +		/* Compatbility with the legacy behaviour : Report one single
> +		 * BaseT mode for fixed-link speeds under or at 10G, or all
> +		 * linkmodes at the speed/duplex for speeds over 10G
> +		 */
> +		if (linkmode_intersects(match, c->linkmodes))
> +			linkmode_and(match, match, c->linkmodes);
> +		else
> +			linkmode_copy(match, c->linkmodes);
> +	}
> 
> [...]
> 
> 	if (c) {
> 		linkmode_or(pl->supported, pl->supported, match);
> 		linkmode_or(pl->link_config.lp_advertising,
> 			    pl->link_config.lp_advertising, match);
> 	}
> 
> For speeds above 10G, you will get all the modes for the requested
> speed, so it should solve the issue in the next steps of your code
> where you need matching linkmodes for your settings ? Did you give it a
> try ?
> 
> You may end up with too many linkmodes, but for fixed-link we don't
> really expect these modes to precisely represent any real linkmodes

Here is more the quick-n-dirty approach that I think does what you were
trying to do without breaking stuff:

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 16a1f31f0091..380e51c5bdaa 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -713,17 +713,24 @@ static int phylink_parse_fixedlink(struct phylink *pl,
                phylink_warn(pl, "fixed link specifies half duplex for %dMbps link?\n",
                             pl->link_config.speed);
 
-       linkmode_zero(pl->supported);
-       phylink_fill_fixedlink_supported(pl->supported);
-
+       linkmode_fill(pl->supported);
        linkmode_copy(pl->link_config.advertising, pl->supported);
        phylink_validate(pl, pl->supported, &pl->link_config);
 
        c = phy_caps_lookup(pl->link_config.speed, pl->link_config.duplex,
                            pl->supported, true);
-       if (c)
+       if (c) {
                linkmode_and(match, pl->supported, c->linkmodes);
 
+               /* Compatbility with the legacy behaviour:
+                * Report one single BaseT mode.
+                */
+               phylink_fill_fixedlink_supported(mask);
+               if (linkmode_intersects(match, mask))
+                       linkmode_and(match, match, mask);
+               linkmode_zero(mask);
+       }
+
        linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT, mask);
        linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, mask);
        linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, mask);

Basically we still need the value to be screened by the pl->supported.
The one change is that we have to run the extra screening on the
intersect instead of skipping the screening, or doing it before we even
start providing bits.

With this approach we will even allow people to use non twisted pair
setups regardless of speed as long as they don't provide any twisted
pair modes in the standard set.

I will try to get this tested today and if it works out I will submit
it for net. I just need to test this and an SFP ksettings_set issue I
found when we aren't using autoneg.

Re: [PATCH net-next v5 09/13] net: phylink: Use phy_caps_lookup for fixed-link configuration

[Maxime Chevallier]

On Tue, 01 Apr 2025 08:28:29 -0700
Alexander H Duyck <alexander.duyck@gmail.com> wrote:

> On Mon, 2025-03-31 at 09:14 +0200, Maxime Chevallier wrote:
> > On Fri, 28 Mar 2025 14:03:54 -0700
> > Alexander Duyck <alexander.duyck@gmail.com> wrote:
> >   
> > > On Fri, Mar 28, 2025 at 1:06 AM Maxime Chevallier
> > > <maxime.chevallier@bootlin.com> wrote:  
> > > > 
> > > > On Thu, 27 Mar 2025 18:16:00 -0700
> > > > Alexander H Duyck <alexander.duyck@gmail.com> wrote:
> > > >    
> > > > > On Fri, 2025-03-07 at 18:36 +0100, Maxime Chevallier wrote:    
> > > > > > When phylink creates a fixed-link configuration, it finds a matching
> > > > > > linkmode to set as the advertised, lp_advertising and supported modes
> > > > > > based on the speed and duplex of the fixed link.
> > > > > > 
> > > > > > Use the newly introduced phy_caps_lookup to get these modes instead of
> > > > > > phy_lookup_settings(). This has the side effect that the matched
> > > > > > settings and configured linkmodes may now contain several linkmodes (the
> > > > > > intersection of supported linkmodes from the phylink settings and the
> > > > > > linkmodes that match speed/duplex) instead of the one from
> > > > > > phy_lookup_settings().
> > > > > > 
> > > > > > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > > > > > ---
> > > > > >  drivers/net/phy/phylink.c | 44 +++++++++++++++++++++++++++------------
> > > > > >  1 file changed, 31 insertions(+), 13 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > > > > > index cf9f019382ad..8e2b7d647a92 100644
> > > > > > --- a/drivers/net/phy/phylink.c
> > > > > > +++ b/drivers/net/phy/phylink.c
> > > > > > @@ -802,12 +802,26 @@ static int phylink_validate(struct phylink *pl, unsigned long *supported,
> > > > > >     return phylink_validate_mac_and_pcs(pl, supported, state);
> > > > > >  }
> > > > > > 
> > > > > > +static void phylink_fill_fixedlink_supported(unsigned long *supported)
> > > > > > +{
> > > > > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT, supported);
> > > > > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT, supported);
> > > > > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT, supported);
> > > > > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT, supported);
> > > > > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT, supported);
> > > > > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, supported);
> > > > > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, supported);
> > > > > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, supported);
> > > > > > +   linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, supported);
> > > > > > +}
> > > > > > +    
> > > > > 
> > > > > Any chance we can go with a different route here than just locking
> > > > > fixed mode to being only for BaseT configurations?
> > > > > 
> > > > > I am currently working on getting the fbnic driver up and running and I
> > > > > am using fixed-link mode as a crutch until I can finish up enabling
> > > > > QSFP module support for phylink and this just knocked out the supported
> > > > > link modes as I was using 25CR, 50CR, 50CR2, and 100CR2.
> > > > > 
> > > > > Seems like this should really be something handled by some sort of
> > > > > validation function rather than just forcing all devices using fixed
> > > > > link to assume that they are BaseT. I know in our direct attached
> > > > > copper case we aren't running autoneg so that plan was to use fixed
> > > > > link until we can add more flexibility by getting QSFP support going.    
> > > > 
> > > > These baseT mode were for compatibility with the previous way to deal
> > > > with fixed links, but we can extend what's being report above 10G with
> > > > other modes. Indeed the above code unnecessarily restricts the
> > > > linkmodes. Can you tell me if the following patch works for you ?
> > > > 
> > > > Maxime
> > > > 
> > > > -----------
> > > > 
> > > > From 595888afb23f209f2b1002d0b0406380195d53c1 Mon Sep 17 00:00:00 2001
> > > > From: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > > > Date: Fri, 28 Mar 2025 08:53:00 +0100
> > > > Subject: [PATCH] net: phylink: Allow fixed-link registration above 10G
> > > > 
> > > > The blamed commit introduced a helper to keep the linkmodes reported by
> > > > fixed links identical to what they were before phy_caps. This means
> > > > filtering linkmodes only to report BaseT modes.
> > > > 
> > > > Doing so, it also filtered out any linkmode above 10G. Reinstate the
> > > > reporting of linkmodes above 10G, where we report any linkmodes that
> > > > exist at these speeds.
> > > > 
> > > > Reported-by: Alexander H Duyck <alexander.duyck@gmail.com>
> > > > Closes: https://lore.kernel.org/netdev/8d3a9c9bb76b1c6bc27d2bd01f4831b2cac83f7f.camel@gmail.com/
> > > > Fixes: de7d3f87be3c ("net: phylink: Use phy_caps_lookup for fixed-link configuration")
> > > > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > > > ---
> > > >  drivers/net/phy/phylink.c | 17 +++++++++++++----
> > > >  1 file changed, 13 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > > > index 69ca765485db..de90fed9c207 100644
> > > > --- a/drivers/net/phy/phylink.c
> > > > +++ b/drivers/net/phy/phylink.c
> > > > @@ -715,16 +715,25 @@ static int phylink_parse_fixedlink(struct phylink *pl,
> > > >                 phylink_warn(pl, "fixed link specifies half duplex for %dMbps link?\n",
> > > >                              pl->link_config.speed);
> > > > 
> > > > -       linkmode_zero(pl->supported);
> > > > -       phylink_fill_fixedlink_supported(pl->supported);
> > > > +       linkmode_fill(pl->supported);
> > > > 
> > > >         linkmode_copy(pl->link_config.advertising, pl->supported);
> > > >         phylink_validate(pl, pl->supported, &pl->link_config);
> > > > 
> > > > +       phylink_fill_fixedlink_supported(match);
> > > > +    
> > > 
> > > I worry that this might put you back into the same problem again with
> > > the older drivers. In addition it is filling in modes that shouldn't
> > > be present after the validation.  
> > 
> > Note that I'm not directly filling pl->supported here.
> > 
> > [...]
> > 
> >  	c = phy_caps_lookup(pl->link_config.speed, pl->link_config.duplex,
> >  			    pl->supported, true);
> > -	if (c)
> > -		linkmode_and(match, pl->supported, c->linkmodes);
> > +	if (c) {
> > +		/* Compatbility with the legacy behaviour : Report one single
> > +		 * BaseT mode for fixed-link speeds under or at 10G, or all
> > +		 * linkmodes at the speed/duplex for speeds over 10G
> > +		 */
> > +		if (linkmode_intersects(match, c->linkmodes))
> > +			linkmode_and(match, match, c->linkmodes);
> > +		else
> > +			linkmode_copy(match, c->linkmodes);
> > +	}
> > 
> > [...]
> > 
> > 	if (c) {
> > 		linkmode_or(pl->supported, pl->supported, match);
> > 		linkmode_or(pl->link_config.lp_advertising,
> > 			    pl->link_config.lp_advertising, match);
> > 	}
> > 
> > For speeds above 10G, you will get all the modes for the requested
> > speed, so it should solve the issue in the next steps of your code
> > where you need matching linkmodes for your settings ? Did you give it a
> > try ?
> > 
> > You may end up with too many linkmodes, but for fixed-link we don't
> > really expect these modes to precisely represent any real linkmodes  
> 
> Here is more the quick-n-dirty approach that I think does what you were
> trying to do without breaking stuff:
> 
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 16a1f31f0091..380e51c5bdaa 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -713,17 +713,24 @@ static int phylink_parse_fixedlink(struct phylink *pl,
>                 phylink_warn(pl, "fixed link specifies half duplex for %dMbps link?\n",
>                              pl->link_config.speed);
>  
> -       linkmode_zero(pl->supported);
> -       phylink_fill_fixedlink_supported(pl->supported);
> -
> +       linkmode_fill(pl->supported);
>         linkmode_copy(pl->link_config.advertising, pl->supported);
>         phylink_validate(pl, pl->supported, &pl->link_config);
>  
>         c = phy_caps_lookup(pl->link_config.speed, pl->link_config.duplex,
>                             pl->supported, true);
> -       if (c)
> +       if (c) {
>                 linkmode_and(match, pl->supported, c->linkmodes);
>  
> +               /* Compatbility with the legacy behaviour:
> +                * Report one single BaseT mode.
> +                */
> +               phylink_fill_fixedlink_supported(mask);
> +               if (linkmode_intersects(match, mask))
> +                       linkmode_and(match, match, mask);
> +               linkmode_zero(mask);
> +       }
> +
>         linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT, mask);
>         linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, mask);
>         linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, mask);
> 
> Basically we still need the value to be screened by the pl->supported.
> The one change is that we have to run the extra screening on the
> intersect instead of skipping the screening, or doing it before we even
> start providing bits.
> 
> With this approach we will even allow people to use non twisted pair
> setups regardless of speed as long as they don't provide any twisted
> pair modes in the standard set.
> 
> I will try to get this tested today and if it works out I will submit
> it for net. I just need to test this and an SFP ksettings_set issue I
> found when we aren't using autoneg.

That looks correct and indeed better than my patch, thanks for that :)

Feel free to send it indeed, I'll give it a try on the setups I have
here as well

Maxime

Re: [PATCH net-next v5 09/13] net: phylink: Use phy_caps_lookup for fixed-link configuration

[Russell King (Oracle)]

On Tue, Apr 01, 2025 at 08:28:29AM -0700, Alexander H Duyck wrote:
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 16a1f31f0091..380e51c5bdaa 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -713,17 +713,24 @@ static int phylink_parse_fixedlink(struct phylink *pl,
>                 phylink_warn(pl, "fixed link specifies half duplex for %dMbps link?\n",
>                              pl->link_config.speed);
>  
> -       linkmode_zero(pl->supported);
> -       phylink_fill_fixedlink_supported(pl->supported);
> -
> +       linkmode_fill(pl->supported);
>         linkmode_copy(pl->link_config.advertising, pl->supported);
>         phylink_validate(pl, pl->supported, &pl->link_config);
>  
>         c = phy_caps_lookup(pl->link_config.speed, pl->link_config.duplex,
>                             pl->supported, true);
> -       if (c)
> +       if (c) {
>                 linkmode_and(match, pl->supported, c->linkmodes);
>  
> +               /* Compatbility with the legacy behaviour:
> +                * Report one single BaseT mode.
> +                */
> +               phylink_fill_fixedlink_supported(mask);
> +               if (linkmode_intersects(match, mask))
> +                       linkmode_and(match, match, mask);
> +               linkmode_zero(mask);
> +       }
> +
>         linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT, mask);
>         linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, mask);
>         linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, mask);
> 
> Basically we still need the value to be screened by the pl->supported.
> The one change is that we have to run the extra screening on the
> intersect instead of skipping the screening, or doing it before we even
> start providing bits.
> 
> With this approach we will even allow people to use non twisted pair
> setups regardless of speed as long as they don't provide any twisted
> pair modes in the standard set.
> 
> I will try to get this tested today and if it works out I will submit
> it for net. I just need to test this and an SFP ksettings_set issue I
> found when we aren't using autoneg.

This code used to be so simple... and that makes me wonder whether
Maxime's work is really the best approach. It seems that the old way
was better precisely because it was more simple.

-- 
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 v5 09/13] net: phylink: Use phy_caps_lookup for fixed-link configuration

[Maxime Chevallier]

> > Basically we still need the value to be screened by the pl->supported.
> > The one change is that we have to run the extra screening on the
> > intersect instead of skipping the screening, or doing it before we even
> > start providing bits.
> > 
> > With this approach we will even allow people to use non twisted pair
> > setups regardless of speed as long as they don't provide any twisted
> > pair modes in the standard set.
> > 
> > I will try to get this tested today and if it works out I will submit
> > it for net. I just need to test this and an SFP ksettings_set issue I
> > found when we aren't using autoneg.  
> 
> This code used to be so simple... and that makes me wonder whether
> Maxime's work is really the best approach. It seems that the old way
> was better precisely because it was more simple.

Sorry to hear you say that. Fixed-link was the main pain point with
this work, I've stressed it out. I agree that for fixed-link, it
ends-up not looking too good, hopefully the rest of the series
compensate for that.

Maxime

Re: [PATCH net-next v5 07/13] net: phy: phy_caps: Allow looking-up link caps based on speed and duplex

[Jijie Shao]

on 2025/3/8 1:36, Maxime Chevallier wrote:
> As the link_caps array is efficient for <speed,duplex> lookups,
> implement a function for speed/duplex lookups that matches a given
> mask. This replicates to some extent the phy_lookup_settings()
> behaviour, matching full link_capabilities instead of a single linkmode.
>
> phy.c's phy_santize_settings() and phylink's
> phylink_ethtool_ksettings_set() performs such lookup using the
> phy_settings table, but are only interested in the actual speed/duplex
> that were matched, rathet than the individual linkmode.
>
> Similar to phy_lookup_settings(), the newly introduced phy_caps_lookup()
> will run through the link_caps[] array by descending speed/duplex order.
>
> If the link_capabilities for a given <speed/duplex> tuple intersects the
> passed linkmodes, we consider that a match.
>
> Similar to phy_lookup_settings(), we also allow passing an 'exact'
> boolean, allowing non-exact match. Here, we MUST always match the
> linkmodes mask, but we allow matching on lower speed settings.
>
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> ---
>   drivers/net/phy/phy-caps.h |  4 ++++
>   drivers/net/phy/phy.c      | 32 ++++++--------------------
>   drivers/net/phy/phy_caps.c | 47 ++++++++++++++++++++++++++++++++++++++
>   drivers/net/phy/phylink.c  | 17 +++++++-------
>   4 files changed, 67 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/net/phy/phy-caps.h b/drivers/net/phy/phy-caps.h
> index 8833798f141f..aef4b54a8429 100644
> --- a/drivers/net/phy/phy-caps.h
> +++ b/drivers/net/phy/phy-caps.h
> @@ -51,4 +51,8 @@ phy_caps_lookup_by_linkmode(const unsigned long *linkmodes);
>   const struct link_capabilities *
>   phy_caps_lookup_by_linkmode_rev(const unsigned long *linkmodes, bool fdx_only);
>   
> +const struct link_capabilities *
> +phy_caps_lookup(int speed, unsigned int duplex, const unsigned long *supported,
> +		bool exact);
> +
>   #endif /* __PHY_CAPS_H */
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 8df37d221fba..562acde89224 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -213,25 +213,6 @@ int phy_aneg_done(struct phy_device *phydev)
>   }
>   EXPORT_SYMBOL(phy_aneg_done);
>   
> -/**
> - * phy_find_valid - find a PHY setting that matches the requested parameters
> - * @speed: desired speed
> - * @duplex: desired duplex
> - * @supported: mask of supported link modes
> - *
> - * Locate a supported phy setting that is, in priority order:
> - * - an exact match for the specified speed and duplex mode
> - * - a match for the specified speed, or slower speed
> - * - the slowest supported speed
> - * Returns the matched phy_setting entry, or %NULL if no supported phy
> - * settings were found.
> - */
> -static const struct phy_setting *
> -phy_find_valid(int speed, int duplex, unsigned long *supported)
> -{
> -	return phy_lookup_setting(speed, duplex, supported, false);
> -}
> -
>   /**
>    * phy_supported_speeds - return all speeds currently supported by a phy device
>    * @phy: The phy device to return supported speeds of.
> @@ -274,13 +255,14 @@ EXPORT_SYMBOL(phy_check_valid);
>    */
>   static void phy_sanitize_settings(struct phy_device *phydev)
>   {
> -	const struct phy_setting *setting;
> +	const struct link_capabilities *c;
> +
> +	c = phy_caps_lookup(phydev->speed, phydev->duplex, phydev->supported,
> +			    false);
>   
> -	setting = phy_find_valid(phydev->speed, phydev->duplex,
> -				 phydev->supported);
> -	if (setting) {
> -		phydev->speed = setting->speed;
> -		phydev->duplex = setting->duplex;
> +	if (c) {
> +		phydev->speed = c->speed;
> +		phydev->duplex = c->duplex;
>   	} else {
>   		/* We failed to find anything (no supported speeds?) */
>   		phydev->speed = SPEED_UNKNOWN;
> diff --git a/drivers/net/phy/phy_caps.c b/drivers/net/phy/phy_caps.c
> index c39f38c12ef2..0366feee2912 100644
> --- a/drivers/net/phy/phy_caps.c
> +++ b/drivers/net/phy/phy_caps.c
> @@ -170,6 +170,53 @@ phy_caps_lookup_by_linkmode_rev(const unsigned long *linkmodes, bool fdx_only)
>   	return NULL;
>   }
>   
> +/**
> + * phy_caps_lookup() - Lookup capabilities by speed/duplex that matches a mask
> + * @speed: Speed to match
> + * @duplex: Duplex to match
> + * @supported: Mask of linkmodes to match
> + * @exact: Perform an exact match or not.
> + *
> + * Lookup a link_capabilities entry that intersect the supported linkmodes mask,
> + * and that matches the passed speed and duplex.
> + *
> + * When @exact is set, an exact match is performed on speed and duplex, meaning
> + * that if the linkmodes for the given speed and duplex intersect the supported
> + * mask, this capability is returned, otherwise we don't have a match and return
> + * NULL.
> + *
> + * When @exact is not set, we return either an exact match, or matching capabilities
> + * at lower speed, or the lowest matching speed, or NULL.
> + *
> + * Returns: a matched link_capabilities according to the above process, NULL
> + *	    otherwise.
> + */
> +const struct link_capabilities *
> +phy_caps_lookup(int speed, unsigned int duplex, const unsigned long *supported,
> +		bool exact)
> +{
> +	const struct link_capabilities *lcap, *last = NULL;
> +
> +	for_each_link_caps_desc_speed(lcap) {
> +		if (linkmode_intersects(lcap->linkmodes, supported)) {
> +			last = lcap;
> +			/* exact match on speed and duplex*/
> +			if (lcap->speed == speed && lcap->duplex == duplex) {
> +				return lcap;
> +			} else if (!exact) {
> +				if (lcap->speed <= speed)
> +					return lcap;
> +			}
> +		}
> +	}
> +
> +	if (!exact)
> +		return last;
> +
> +	return NULL;
> +}
> +EXPORT_SYMBOL_GPL(phy_caps_lookup);
> +
>   /**
>    * phy_caps_linkmode_max_speed() - Clamp a linkmodes set to a max speed
>    * @max_speed: Speed limit for the linkmode set
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index a3f64b6d2d34..cf9f019382ad 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -20,6 +20,7 @@
>   #include <linux/timer.h>
>   #include <linux/workqueue.h>
>   
> +#include "phy-caps.h"
>   #include "sfp.h"
>   #include "swphy.h"
>   
> @@ -2852,8 +2853,8 @@ int phylink_ethtool_ksettings_set(struct phylink *pl,
>   				  const struct ethtool_link_ksettings *kset)
>   {
>   	__ETHTOOL_DECLARE_LINK_MODE_MASK(support);
> +	const struct link_capabilities *c;
>   	struct phylink_link_state config;
> -	const struct phy_setting *s;
>   
>   	ASSERT_RTNL();
>   
> @@ -2896,23 +2897,23 @@ int phylink_ethtool_ksettings_set(struct phylink *pl,
>   		/* Autonegotiation disabled, select a suitable speed and
>   		 * duplex.
>   		 */
> -		s = phy_lookup_setting(kset->base.speed, kset->base.duplex,
> -				       pl->supported, false);
> -		if (!s)
> +		c = phy_caps_lookup(kset->base.speed, kset->base.duplex,
> +				    pl->supported, false);
> +		if (!c)
>   			return -EINVAL;



Hi Maxime,  fc81e257d19f ("net: phy: phy_caps: Allow looking-up link caps based on speed and duplex") might have different behavior than the modification.
My case is set 10M Half with disable autoneg both sides and I expect it is
link in 10M Half. But now, it is link in 10M Full，which is not what I
expect.

I used followed command and trace how phy worked.
	ethtool -s eth1 autoneg off speed 10 duplex half
The log is showed as followed:
ethtool-13127	[067]	6164.771853: phy_ethtool_ksettings set: (phy_ethtool ksettings set+0x0/0x200) duplex=0 speed=10
kworker/u322:2-11096	[070]	6164.771853:	_phy_start_aneq: ( _phy_start_aneg+0x0/0xb8) duplex=0 speed=10
kworker/u322:2-11096	[070]	6164.771854:	phy_caps_lookup: (phy_caps_lookup+0x0/0xf0) duplex=0 speed=10
kworker/u322:2-11096	[070]	6164.771855:	phy_config_aneg: (phy_config_aneg+0x0/0x70) duplex=1 speed=10
kworker/u322:2-11096	[070]	6164.771856:	genphy_config_aneg:	(__genphy_config_aneg+0X0/0X270) duplex=1 speed=10

I also try to fixed it and it works. Do you have any idea about it.

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 0e762fc3a529..2986c41c42a8 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -258,7 +258,7 @@ static void phy_sanitize_settings(struct phy_device *phydev)
         const struct link_capabilities *c;

         c = phy_caps_lookup(phydev->speed, phydev->duplex, phydev->supported,
-                           false);
+                           true);

         if (c) {
                 phydev->speed = c->speed;



>   
>   		/* If we have a fixed link, refuse to change link parameters.
>   		 * If the link parameters match, accept them but do nothing.
>   		 */
>   		if (pl->req_link_an_mode == MLO_AN_FIXED) {
> -			if (s->speed != pl->link_config.speed ||
> -			    s->duplex != pl->link_config.duplex)
> +			if (c->speed != pl->link_config.speed ||
> +			    c->duplex != pl->link_config.duplex)
>   				return -EINVAL;
>   			return 0;
>   		}
>   
> -		config.speed = s->speed;
> -		config.duplex = s->duplex;
> +		config.speed = c->speed;
> +		config.duplex = c->duplex;
>   		break;
>   
>   	case AUTONEG_ENABLE:

Re: [PATCH net-next v5 07/13] net: phy: phy_caps: Allow looking-up link caps based on speed and duplex

[Russell King (Oracle)]

On Thu, May 29, 2025 at 05:36:11PM +0800, Jijie Shao wrote:
> Hi Maxime,  fc81e257d19f ("net: phy: phy_caps: Allow looking-up link caps based on speed and duplex") might have different behavior than the modification.
> My case is set 10M Half with disable autoneg both sides and I expect it is
> link in 10M Half. But now, it is link in 10M Full，which is not what I
> expect.
> 
> I used followed command and trace how phy worked.
> 	ethtool -s eth1 autoneg off speed 10 duplex half
> The log is showed as followed:
> ethtool-13127	[067]	6164.771853: phy_ethtool_ksettings set: (phy_ethtool ksettings set+0x0/0x200) duplex=0 speed=10
> kworker/u322:2-11096	[070]	6164.771853:	_phy_start_aneq: ( _phy_start_aneg+0x0/0xb8) duplex=0 speed=10
> kworker/u322:2-11096	[070]	6164.771854:	phy_caps_lookup: (phy_caps_lookup+0x0/0xf0) duplex=0 speed=10
> kworker/u322:2-11096	[070]	6164.771855:	phy_config_aneg: (phy_config_aneg+0x0/0x70) duplex=1 speed=10
> kworker/u322:2-11096	[070]	6164.771856:	genphy_config_aneg:	(__genphy_config_aneg+0X0/0X270) duplex=1 speed=10
> 
> I also try to fixed it and it works. Do you have any idea about it.
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 0e762fc3a529..2986c41c42a8 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -258,7 +258,7 @@ static void phy_sanitize_settings(struct phy_device *phydev)
>         const struct link_capabilities *c;
> 
>         c = phy_caps_lookup(phydev->speed, phydev->duplex, phydev->supported,
> -                           false);
> +                           true);

This isn't the correct fix, as:

+ * When @exact is not set, we return either an exact match, or matching capabilities
+ * at lower speed, or the lowest matching speed, or NULL.

So it isn't returning the exact match but apparently ignoring the
duplex.

-- 
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 v5 07/13] net: phy: phy_caps: Allow looking-up link caps based on speed and duplex

[Maxime Chevallier]

On Thu, 29 May 2025 17:36:11 +0800
Jijie Shao <shaojijie@huawei.com> wrote:

> on 2025/3/8 1:36, Maxime Chevallier wrote:
> > As the link_caps array is efficient for <speed,duplex> lookups,
> > implement a function for speed/duplex lookups that matches a given
> > mask. This replicates to some extent the phy_lookup_settings()
> > behaviour, matching full link_capabilities instead of a single linkmode.
> >
> > phy.c's phy_santize_settings() and phylink's
> > phylink_ethtool_ksettings_set() performs such lookup using the
> > phy_settings table, but are only interested in the actual speed/duplex
> > that were matched, rathet than the individual linkmode.
> >
> > Similar to phy_lookup_settings(), the newly introduced phy_caps_lookup()
> > will run through the link_caps[] array by descending speed/duplex order.
> >
> > If the link_capabilities for a given <speed/duplex> tuple intersects the
> > passed linkmodes, we consider that a match.
> >
> > Similar to phy_lookup_settings(), we also allow passing an 'exact'
> > boolean, allowing non-exact match. Here, we MUST always match the
> > linkmodes mask, but we allow matching on lower speed settings.
> >
> > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > ---
> >   drivers/net/phy/phy-caps.h |  4 ++++
> >   drivers/net/phy/phy.c      | 32 ++++++--------------------
> >   drivers/net/phy/phy_caps.c | 47 ++++++++++++++++++++++++++++++++++++++
> >   drivers/net/phy/phylink.c  | 17 +++++++-------
> >   4 files changed, 67 insertions(+), 33 deletions(-)
> >
> > diff --git a/drivers/net/phy/phy-caps.h b/drivers/net/phy/phy-caps.h
> > index 8833798f141f..aef4b54a8429 100644
> > --- a/drivers/net/phy/phy-caps.h
> > +++ b/drivers/net/phy/phy-caps.h
> > @@ -51,4 +51,8 @@ phy_caps_lookup_by_linkmode(const unsigned long *linkmodes);
> >   const struct link_capabilities *
> >   phy_caps_lookup_by_linkmode_rev(const unsigned long *linkmodes, bool fdx_only);
> >   
> > +const struct link_capabilities *
> > +phy_caps_lookup(int speed, unsigned int duplex, const unsigned long *supported,
> > +		bool exact);
> > +
> >   #endif /* __PHY_CAPS_H */
> > diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> > index 8df37d221fba..562acde89224 100644
> > --- a/drivers/net/phy/phy.c
> > +++ b/drivers/net/phy/phy.c
> > @@ -213,25 +213,6 @@ int phy_aneg_done(struct phy_device *phydev)
> >   }
> >   EXPORT_SYMBOL(phy_aneg_done);
> >   
> > -/**
> > - * phy_find_valid - find a PHY setting that matches the requested parameters
> > - * @speed: desired speed
> > - * @duplex: desired duplex
> > - * @supported: mask of supported link modes
> > - *
> > - * Locate a supported phy setting that is, in priority order:
> > - * - an exact match for the specified speed and duplex mode
> > - * - a match for the specified speed, or slower speed
> > - * - the slowest supported speed
> > - * Returns the matched phy_setting entry, or %NULL if no supported phy
> > - * settings were found.
> > - */
> > -static const struct phy_setting *
> > -phy_find_valid(int speed, int duplex, unsigned long *supported)
> > -{
> > -	return phy_lookup_setting(speed, duplex, supported, false);
> > -}
> > -
> >   /**
> >    * phy_supported_speeds - return all speeds currently supported by a phy device
> >    * @phy: The phy device to return supported speeds of.
> > @@ -274,13 +255,14 @@ EXPORT_SYMBOL(phy_check_valid);
> >    */
> >   static void phy_sanitize_settings(struct phy_device *phydev)
> >   {
> > -	const struct phy_setting *setting;
> > +	const struct link_capabilities *c;
> > +
> > +	c = phy_caps_lookup(phydev->speed, phydev->duplex, phydev->supported,
> > +			    false);
> >   
> > -	setting = phy_find_valid(phydev->speed, phydev->duplex,
> > -				 phydev->supported);
> > -	if (setting) {
> > -		phydev->speed = setting->speed;
> > -		phydev->duplex = setting->duplex;
> > +	if (c) {
> > +		phydev->speed = c->speed;
> > +		phydev->duplex = c->duplex;
> >   	} else {
> >   		/* We failed to find anything (no supported speeds?) */
> >   		phydev->speed = SPEED_UNKNOWN;
> > diff --git a/drivers/net/phy/phy_caps.c b/drivers/net/phy/phy_caps.c
> > index c39f38c12ef2..0366feee2912 100644
> > --- a/drivers/net/phy/phy_caps.c
> > +++ b/drivers/net/phy/phy_caps.c
> > @@ -170,6 +170,53 @@ phy_caps_lookup_by_linkmode_rev(const unsigned long *linkmodes, bool fdx_only)
> >   	return NULL;
> >   }
> >   
> > +/**
> > + * phy_caps_lookup() - Lookup capabilities by speed/duplex that matches a mask
> > + * @speed: Speed to match
> > + * @duplex: Duplex to match
> > + * @supported: Mask of linkmodes to match
> > + * @exact: Perform an exact match or not.
> > + *
> > + * Lookup a link_capabilities entry that intersect the supported linkmodes mask,
> > + * and that matches the passed speed and duplex.
> > + *
> > + * When @exact is set, an exact match is performed on speed and duplex, meaning
> > + * that if the linkmodes for the given speed and duplex intersect the supported
> > + * mask, this capability is returned, otherwise we don't have a match and return
> > + * NULL.
> > + *
> > + * When @exact is not set, we return either an exact match, or matching capabilities
> > + * at lower speed, or the lowest matching speed, or NULL.
> > + *
> > + * Returns: a matched link_capabilities according to the above process, NULL
> > + *	    otherwise.
> > + */
> > +const struct link_capabilities *
> > +phy_caps_lookup(int speed, unsigned int duplex, const unsigned long *supported,
> > +		bool exact)
> > +{
> > +	const struct link_capabilities *lcap, *last = NULL;
> > +
> > +	for_each_link_caps_desc_speed(lcap) {
> > +		if (linkmode_intersects(lcap->linkmodes, supported)) {
> > +			last = lcap;
> > +			/* exact match on speed and duplex*/
> > +			if (lcap->speed == speed && lcap->duplex == duplex) {
> > +				return lcap;
> > +			} else if (!exact) {
> > +				if (lcap->speed <= speed)
> > +					return lcap;
> > +			}
> > +		}
> > +	}
> > +
> > +	if (!exact)
> > +		return last;
> > +
> > +	return NULL;
> > +}
> > +EXPORT_SYMBOL_GPL(phy_caps_lookup);
> > +
> >   /**
> >    * phy_caps_linkmode_max_speed() - Clamp a linkmodes set to a max speed
> >    * @max_speed: Speed limit for the linkmode set
> > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > index a3f64b6d2d34..cf9f019382ad 100644
> > --- a/drivers/net/phy/phylink.c
> > +++ b/drivers/net/phy/phylink.c
> > @@ -20,6 +20,7 @@
> >   #include <linux/timer.h>
> >   #include <linux/workqueue.h>
> >   
> > +#include "phy-caps.h"
> >   #include "sfp.h"
> >   #include "swphy.h"
> >   
> > @@ -2852,8 +2853,8 @@ int phylink_ethtool_ksettings_set(struct phylink *pl,
> >   				  const struct ethtool_link_ksettings *kset)
> >   {
> >   	__ETHTOOL_DECLARE_LINK_MODE_MASK(support);
> > +	const struct link_capabilities *c;
> >   	struct phylink_link_state config;
> > -	const struct phy_setting *s;
> >   
> >   	ASSERT_RTNL();
> >   
> > @@ -2896,23 +2897,23 @@ int phylink_ethtool_ksettings_set(struct phylink *pl,
> >   		/* Autonegotiation disabled, select a suitable speed and
> >   		 * duplex.
> >   		 */
> > -		s = phy_lookup_setting(kset->base.speed, kset->base.duplex,
> > -				       pl->supported, false);
> > -		if (!s)
> > +		c = phy_caps_lookup(kset->base.speed, kset->base.duplex,
> > +				    pl->supported, false);
> > +		if (!c)
> >   			return -EINVAL;  
> 
> 
> 
> Hi Maxime,  fc81e257d19f ("net: phy: phy_caps: Allow looking-up link caps based on speed and duplex") might have different behavior than the modification.
> My case is set 10M Half with disable autoneg both sides and I expect it is
> link in 10M Half. But now, it is link in 10M Full，which is not what I
> expect.
> 
> I used followed command and trace how phy worked.
> 	ethtool -s eth1 autoneg off speed 10 duplex half
> The log is showed as followed:
> ethtool-13127	[067]	6164.771853: phy_ethtool_ksettings set: (phy_ethtool ksettings set+0x0/0x200) duplex=0 speed=10
> kworker/u322:2-11096	[070]	6164.771853:	_phy_start_aneq: ( _phy_start_aneg+0x0/0xb8) duplex=0 speed=10
> kworker/u322:2-11096	[070]	6164.771854:	phy_caps_lookup: (phy_caps_lookup+0x0/0xf0) duplex=0 speed=10
> kworker/u322:2-11096	[070]	6164.771855:	phy_config_aneg: (phy_config_aneg+0x0/0x70) duplex=1 speed=10
> kworker/u322:2-11096	[070]	6164.771856:	genphy_config_aneg:	(__genphy_config_aneg+0X0/0X270) duplex=1 speed=10
> 
> I also try to fixed it and it works. Do you have any idea about it.

The !exact match logic in the rework is wrong indeed, sorry...

For non-exact matches we return a non-exact match too early without
giving the chance for a potentially exact half-duplex match...

As for the fix, can you try this out :

diff --git a/drivers/net/phy/phy_caps.c b/drivers/net/phy/phy_caps.c
index 703321689726..d80f6a37edf1 100644
--- a/drivers/net/phy/phy_caps.c
+++ b/drivers/net/phy/phy_caps.c
@@ -195,7 +195,7 @@ const struct link_capabilities *
 phy_caps_lookup(int speed, unsigned int duplex, const unsigned long *supported,
 		bool exact)
 {
-	const struct link_capabilities *lcap, *last = NULL;
+	const struct link_capabilities *lcap, *match = NULL, *last = NULL;
 
 	for_each_link_caps_desc_speed(lcap) {
 		if (linkmode_intersects(lcap->linkmodes, supported)) {
@@ -204,16 +204,19 @@ phy_caps_lookup(int speed, unsigned int duplex, const unsigned long *supported,
 			if (lcap->speed == speed && lcap->duplex == duplex) {
 				return lcap;
 			} else if (!exact) {
-				if (lcap->speed <= speed)
-					return lcap;
+				if (!match && lcap->speed <= speed)
+					match = lcap;
+
+				if (lcap->speed < speed)
+					break;
 			}
 		}
 	}
 
-	if (!exact)
-		return last;
+	if (!match && !exact)
+		match = last;
 
-	return NULL;
+	return match;
 }
 EXPORT_SYMBOL_GPL(phy_caps_lookup);

Re: [PATCH net-next v5 07/13] net: phy: phy_caps: Allow looking-up link caps based on speed and duplex

[Maxime Chevallier]

Hello again,

On Thu, 29 May 2025 17:36:11 +0800
Jijie Shao <shaojijie@huawei.com> wrote:

>
> 
> Hi Maxime,  fc81e257d19f ("net: phy: phy_caps: Allow looking-up link caps based on speed and duplex") might have different behavior than the modification.
> My case is set 10M Half with disable autoneg both sides and I expect it is
> link in 10M Half. But now, it is link in 10M Full，which is not what I
> expect.
> 
> I used followed command and trace how phy worked.
> 	ethtool -s eth1 autoneg off speed 10 duplex half
> The log is showed as followed:
> ethtool-13127	[067]	6164.771853: phy_ethtool_ksettings set: (phy_ethtool ksettings set+0x0/0x200) duplex=0 speed=10
> kworker/u322:2-11096	[070]	6164.771853:	_phy_start_aneq: ( _phy_start_aneg+0x0/0xb8) duplex=0 speed=10
> kworker/u322:2-11096	[070]	6164.771854:	phy_caps_lookup: (phy_caps_lookup+0x0/0xf0) duplex=0 speed=10
> kworker/u322:2-11096	[070]	6164.771855:	phy_config_aneg: (phy_config_aneg+0x0/0x70) duplex=1 speed=10
> kworker/u322:2-11096	[070]	6164.771856:	genphy_config_aneg:	(__genphy_config_aneg+0X0/0X270) duplex=1 speed=10

I managed to get a bit of time to look into this, and indeed I was able
to reproduce the issue locally.

I'll send a patch ASAP, based on the the prototype patch I gave in the
previous reply.

Thanks for reporting,

Maxime