[PATCH net-next 0/3] net: phylink: simplify SFP PHY attachment

[PATCH net-next 0/3] net: phylink: simplify SFP PHY attachment

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

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(-)

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

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

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

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phylink.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index b5870f8666ac..62d347d1112c 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -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);
-- 
2.30.2

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

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

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phylink.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 62d347d1112c..4049d85cb477 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -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;
 }
-- 
2.30.2

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

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

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phylink.c | 43 +++++++++++++--------------------------
 1 file changed, 14 insertions(+), 29 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 4049d85cb477..183d6fc6d416 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -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,
-- 
2.30.2

Re: [PATCH net-next 0/3] net: phylink: simplify SFP PHY attachment

[Maxime Chevallier]

Hello Russell,

On Tue, 22 Oct 2024 12:39:06 +0100
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:

> 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(-)

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

Maxime

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

[Russell King (Oracle)]

On Tue, Oct 22, 2024 at 12:54:17PM +0100, Russell King (Oracle) wrote:
> 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.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

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

>  static int phylink_sfp_connect_phy(void *upstream, struct phy_device *phy)
>  {
>  	struct phylink *pl = upstream;
> -	phy_interface_t interface;
>  	u8 mode;
>  	int ret;

"ret" needs to be removed.

-- 
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 0/3] net: phylink: simplify SFP PHY attachment

[Russell King (Oracle)]

On Tue, Oct 22, 2024 at 01:54:47PM +0200, Maxime Chevallier wrote:
> Hello Russell,
> 
> On Tue, 22 Oct 2024 12:39:06 +0100
> "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> 
> > 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(-)
> 
> 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. :(

-- 
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 1/3] net: phylink: add common validation for sfp_select_interface()

[Maxime Chevallier]

On Tue, 22 Oct 2024 12:54:07 +0100
"Russell King (Oracle)" <rmk+kernel@armlinux.org.uk> wrote:

> 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.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

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

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Tested-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Thanks,

Maxime

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

[Maxime Chevallier]

On Tue, 22 Oct 2024 12:54:12 +0100
"Russell King (Oracle)" <rmk+kernel@armlinux.org.uk> wrote:

> 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.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Tested-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Thanks,

Maxime

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

[Maxime Chevallier]

Hi Russell,

On Tue, 22 Oct 2024 12:54:17 +0100
"Russell King (Oracle)" <rmk+kernel@armlinux.org.uk> wrote:

> 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.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

This is indeed cleaner and easier to understand, thanks.

Modulo the warning,

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Tested-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Thanks,

Maxime