Skip to contentSkip to conversation

net: phylink: simplify SFP PHY attachment

PATCH Superseded Russell King (Oracle) 4 participants 11 messages Oct 22, 2024 – Oct 23, 2024

Subsystems (4)

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Russell King (Oracle)

author maintainer
#0 lore raw

cover net: phylink: simplify SFP PHY attachment

Hi,

These two patches simplify how we attach SFP PHYs.

The first patch notices that at the two sites where we call sfp_select_interface(), if that fails, we always print the same error. Move this into its own function.

The second patch adds an additional level of validation, checking that the returned interface is one that is supported by the MAC/PCS.

The last patch simplifies how SFP PHYs are attached, reducing the number of times that we do validation in this path.

 drivers/net/phy/phylink.c | 82 ++++++++++++++++++++++++-----------------------
 1 file changed, 42 insertions(+), 40 deletions(-)
Signature · 2 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
#1 lore raw

PATCH 1/3 net: phylink: add common validation for sfp_select_interface()

Whenever we call sfp_select_interface(), we check the returned value and print an error. There are two cases where this happens with the same message. Provide a common function to do this.

---
 drivers/net/phy/phylink.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)
drivers/net/phy/phylink.c +19 -13
index b5870f8666ac..62d347d1112c 100644
@@ -2415,6 +2415,21 @@ int phylink_ethtool_set_wol(struct phylink *pl, struct ethtool_wolinfo *wol)
 }
 EXPORT_SYMBOL_GPL(phylink_ethtool_set_wol);
 
+static phy_interface_t phylink_sfp_select_interface(struct phylink *pl,
+						const unsigned long *link_modes)
+{
+	phy_interface_t interface;
+
+	interface = sfp_select_interface(pl->sfp_bus, link_modes);
+	if (interface == PHY_INTERFACE_MODE_NA)
+		phylink_err(pl,
+			    "selection of interface failed, advertisement %*pb\n",
+			    __ETHTOOL_LINK_MODE_MASK_NBITS,
+			    link_modes);
+
+	return interface;
+}
+
 static void phylink_merge_link_mode(unsigned long *dst, const unsigned long *b)
 {
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask);
@@ -2597,15 +2612,10 @@ int phylink_ethtool_ksettings_set(struct phylink *pl,
 	 * link can be configured correctly.
 	 */
 	if (pl->sfp_bus) {
-		config.interface = sfp_select_interface(pl->sfp_bus,
+		config.interface = phylink_sfp_select_interface(pl,
 							config.advertising);
-		if (config.interface == PHY_INTERFACE_MODE_NA) {
-			phylink_err(pl,
-				    "selection of interface failed, advertisement %*pb\n",
-				    __ETHTOOL_LINK_MODE_MASK_NBITS,
-				    config.advertising);
+		if (config.interface == PHY_INTERFACE_MODE_NA)
 			return -EINVAL;
-		}
 
 		/* Revalidate with the selected interface */
 		linkmode_copy(support, pl->supported);
@@ -3234,13 +3244,9 @@ static int phylink_sfp_config_phy(struct phylink *pl, u8 mode,
 		return ret;
 	}
 
-	iface = sfp_select_interface(pl->sfp_bus, config.advertising);
-	if (iface == PHY_INTERFACE_MODE_NA) {
-		phylink_err(pl,
-			    "selection of interface failed, advertisement %*pb\n",
-			    __ETHTOOL_LINK_MODE_MASK_NBITS, config.advertising);
+	iface = phylink_sfp_select_interface(pl, config.advertising);
+	if (iface == PHY_INTERFACE_MODE_NA)
 		return -EINVAL;
-	}
 
 	config.interface = iface;
 	linkmode_copy(support1, support);
Signature · 1 line
2.30.2
Reviewed-by Tested-by on v1 1/3
#2 lore raw

Re: [PATCH net-next 1/3] net: phylink: add common validation for sfp_select_interface()

Quoted 5 lines · On Tue, 22 Oct 2024 12:54:07 +0100 "Russell King (Oracle)"…

Whenever we call sfp_select_interface(), we check the returned value and print an error. There are two cases where this happens with the same message. Provide a common function to do this.

I gave the series a test on 2 platforms I have that use phylink with SFPs:

- The Macchiatobin, interface eth3 with different copper modules
- A board that has an Armada 3720 wired to an SFP cage

Thanks,

Maxime

#3 lore raw

PATCH 2/3 net: phylink: validate sfp_select_interface() returned interface

In reply to Russell King (Oracle)

Validate that the returned interface from sfp_select_interface() is supportable by the MAC/PCS. If it isn't, print an error and return the NA interface type. This is a preparatory step to reorganising how a PHY on a SFP module is handled.

---
 drivers/net/phy/phylink.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)
drivers/net/phy/phylink.c +12 -1
index 62d347d1112c..4049d85cb477 100644
@@ -2421,11 +2421,22 @@ static phy_interface_t phylink_sfp_select_interface(struct phylink *pl,
 	phy_interface_t interface;
 
 	interface = sfp_select_interface(pl->sfp_bus, link_modes);
-	if (interface == PHY_INTERFACE_MODE_NA)
+	if (interface == PHY_INTERFACE_MODE_NA) {
 		phylink_err(pl,
 			    "selection of interface failed, advertisement %*pb\n",
 			    __ETHTOOL_LINK_MODE_MASK_NBITS,
 			    link_modes);
+		return interface;
+	}
+
+	if (!test_bit(interface, pl->config->supported_interfaces)) {
+		phylink_err(pl,
+			    "selection of interface failed, SFP selected %s (%u) but MAC supports %*pbl\n",
+			    phy_modes(interface), interface,
+			    (int)PHY_INTERFACE_MODE_MAX,
+			    pl->config->supported_interfaces);
+		return PHY_INTERFACE_MODE_NA;
+	}
 
 	return interface;
 }
Signature · 1 line
2.30.2
Reviewed-by Tested-by on v1 2/3
#4 lore raw

Re: [PATCH net-next 2/3] net: phylink: validate sfp_select_interface() returned interface

Quoted 6 lines · On Tue, 22 Oct 2024 12:54:12 +0100 "Russell King (Oracle)"…

Validate that the returned interface from sfp_select_interface() is supportable by the MAC/PCS. If it isn't, print an error and return the NA interface type. This is a preparatory step to reorganising how a PHY on a SFP module is handled.

Thanks,

Maxime

#5 lore raw

PATCH 3/3 net: phylink: simplify how SFP PHYs are attached

In reply to Russell King (Oracle)

There are a few issues with how SFP PHYs are attached:

a) The phylink_sfp_connect_phy() and phylink_sfp_config_phy() code validates the configuration three times:

1. To discover the support/advertising masks that the PHY/PCS/MAC can support in order to select an interface.
2. To validate the selected interface.
3. When the PHY is brought up after being attached, another validation is done.

This is needlessly complex.

b) The configuration is set prior to the PHY being attached, which means we don't have the PHY available in phylink_major_config() for phylink_pcs_neg_mode() to make decisions upon.

We have already added an extra step to validate the selected interface, so we can now move the attachment and bringup of the PHY earlier, inside phylink_sfp_config_phy(). This results in the validation at step 2 above becoming entirely unnecessary, so remove that too.

---
 drivers/net/phy/phylink.c | 43 +++++++++++++--------------------------
 1 file changed, 14 insertions(+), 29 deletions(-)
drivers/net/phy/phylink.c +14 -29
index 4049d85cb477..183d6fc6d416 100644
@@ -3231,10 +3231,8 @@ static void phylink_sfp_set_config(struct phylink *pl, u8 mode,
 static int phylink_sfp_config_phy(struct phylink *pl, u8 mode,
 				  struct phy_device *phy)
 {
-	__ETHTOOL_DECLARE_LINK_MODE_MASK(support1);
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(support);
 	struct phylink_link_state config;
-	phy_interface_t iface;
 	int ret;
 
 	linkmode_copy(support, phy->supported);
@@ -3255,20 +3253,21 @@ static int phylink_sfp_config_phy(struct phylink *pl, u8 mode,
 		return ret;
 	}
 
-	iface = phylink_sfp_select_interface(pl, config.advertising);
-	if (iface == PHY_INTERFACE_MODE_NA)
+	config.interface = phylink_sfp_select_interface(pl, config.advertising);
+	if (config.interface == PHY_INTERFACE_MODE_NA)
 		return -EINVAL;
 
-	config.interface = iface;
-	linkmode_copy(support1, support);
-	ret = phylink_validate(pl, support1, &config);
-	if (ret) {
-		phylink_err(pl,
-			    "validation of %s/%s with support %*pb failed: %pe\n",
-			    phylink_an_mode_str(mode),
-			    phy_modes(config.interface),
-			    __ETHTOOL_LINK_MODE_MASK_NBITS, support,
-			    ERR_PTR(ret));
+	/* Attach the PHY so that the PHY is present when we do the major
+	 * configuration step.
+	 */
+	ret = phylink_attach_phy(pl, phy, config.interface);
+	if (ret < 0)
+		return ret;
+
+	/* This will validate the configuration for us. */
+	ret = phylink_bringup_phy(pl, phy, config.interface);
+	if (ret < 0) {
+		phy_detach(phy);
 		return ret;
 	}
 
@@ -3426,7 +3425,6 @@ static bool phylink_phy_no_inband(struct phy_device *phy)
 static int phylink_sfp_connect_phy(void *upstream, struct phy_device *phy)
 {
 	struct phylink *pl = upstream;
-	phy_interface_t interface;
 	u8 mode;
 	int ret;
 
@@ -3449,20 +3447,7 @@ static int phylink_sfp_connect_phy(void *upstream, struct phy_device *phy)
 			  pl->config->supported_interfaces);
 
 	/* Do the initial configuration */
-	ret = phylink_sfp_config_phy(pl, mode, phy);
-	if (ret < 0)
-		return ret;
-
-	interface = pl->link_config.interface;
-	ret = phylink_attach_phy(pl, phy, interface);
-	if (ret < 0)
-		return ret;
-
-	ret = phylink_bringup_phy(pl, phy, interface);
-	if (ret)
-		phy_detach(phy);
-
-	return ret;
+	return phylink_sfp_config_phy(pl, mode, phy);
 }
 
 static void phylink_sfp_disconnect_phy(void *upstream,
Signature · 1 line
2.30.2
author maintainer
#6 lore raw

Re: [PATCH net-next 3/3] net: phylink: simplify how SFP PHYs are attached

Quoted 17 lines · Russell King (Oracle)

There are a few issues with how SFP PHYs are attached:

a) The phylink_sfp_connect_phy() and phylink_sfp_config_phy() code validates the configuration three times:

1. To discover the support/advertising masks that the PHY/PCS/MAC can support in order to select an interface.
2. To validate the selected interface.
3. When the PHY is brought up after being attached, another validation is done.

This is needlessly complex.

b) The configuration is set prior to the PHY being attached, which means we don't have the PHY available in phylink_major_config() for phylink_pcs_neg_mode() to make decisions upon.

Quoted 6 lines

We have already added an extra step to validate the selected interface, so we can now move the attachment and bringup of the PHY earlier, inside phylink_sfp_config_phy(). This results in the validation at step 2 above becoming entirely unnecessary, so remove that too.

There's a build warning in this patch, which I'll address - but please let me have any comments in the mean time.

drivers/net/phy/phylink.c static bool phylink_phy_no_inband(struct phy_device *phy) · line 3429

Quoted 6 lines

static int phylink_sfp_connect_phy(void *upstream, struct phy_device *phy) {

drivers/net/phy/phylink.c +0 -1
 	struct phylink *pl = upstream;
-	phy_interface_t interface;
 	u8 mode;
 	int ret;

"ret" needs to be removed.

Signature · 2 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Reviewed-by Tested-by on v1 3/3
#7 lore raw

Re: [PATCH net-next 3/3] net: phylink: simplify how SFP PHYs are attached

In reply to Russell King (Oracle)

Hi Russell,

Quoted 17 lines · On Tue, 22 Oct 2024 12:54:17 +0100 "Russell King (Oracle)"…

There are a few issues with how SFP PHYs are attached:

a) The phylink_sfp_connect_phy() and phylink_sfp_config_phy() code validates the configuration three times:

1. To discover the support/advertising masks that the PHY/PCS/MAC can support in order to select an interface.
2. To validate the selected interface.
3. When the PHY is brought up after being attached, another validation is done.

This is needlessly complex.

b) The configuration is set prior to the PHY being attached, which means we don't have the PHY available in phylink_major_config() for phylink_pcs_neg_mode() to make decisions upon.

Quoted 6 lines

We have already added an extra step to validate the selected interface, so we can now move the attachment and bringup of the PHY earlier, inside phylink_sfp_config_phy(). This results in the validation at step 2 above becoming entirely unnecessary, so remove that too.

This is indeed cleaner and easier to understand, thanks.

Modulo the warning,

Thanks,

Maxime

#8 lore raw

Re: [PATCH net-next 3/3] net: phylink: simplify how SFP PHYs are attached

In reply to Russell King (Oracle)
kernel test robot noticed the following build warnings:

Hi Russell,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Russell-King-Oracle/net-phylink-add-common-validation-for-sfp_select_interface/20241022-234251
base:   net-next/main
patch link:    https://lore.kernel.org/r/E1t3DSr-000Vxg-Qx%40rmk-PC.armlinux.org.uk
patch subject: [PATCH net-next 3/3] net: phylink: simplify how SFP PHYs are attached
config: x86_64-kexec (https://download.01.org/0day-ci/archive/20241023/[email protected]/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241023/[email protected]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All warnings (new ones prefixed by >>):

>> drivers/net/phy/phylink.c:3429:6: warning: unused variable 'ret' [-Wunused-variable]
    3429 |         int ret;
         |             ^~~

1 warning generated.

vim +/ret +3429 drivers/net/phy/phylink.c

7adb5b2126bc013 Russell King          2019-12-11  3424  
ce0aa27ff3f68ed Russell King          2017-07-25  3425  static int phylink_sfp_connect_phy(void *upstream, struct phy_device *phy)
ce0aa27ff3f68ed Russell King          2017-07-25  3426  {
7e4183752735deb Baruch Siach          2018-10-03  3427  	struct phylink *pl = upstream;
7adb5b2126bc013 Russell King          2019-12-11  3428  	u8 mode;
938d44c2b502ba4 Russell King          2019-12-11 @3429  	int ret;
7e4183752735deb Baruch Siach          2018-10-03  3430  
52c956003a9d5bc Russell King          2019-12-11  3431  	/*
52c956003a9d5bc Russell King          2019-12-11  3432  	 * This is the new way of dealing with flow control for PHYs,
52c956003a9d5bc Russell King          2019-12-11  3433  	 * as described by Timur Tabi in commit 529ed1275263 ("net: phy:
52c956003a9d5bc Russell King          2019-12-11  3434  	 * phy drivers should not set SUPPORTED_[Asym_]Pause") except
52c956003a9d5bc Russell King          2019-12-11  3435  	 * using our validate call to the MAC, we rely upon the MAC
52c956003a9d5bc Russell King          2019-12-11  3436  	 * clearing the bits from both supported and advertising fields.
52c956003a9d5bc Russell King          2019-12-11  3437  	 */
52c956003a9d5bc Russell King          2019-12-11  3438  	phy_support_asym_pause(phy);
52c956003a9d5bc Russell King          2019-12-11  3439  
7adb5b2126bc013 Russell King          2019-12-11  3440  	if (phylink_phy_no_inband(phy))
7adb5b2126bc013 Russell King          2019-12-11  3441  		mode = MLO_AN_PHY;
7adb5b2126bc013 Russell King          2019-12-11  3442  	else
7adb5b2126bc013 Russell King          2019-12-11  3443  		mode = MLO_AN_INBAND;
7adb5b2126bc013 Russell King          2019-12-11  3444  
eca68a3c7d05b38 Marek Behún           2022-09-30  3445  	/* Set the PHY's host supported interfaces */
eca68a3c7d05b38 Marek Behún           2022-09-30  3446  	phy_interface_and(phy->host_interfaces, phylink_sfp_interfaces,
eca68a3c7d05b38 Marek Behún           2022-09-30  3447  			  pl->config->supported_interfaces);
eca68a3c7d05b38 Marek Behún           2022-09-30  3448  
52c956003a9d5bc Russell King          2019-12-11  3449  	/* Do the initial configuration */
e48bc58d0fa1e08 Russell King (Oracle  2024-10-22  3450) 	return phylink_sfp_config_phy(pl, mode, phy);
ce0aa27ff3f68ed Russell King          2017-07-25  3451  }
ce0aa27ff3f68ed Russell King          2017-07-25  3452  
Signature · 2 lines
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
#9 lore raw
In reply to Russell King (Oracle)

Hello Russell,

Quoted 8 lines · On Tue, 22 Oct 2024 12:39:06 +0100 "Russell King (Oracle)"…

Hi,

These two patches simplify how we attach SFP PHYs.

The first patch notices that at the two sites where we call sfp_select_interface(), if that fails, we always print the same error. Move this into its own function.

Quoted 8 lines

The second patch adds an additional level of validation, checking that the returned interface is one that is supported by the MAC/PCS.

The last patch simplifies how SFP PHYs are attached, reducing the number of times that we do validation in this path.

 drivers/net/phy/phylink.c | 82 ++++++++++++++++++++++++-----------------------
 1 file changed, 42 insertions(+), 40 deletions(-)

It looks like the patches didn't make it through, there're also not on lore nor patchwork :(

Maxime

author maintainer
#10 lore raw
Quoted 16 lines · Maxime Chevallier

Hello Russell,

Quoted 11 lines · On Tue, 22 Oct 2024 12:39:06 +0100 "Russell King (Oracle)"…

Hi,

These two patches simplify how we attach SFP PHYs.

The first patch notices that at the two sites where we call sfp_select_interface(), if that fails, we always print the same error. Move this into its own function.

The second patch adds an additional level of validation, checking that the returned interface is one that is supported by the MAC/PCS.

Quoted 8 lines
Quoted 5 lines

The last patch simplifies how SFP PHYs are attached, reducing the number of times that we do validation in this path.

 drivers/net/phy/phylink.c | 82 ++++++++++++++++++++++++-----------------------
 1 file changed, 42 insertions(+), 40 deletions(-)

It looks like the patches didn't make it through, there're also not on lore nor patchwork :(

You're too quick. I was delaying the patches for two reasons:

1) to ensure Jakub's nipa bot bug doesn't get me again.
2) build-testing the series, as I'd made a change... and unsurprisingly not waiting for that to finish, and sending the patches, immediately after sending it found a problem. :(

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