[PATCH net-next 0/3] net: phylink: support !autoneg configuration for SFPs

[PATCH net-next 0/3] net: phylink: support !autoneg configuration for SFPs

[Russell King (Oracle)]

This series comes from discussion during a patch series that was posted
at the beginning of April, but these patches were never posted (I was
too busy!)

We restrict ->sfp_interfaces to those that the host system supports,
and ensure that ->sfp_interfaces is cleared when a SFP is removed. We
then add phylink_sfp_select_interface_speed() which will select an
appropriate interface from ->sfp_interfaces for the speed, and use that
in our phylink_ethtool_ksettings_set() when a SFP bus is present on a
directly connected host (not with a PHY.)

 drivers/net/phy/phylink.c | 60 ++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 54 insertions(+), 6 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: restrict SFP interfaces to those that are supported

[Russell King (Oracle)]

When configuring an optical SFP interface, restrict the bitmap of SFP
interfaces (pl->sfp_interfaces) to those that are supported by the
host, rather than calculating this in a local variable.

This will allow us to avoid recomputing this in the
phylink_ethtool_ksettings_set() path.

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

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 67218d278ce6..6420e76f8ab1 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -3582,7 +3582,6 @@ static int phylink_sfp_config_phy(struct phylink *pl, struct phy_device *phy)
 static int phylink_sfp_config_optical(struct phylink *pl)
 {
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(support);
-	DECLARE_PHY_INTERFACE_MASK(interfaces);
 	struct phylink_link_state config;
 	phy_interface_t interface;
 	int ret;
@@ -3596,9 +3595,9 @@ static int phylink_sfp_config_optical(struct phylink *pl)
 	/* Find the union of the supported interfaces by the PCS/MAC and
 	 * the SFP module.
 	 */
-	phy_interface_and(interfaces, pl->config->supported_interfaces,
+	phy_interface_and(pl->sfp_interfaces, pl->config->supported_interfaces,
 			  pl->sfp_interfaces);
-	if (phy_interface_empty(interfaces)) {
+	if (phy_interface_empty(pl->sfp_interfaces)) {
 		phylink_err(pl, "unsupported SFP module: no common interface modes\n");
 		return -EINVAL;
 	}
@@ -3614,14 +3613,14 @@ static int phylink_sfp_config_optical(struct phylink *pl)
 	 * mask to only those link modes that can be supported.
 	 */
 	ret = phylink_validate_mask(pl, NULL, pl->sfp_support, &config,
-				    interfaces);
+				    pl->sfp_interfaces);
 	if (ret) {
 		phylink_err(pl, "unsupported SFP module: validation with support %*pb failed\n",
 			    __ETHTOOL_LINK_MODE_MASK_NBITS, support);
 		return ret;
 	}
 
-	interface = phylink_choose_sfp_interface(pl, interfaces);
+	interface = phylink_choose_sfp_interface(pl, pl->sfp_interfaces);
 	if (interface == PHY_INTERFACE_MODE_NA) {
 		phylink_err(pl, "failed to select SFP interface\n");
 		return -EINVAL;
-- 
2.30.2

Re: [PATCH net-next 1/3] net: phylink: restrict SFP interfaces to those that are supported

[Maxime Chevallier]

On Wed, 02 Jul 2025 10:44:24 +0100
"Russell King (Oracle)" <rmk+kernel@armlinux.org.uk> wrote:

> When configuring an optical SFP interface, restrict the bitmap of SFP
> interfaces (pl->sfp_interfaces) to those that are supported by the
> host, rather than calculating this in a local variable.
> 
> This will allow us to avoid recomputing this in the
> phylink_ethtool_ksettings_set() path.
> 
> 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 Russell,

Maxime

[PATCH net-next 2/3] net: phylink: clear SFP interfaces when not in use

[Russell King (Oracle)]

Clear the SFP interfaces bitmap when we're not using it - in other
words, when a module is unplugged, or we're using a PHY on the
module.

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

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 6420e76f8ab1..c92a878ab717 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -3536,6 +3536,8 @@ static int phylink_sfp_config_phy(struct phylink *pl, struct phy_device *phy)
 	struct phylink_link_state config;
 	int ret;
 
+	/* We're not using pl->sfp_interfaces, so clear it. */
+	phy_interface_zero(pl->sfp_interfaces);
 	linkmode_copy(support, phy->supported);
 
 	memset(&config, 0, sizeof(config));
@@ -3673,6 +3675,13 @@ static int phylink_sfp_module_insert(void *upstream,
 	return phylink_sfp_config_optical(pl);
 }
 
+static void phylink_sfp_module_remove(void *upstream)
+{
+	struct phylink *pl = upstream;
+
+	phy_interface_zero(pl->sfp_interfaces);
+}
+
 static int phylink_sfp_module_start(void *upstream)
 {
 	struct phylink *pl = upstream;
@@ -3757,6 +3766,7 @@ static const struct sfp_upstream_ops sfp_phylink_ops = {
 	.attach = phylink_sfp_attach,
 	.detach = phylink_sfp_detach,
 	.module_insert = phylink_sfp_module_insert,
+	.module_remove = phylink_sfp_module_remove,
 	.module_start = phylink_sfp_module_start,
 	.module_stop = phylink_sfp_module_stop,
 	.link_up = phylink_sfp_link_up,
-- 
2.30.2

Re: [PATCH net-next 2/3] net: phylink: clear SFP interfaces when not in use

[Maxime Chevallier]

On Wed, 02 Jul 2025 10:44:29 +0100
"Russell King (Oracle)" <rmk+kernel@armlinux.org.uk> wrote:

> Clear the SFP interfaces bitmap when we're not using it - in other
> words, when a module is unplugged, or we're using a PHY on the
> module.
> 
> 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

[PATCH net-next 3/3] net: phylink: add phylink_sfp_select_interface_speed()

[Russell King (Oracle)]

Add phylink_sfp_select_interface_speed() which attempts to select the
SFP interface based on the ethtool speed when autoneg is turned off.
This allows users to turn off autoneg for SFPs that support multiple
interface modes, and have an appropriate interface mode selected.

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

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index c92a878ab717..f5473510b762 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -2709,6 +2709,39 @@ static phy_interface_t phylink_sfp_select_interface(struct phylink *pl,
 	return interface;
 }
 
+static phy_interface_t phylink_sfp_select_interface_speed(struct phylink *pl,
+							  u32 speed)
+{
+	phy_interface_t best_interface = PHY_INTERFACE_MODE_NA;
+	phy_interface_t interface;
+	u32 max_speed;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(phylink_sfp_interface_preference); i++) {
+		interface = phylink_sfp_interface_preference[i];
+		if (!test_bit(interface, pl->sfp_interfaces))
+			continue;
+
+		max_speed = phylink_interface_max_speed(interface);
+
+		/* The logic here is: if speed == max_speed, then we've found
+		 * the best interface. Otherwise we find the interface that
+		 * can just support the requested speed.
+		 */
+		if (max_speed >= speed)
+			best_interface = interface;
+
+		if (max_speed <= speed)
+			break;
+	}
+
+	if (best_interface == PHY_INTERFACE_MODE_NA)
+		phylink_err(pl, "selection of interface failed, speed %u\n",
+			    speed);
+
+	return best_interface;
+}
+
 static void phylink_merge_link_mode(unsigned long *dst, const unsigned long *b)
 {
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask);
@@ -2911,8 +2944,14 @@ int phylink_ethtool_ksettings_set(struct phylink *pl,
 	 * link can be configured correctly.
 	 */
 	if (pl->sfp_bus) {
-		config.interface = phylink_sfp_select_interface(pl,
+		if (kset->base.autoneg == AUTONEG_ENABLE)
+			config.interface =
+				phylink_sfp_select_interface(pl,
 							config.advertising);
+		else
+			config.interface =
+				phylink_sfp_select_interface_speed(pl,
+							config.speed);
 		if (config.interface == PHY_INTERFACE_MODE_NA)
 			return -EINVAL;
 
-- 
2.30.2

Re: [PATCH net-next 3/3] net: phylink: add phylink_sfp_select_interface_speed()

[Maxime Chevallier]

On Wed, 02 Jul 2025 10:44:34 +0100
"Russell King (Oracle)" <rmk+kernel@armlinux.org.uk> wrote:

> Add phylink_sfp_select_interface_speed() which attempts to select the
> SFP interface based on the ethtool speed when autoneg is turned off.
> This allows users to turn off autoneg for SFPs that support multiple
> interface modes, and have an appropriate interface mode selected.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

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

I don't have any hardware to perform relevant tests on this :(

Maxime

Re: [PATCH net-next 3/3] net: phylink: add phylink_sfp_select_interface_speed()

[Russell King (Oracle)]

On Wed, Jul 02, 2025 at 03:14:26PM +0200, Maxime Chevallier wrote:
> On Wed, 02 Jul 2025 10:44:34 +0100
> "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk> wrote:
> 
> > Add phylink_sfp_select_interface_speed() which attempts to select the
> > SFP interface based on the ethtool speed when autoneg is turned off.
> > This allows users to turn off autoneg for SFPs that support multiple
> > interface modes, and have an appropriate interface mode selected.
> > 
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> 
> Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> 
> I don't have any hardware to perform relevant tests on this :(

Me neither, I should've said. I'd like to see a t-b from
Alexander Duyck who originally had the problem before this is
merged.

-- 
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 3/3] net: phylink: add phylink_sfp_select_interface_speed()

[Alexander Duyck]

On Wed, Jul 2, 2025 at 6:37 AM Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
>
> On Wed, Jul 02, 2025 at 03:14:26PM +0200, Maxime Chevallier wrote:
> > On Wed, 02 Jul 2025 10:44:34 +0100
> > "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk> wrote:
> >
> > > Add phylink_sfp_select_interface_speed() which attempts to select the
> > > SFP interface based on the ethtool speed when autoneg is turned off.
> > > This allows users to turn off autoneg for SFPs that support multiple
> > > interface modes, and have an appropriate interface mode selected.
> > >
> > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> >
> > Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> >
> > I don't have any hardware to perform relevant tests on this :(
>
> Me neither, I should've said. I'd like to see a t-b from
> Alexander Duyck who originally had the problem before this is
> merged.

It will probably be several days before I can get around to testing it
since I am slammed with meetings most of the next two days, then have
a holiday weekend coming up. I will need to rebase the patches I had
that add support for SFF-8636 and the QSFP support for fbnic before I
can get to testing it. As such you can probably look at having this
pulled in and I will try to get it tested next week.

To add a bit of complexity to all this I am looking at reworking the
fbnic driver to probably add a phydev or some type of c45 swphy to
handle the PMA/PMD (serdes PHY) and AN which is mostly handled by the
firmware which provides the advertised link config. The main reason
for doing this is that the PMD has a 4 second training window that the
firmware kicks in after the link comes up, and I need to essentially
do an "&=" on the link state to keep the PCS from reporting the link
up while training is causing the link to flap.

Re: [PATCH net-next 3/3] net: phylink: add phylink_sfp_select_interface_speed()

[Russell King (Oracle)]

On Wed, Jul 02, 2025 at 11:07:52AM -0700, Alexander Duyck wrote:
> On Wed, Jul 2, 2025 at 6:37 AM Russell King (Oracle)
> <linux@armlinux.org.uk> wrote:
> >
> > On Wed, Jul 02, 2025 at 03:14:26PM +0200, Maxime Chevallier wrote:
> > > On Wed, 02 Jul 2025 10:44:34 +0100
> > > "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk> wrote:
> > >
> > > > Add phylink_sfp_select_interface_speed() which attempts to select the
> > > > SFP interface based on the ethtool speed when autoneg is turned off.
> > > > This allows users to turn off autoneg for SFPs that support multiple
> > > > interface modes, and have an appropriate interface mode selected.
> > > >
> > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > >
> > > Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > >
> > > I don't have any hardware to perform relevant tests on this :(
> >
> > Me neither, I should've said. I'd like to see a t-b from
> > Alexander Duyck who originally had the problem before this is
> > merged.
> 
> It will probably be several days before I can get around to testing it
> since I am slammed with meetings most of the next two days, then have
> a holiday weekend coming up.

I, too, have a vacation - from tomorrow for three weeks. I may dip in
and out of kernel emails during that period, but it depends what
happens each day.

-- 
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 3/3] net: phylink: add phylink_sfp_select_interface_speed()

[Alexander Duyck]

On Wed, Jul 2, 2025 at 12:18 PM Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
>
> On Wed, Jul 02, 2025 at 11:07:52AM -0700, Alexander Duyck wrote:
> > On Wed, Jul 2, 2025 at 6:37 AM Russell King (Oracle)
> > <linux@armlinux.org.uk> wrote:
> > >
> > > On Wed, Jul 02, 2025 at 03:14:26PM +0200, Maxime Chevallier wrote:
> > > > On Wed, 02 Jul 2025 10:44:34 +0100
> > > > "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk> wrote:
> > > >
> > > > > Add phylink_sfp_select_interface_speed() which attempts to select the
> > > > > SFP interface based on the ethtool speed when autoneg is turned off.
> > > > > This allows users to turn off autoneg for SFPs that support multiple
> > > > > interface modes, and have an appropriate interface mode selected.
> > > > >
> > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > > >
> > > > Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > > >
> > > > I don't have any hardware to perform relevant tests on this :(
> > >
> > > Me neither, I should've said. I'd like to see a t-b from
> > > Alexander Duyck who originally had the problem before this is
> > > merged.
> >
> > It will probably be several days before I can get around to testing it
> > since I am slammed with meetings most of the next two days, then have
> > a holiday weekend coming up.
>
> I, too, have a vacation - from tomorrow for three weeks. I may dip in
> and out of kernel emails during that period, but it depends what
> happens each day.

So I was able to go in and test it. I ended up just running the
testing in QEMU w/ my patch set that currently enables QSFP support.
From what I can tell it appears to be mostly working. Before when I
tried to alter the speed to go from 100G to 50G it wouldn't change.
After your patch set it appears to change, although I am noticing a
slight difference from the default config.

So by default we come up in the 100G w/ the QSFP configuration:
[root@localhost fbnic]# ethtool enp1s0
Settings for enp1s0:
        Supported ports: [  ]
        Supported link modes:   50000baseCR/Full
                                100000baseCR2/Full
        Supported pause frame use: Symmetric Receive-only
        Supports auto-negotiation: No
        Supported FEC modes: RS
        Advertised link modes:  100000baseCR2/Full
        Advertised pause frame use: Symmetric Receive-only
        Advertised auto-negotiation: No
        Advertised FEC modes: RS
        Link partner advertised link modes:  100000baseCR2/Full
        Link partner advertised pause frame use: No
        Link partner advertised auto-negotiation: No
        Link partner advertised FEC modes: RS
        Speed: 100000Mb/s
        Duplex: Full
        Auto-negotiation: off
        Port: Other
        PHYAD: 0
        Transceiver: internal
        Link detected: yes

I then change the speed to 50G and it links back up after a few
seconds, however the "Advertised link modes" goes from
"100000baseCR2/Full" to "Not reported" as shown here:
[root@localhost fbnic]# ethtool -s enp1s0 speed 50000
[root@localhost fbnic]# ethtool enp1s0
Settings for enp1s0:
        Supported ports: [  ]
        Supported link modes:   50000baseCR/Full
                                100000baseCR2/Full
        Supported pause frame use: Symmetric Receive-only
        Supports auto-negotiation: No
        Supported FEC modes: RS
        Advertised link modes:  Not reported
        Advertised pause frame use: Symmetric Receive-only
        Advertised auto-negotiation: No
        Advertised FEC modes: RS
        Link partner advertised link modes:  100000baseCR2/Full
        Link partner advertised pause frame use: No
        Link partner advertised auto-negotiation: No
        Link partner advertised FEC modes: RS
        Speed: 50000Mb/s
        Duplex: Full
        Auto-negotiation: off
        Port: Other
        PHYAD: 0
        Transceiver: internal
        Link detected: yes

So all-in-all it is an improvement over the previous behavior although
there may still need to be some work done to improve the consistency
so that it more closely matches up with what happens when you
initially configure the interface.

Re: [PATCH net-next 3/3] net: phylink: add phylink_sfp_select_interface_speed()

[Russell King (Oracle)]

On Wed, Jul 09, 2025 at 08:37:51AM -0700, Alexander Duyck wrote:
> On Wed, Jul 2, 2025 at 12:18 PM Russell King (Oracle)
> <linux@armlinux.org.uk> wrote:
> >
> > On Wed, Jul 02, 2025 at 11:07:52AM -0700, Alexander Duyck wrote:
> > > On Wed, Jul 2, 2025 at 6:37 AM Russell King (Oracle)
> > > <linux@armlinux.org.uk> wrote:
> > > >
> > > > On Wed, Jul 02, 2025 at 03:14:26PM +0200, Maxime Chevallier wrote:
> > > > > On Wed, 02 Jul 2025 10:44:34 +0100
> > > > > "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk> wrote:
> > > > >
> > > > > > Add phylink_sfp_select_interface_speed() which attempts to select the
> > > > > > SFP interface based on the ethtool speed when autoneg is turned off.
> > > > > > This allows users to turn off autoneg for SFPs that support multiple
> > > > > > interface modes, and have an appropriate interface mode selected.
> > > > > >
> > > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > > > >
> > > > > Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > > > >
> > > > > I don't have any hardware to perform relevant tests on this :(
> > > >
> > > > Me neither, I should've said. I'd like to see a t-b from
> > > > Alexander Duyck who originally had the problem before this is
> > > > merged.
> > >
> > > It will probably be several days before I can get around to testing it
> > > since I am slammed with meetings most of the next two days, then have
> > > a holiday weekend coming up.
> >
> > I, too, have a vacation - from tomorrow for three weeks. I may dip in
> > and out of kernel emails during that period, but it depends what
> > happens each day.
> 
> So I was able to go in and test it. I ended up just running the
> testing in QEMU w/ my patch set that currently enables QSFP support.
> From what I can tell it appears to be mostly working. Before when I
> tried to alter the speed to go from 100G to 50G it wouldn't change.
> After your patch set it appears to change, although I am noticing a
> slight difference from the default config.
> 
> So by default we come up in the 100G w/ the QSFP configuration:
> [root@localhost fbnic]# ethtool enp1s0
> Settings for enp1s0:
>         Supported ports: [  ]
>         Supported link modes:   50000baseCR/Full
>                                 100000baseCR2/Full
>         Supported pause frame use: Symmetric Receive-only
>         Supports auto-negotiation: No
>         Supported FEC modes: RS
>         Advertised link modes:  100000baseCR2/Full
>         Advertised pause frame use: Symmetric Receive-only
>         Advertised auto-negotiation: No
>         Advertised FEC modes: RS
>         Link partner advertised link modes:  100000baseCR2/Full
>         Link partner advertised pause frame use: No
>         Link partner advertised auto-negotiation: No
>         Link partner advertised FEC modes: RS
>         Speed: 100000Mb/s
>         Duplex: Full
>         Auto-negotiation: off
>         Port: Other
>         PHYAD: 0
>         Transceiver: internal
>         Link detected: yes
> 
> I then change the speed to 50G and it links back up after a few
> seconds, however the "Advertised link modes" goes from
> "100000baseCR2/Full" to "Not reported" as shown here:
> [root@localhost fbnic]# ethtool -s enp1s0 speed 50000
> [root@localhost fbnic]# ethtool enp1s0
> Settings for enp1s0:
>         Supported ports: [  ]
>         Supported link modes:   50000baseCR/Full
>                                 100000baseCR2/Full
>         Supported pause frame use: Symmetric Receive-only
>         Supports auto-negotiation: No
>         Supported FEC modes: RS
>         Advertised link modes:  Not reported
>         Advertised pause frame use: Symmetric Receive-only
>         Advertised auto-negotiation: No
>         Advertised FEC modes: RS
>         Link partner advertised link modes:  100000baseCR2/Full
>         Link partner advertised pause frame use: No
>         Link partner advertised auto-negotiation: No
>         Link partner advertised FEC modes: RS
>         Speed: 50000Mb/s
>         Duplex: Full
>         Auto-negotiation: off
>         Port: Other
>         PHYAD: 0
>         Transceiver: internal
>         Link detected: yes
> 
> So all-in-all it is an improvement over the previous behavior although
> there may still need to be some work done to improve the consistency
> so that it more closely matches up with what happens when you
> initially configure the interface.

Likely, it's ethtool doing that. Autoneg-off (fixed speed) modes
generally don't have an advertisement. I suggest you debug at UAPI
level.

-- 
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 3/3] net: phylink: add phylink_sfp_select_interface_speed()

[Alexander Duyck]

On Wed, Jul 9, 2025 at 8:49 AM Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
>
> On Wed, Jul 09, 2025 at 08:37:51AM -0700, Alexander Duyck wrote:
> > On Wed, Jul 2, 2025 at 12:18 PM Russell King (Oracle)
> > <linux@armlinux.org.uk> wrote:
> > >
> > > On Wed, Jul 02, 2025 at 11:07:52AM -0700, Alexander Duyck wrote:
> > > > On Wed, Jul 2, 2025 at 6:37 AM Russell King (Oracle)
> > > > <linux@armlinux.org.uk> wrote:
> > > > >
> > > > > On Wed, Jul 02, 2025 at 03:14:26PM +0200, Maxime Chevallier wrote:
> > > > > > On Wed, 02 Jul 2025 10:44:34 +0100
> > > > > > "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk> wrote:
> > > > > >
> > > > > > > Add phylink_sfp_select_interface_speed() which attempts to select the
> > > > > > > SFP interface based on the ethtool speed when autoneg is turned off.
> > > > > > > This allows users to turn off autoneg for SFPs that support multiple
> > > > > > > interface modes, and have an appropriate interface mode selected.
> > > > > > >
> > > > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > > > > >
> > > > > > Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > > > > >
> > > > > > I don't have any hardware to perform relevant tests on this :(
> > > > >
> > > > > Me neither, I should've said. I'd like to see a t-b from
> > > > > Alexander Duyck who originally had the problem before this is
> > > > > merged.
> > > >
> > > > It will probably be several days before I can get around to testing it
> > > > since I am slammed with meetings most of the next two days, then have
> > > > a holiday weekend coming up.
> > >
> > > I, too, have a vacation - from tomorrow for three weeks. I may dip in
> > > and out of kernel emails during that period, but it depends what
> > > happens each day.
> >
> > So I was able to go in and test it. I ended up just running the
> > testing in QEMU w/ my patch set that currently enables QSFP support.
> > From what I can tell it appears to be mostly working. Before when I
> > tried to alter the speed to go from 100G to 50G it wouldn't change.
> > After your patch set it appears to change, although I am noticing a
> > slight difference from the default config.
> >
> > So by default we come up in the 100G w/ the QSFP configuration:
> > [root@localhost fbnic]# ethtool enp1s0
> > Settings for enp1s0:
> >         Supported ports: [  ]
> >         Supported link modes:   50000baseCR/Full
> >                                 100000baseCR2/Full
> >         Supported pause frame use: Symmetric Receive-only
> >         Supports auto-negotiation: No
> >         Supported FEC modes: RS
> >         Advertised link modes:  100000baseCR2/Full
> >         Advertised pause frame use: Symmetric Receive-only
> >         Advertised auto-negotiation: No
> >         Advertised FEC modes: RS
> >         Link partner advertised link modes:  100000baseCR2/Full
> >         Link partner advertised pause frame use: No
> >         Link partner advertised auto-negotiation: No
> >         Link partner advertised FEC modes: RS
> >         Speed: 100000Mb/s
> >         Duplex: Full
> >         Auto-negotiation: off
> >         Port: Other
> >         PHYAD: 0
> >         Transceiver: internal
> >         Link detected: yes
> >
> > I then change the speed to 50G and it links back up after a few
> > seconds, however the "Advertised link modes" goes from
> > "100000baseCR2/Full" to "Not reported" as shown here:
> > [root@localhost fbnic]# ethtool -s enp1s0 speed 50000
> > [root@localhost fbnic]# ethtool enp1s0
> > Settings for enp1s0:
> >         Supported ports: [  ]
> >         Supported link modes:   50000baseCR/Full
> >                                 100000baseCR2/Full
> >         Supported pause frame use: Symmetric Receive-only
> >         Supports auto-negotiation: No
> >         Supported FEC modes: RS
> >         Advertised link modes:  Not reported
> >         Advertised pause frame use: Symmetric Receive-only
> >         Advertised auto-negotiation: No
> >         Advertised FEC modes: RS
> >         Link partner advertised link modes:  100000baseCR2/Full
> >         Link partner advertised pause frame use: No
> >         Link partner advertised auto-negotiation: No
> >         Link partner advertised FEC modes: RS
> >         Speed: 50000Mb/s
> >         Duplex: Full
> >         Auto-negotiation: off
> >         Port: Other
> >         PHYAD: 0
> >         Transceiver: internal
> >         Link detected: yes
> >
> > So all-in-all it is an improvement over the previous behavior although
> > there may still need to be some work done to improve the consistency
> > so that it more closely matches up with what happens when you
> > initially configure the interface.
>
> Likely, it's ethtool doing that. Autoneg-off (fixed speed) modes
> generally don't have an advertisement. I suggest you debug at UAPI
> level.

Makes sense. If I change the line to:
[root@localhost fbnic]# ethtool -s enp1s0 speed 50000 advertise
0x840000000000000

Then I am able to get it to populate the advertised mode as it did
before and we can switch back and forth with the advertised mode
listed matching the advertised speed. So it looks like the
ksettings_set call just takes in what is there and ANDs it a few times
to validate what is being selected.

Re: [PATCH net-next 3/3] net: phylink: add phylink_sfp_select_interface_speed()

[Andrew Lunn]

> Settings for enp1s0:
>         Supported ports: [  ]
>         Supported link modes:   50000baseCR/Full
>                                 100000baseCR2/Full
>         Supported pause frame use: Symmetric Receive-only
>         Supports auto-negotiation: No
>         Supported FEC modes: RS
>         Advertised link modes:  100000baseCR2/Full
>         Advertised pause frame use: Symmetric Receive-only
>         Advertised auto-negotiation: No
>         Advertised FEC modes: RS
>         Link partner advertised link modes:  100000baseCR2/Full
>         Link partner advertised pause frame use: No
>         Link partner advertised auto-negotiation: No
>         Link partner advertised FEC modes: RS

This all looks suspicious. If it does not support autoneg, how can it
know what the link partner is advertising?

If you look at what phylib does, for plain old 1G devices:

genphy_read_status() calls genphy_read_lpa()

genphy_read_lpa() then looks to see if autoneg is enabled, and if not
it does linkmode_zero(phydev->lp_advertising).

So it will not report any link partner information.

What is wrong, that it is reporting LP information, or that it is
reporting it does not support autoneg when in fact it is actually
doing autoneg?

	Andrew

Re: [PATCH net-next 3/3] net: phylink: add phylink_sfp_select_interface_speed()

[Alexander Duyck]

On Thu, Jul 10, 2025 at 9:11 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > Settings for enp1s0:
> >         Supported ports: [  ]
> >         Supported link modes:   50000baseCR/Full
> >                                 100000baseCR2/Full
> >         Supported pause frame use: Symmetric Receive-only
> >         Supports auto-negotiation: No
> >         Supported FEC modes: RS
> >         Advertised link modes:  100000baseCR2/Full
> >         Advertised pause frame use: Symmetric Receive-only
> >         Advertised auto-negotiation: No
> >         Advertised FEC modes: RS
> >         Link partner advertised link modes:  100000baseCR2/Full
> >         Link partner advertised pause frame use: No
> >         Link partner advertised auto-negotiation: No
> >         Link partner advertised FEC modes: RS
>
> This all looks suspicious. If it does not support autoneg, how can it
> know what the link partner is advertising?
>
> If you look at what phylib does, for plain old 1G devices:
>
> genphy_read_status() calls genphy_read_lpa()
>
> genphy_read_lpa() then looks to see if autoneg is enabled, and if not
> it does linkmode_zero(phydev->lp_advertising).
>
> So it will not report any link partner information.
>
> What is wrong, that it is reporting LP information, or that it is
> reporting it does not support autoneg when in fact it is actually
> doing autoneg?

I have some debug code on here that is reporting the FW config as the
"LP Advertised". I had borrowed that approach from the phylink
fixedlink config as I thought it was a good way for me to know what
the FW was requesting without having to report it out to a log file.

Re: [PATCH net-next 3/3] net: phylink: add phylink_sfp_select_interface_speed()

[Russell King (Oracle)]

On Thu, Jul 10, 2025 at 10:22:44AM -0700, Alexander Duyck wrote:
> On Thu, Jul 10, 2025 at 9:11 AM Andrew Lunn <andrew@lunn.ch> wrote:
> > What is wrong, that it is reporting LP information, or that it is
> > reporting it does not support autoneg when in fact it is actually
> > doing autoneg?
> 
> I have some debug code on here that is reporting the FW config as the
> "LP Advertised". I had borrowed that approach from the phylink
> fixedlink config as I thought it was a good way for me to know what
> the FW was requesting without having to report it out to a log file.

There are a few points to be made here.

1. Fixed link configuration is not the same as !autoneg setting with
   the presence of a PHY. !autoneg with a PHY present means that the
   PHY has been instructed not to perform autonegotiation, but to set
   the specified parameters for the link and only allow the link to
   operate at the specified speed/duplex. There are exceptions - as
   users expect 1G to work with "autoneg" disabled, and 1G requires
   AN in order to bring the link up. Some PHYs support disabling the
   autoneg function at 1G speed by internally ignoring the request
   to disable autoneg, and instead only advertising to the link
   partner that 1G at the specified duplex is supported. We took
   that and turned it into a software thing for all PHYs as some
   PHYs decided to go a different route - basically not supporting
   the AN enable bit being turned off at 1G speeds.

2. Fixed link configuration is a software concept where there is no
   accessible PHY present. Phylink rejects fixed link configuration
   with a PHY. There is no support to configure a PHY into fixed
   speed/duplex if present, and has never been supported prior to
   phylink.

3. The history. Prior to phylink (and it remains in some cases today),
   fixed link configuration was created by providing a software
   emulated PHY to phylib for the MAC driver to use, thus avoiding
   MAC drivers having to add explicit code for fixed links. They
   looked just like a normal PHY, but was limited to no faster than
   1G speeds as the software emulation is a Clause 22 PHY.

   This software emulated PHY replaces the presence of a physical
   PHY (there is none) and the PHY it emulates looks like a PHY that
   supports AN, has AN enabled, but only supports a single speed
   and duplex, only advertises a single baseT(x) speed and duplex,
   and the link partner agrees on the speed and duplex. This "fools
   phylib into resolving the speed and duplex as per the fixed link
   configuration.

   However, in reality, there is no AN.

   This has become part of the user API, because the MII registers of
   the fixed link PHY were exported to userspace, and of course through
   ethtool.

   There has never been a MII API for reading the fixed link parameters
   for speeds > 1G, so while phylink enables fixed link configuration
   for those speeds, there is no MII register support for this for
   userspace.

(As an aside)
Someone earlier today sent a reminder about a bug I'd introduced for
10GBASE-R, 5GBASE-R and another interface (I don't recall right now)
and I proposed a patch that only cleared the Autoneg bit in the
adertising mask. Having been reminded about it, and had Andrew's
input on this thread, I'm wondering whether config.advertising
should be entirely cleared as in !autoneg mode, the advertising mask
makes no sense.

However, I'm supposed to be on my vacation, so I'm not going to start
testing anything... this email as a bonus that would've otherwise have
been delayed by about two weeks... but the way things are going (family
issues) it could turn out to be a lot longer as I may have to become a
full time carer. So much for an opportunity to have an opportunity to
relax, which I desperately need.

-- 
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 3/3] net: phylink: add phylink_sfp_select_interface_speed()

[Alexander Duyck]

On Thu, Jul 10, 2025 at 11:35 AM Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
>
> On Thu, Jul 10, 2025 at 10:22:44AM -0700, Alexander Duyck wrote:
> > On Thu, Jul 10, 2025 at 9:11 AM Andrew Lunn <andrew@lunn.ch> wrote:
> > > What is wrong, that it is reporting LP information, or that it is
> > > reporting it does not support autoneg when in fact it is actually
> > > doing autoneg?
> >
> > I have some debug code on here that is reporting the FW config as the
> > "LP Advertised". I had borrowed that approach from the phylink
> > fixedlink config as I thought it was a good way for me to know what
> > the FW was requesting without having to report it out to a log file.
>
> There are a few points to be made here.
>
> 1. Fixed link configuration is not the same as !autoneg setting with
>    the presence of a PHY. !autoneg with a PHY present means that the
>    PHY has been instructed not to perform autonegotiation, but to set
>    the specified parameters for the link and only allow the link to
>    operate at the specified speed/duplex. There are exceptions - as
>    users expect 1G to work with "autoneg" disabled, and 1G requires
>    AN in order to bring the link up. Some PHYs support disabling the
>    autoneg function at 1G speed by internally ignoring the request
>    to disable autoneg, and instead only advertising to the link
>    partner that 1G at the specified duplex is supported. We took
>    that and turned it into a software thing for all PHYs as some
>    PHYs decided to go a different route - basically not supporting
>    the AN enable bit being turned off at 1G speeds.
>
> 2. Fixed link configuration is a software concept where there is no
>    accessible PHY present. Phylink rejects fixed link configuration
>    with a PHY. There is no support to configure a PHY into fixed
>    speed/duplex if present, and has never been supported prior to
>    phylink.
>
> 3. The history. Prior to phylink (and it remains in some cases today),
>    fixed link configuration was created by providing a software
>    emulated PHY to phylib for the MAC driver to use, thus avoiding
>    MAC drivers having to add explicit code for fixed links. They
>    looked just like a normal PHY, but was limited to no faster than
>    1G speeds as the software emulation is a Clause 22 PHY.
>
>    This software emulated PHY replaces the presence of a physical
>    PHY (there is none) and the PHY it emulates looks like a PHY that
>    supports AN, has AN enabled, but only supports a single speed
>    and duplex, only advertises a single baseT(x) speed and duplex,
>    and the link partner agrees on the speed and duplex. This "fools
>    phylib into resolving the speed and duplex as per the fixed link
>    configuration.
>
>    However, in reality, there is no AN.
>
>    This has become part of the user API, because the MII registers of
>    the fixed link PHY were exported to userspace, and of course through
>    ethtool.
>
>    There has never been a MII API for reading the fixed link parameters
>    for speeds > 1G, so while phylink enables fixed link configuration
>    for those speeds, there is no MII register support for this for
>    userspace.

So the issue is in our setup we have a SerDes PHY so it isn't a real
MII based PHY and it is being managed by the firmware so we don't have
direct access.

One thought I was playing around with was emulating a 25/50/100G PMA
and AN in software and exposing that as the interface to the FW to
play the role of the PHY. The FW is playing the role of a device tree
configuration in that the EEPROM is pre-programmed with the expected
speed/lane/FEC configuration and the FW is reading that to determine
what the link configuration should be as we cannot use autoneg as the
switch port on the other side doesn't support it. For our production
use case we will always be using that speed, but for testing we will
need to support the ability to set manual speeds.

> (As an aside)
> Someone earlier today sent a reminder about a bug I'd introduced for
> 10GBASE-R, 5GBASE-R and another interface (I don't recall right now)
> and I proposed a patch that only cleared the Autoneg bit in the
> adertising mask. Having been reminded about it, and had Andrew's
> input on this thread, I'm wondering whether config.advertising
> should be entirely cleared as in !autoneg mode, the advertising mask
> makes no sense.
>
> However, I'm supposed to be on my vacation, so I'm not going to start
> testing anything... this email as a bonus that would've otherwise have
> been delayed by about two weeks... but the way things are going (family
> issues) it could turn out to be a lot longer as I may have to become a
> full time carer. So much for an opportunity to have an opportunity to
> relax, which I desperately need.

Enjoy your vacation. It will probably take me a while to try to work
out an acceptable solution for how to deal with the buried/hidden PHY
behind the firmware anyway. For now this thread has become more about
fbnic anyway then your original patch since we have more-or-less
verified it works as expected.

Re: [PATCH net-next 0/3] net: phylink: support !autoneg configuration for SFPs

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 2 Jul 2025 10:44:38 +0100 you wrote:
> This series comes from discussion during a patch series that was posted
> at the beginning of April, but these patches were never posted (I was
> too busy!)
> 
> We restrict ->sfp_interfaces to those that the host system supports,
> and ensure that ->sfp_interfaces is cleared when a SFP is removed. We
> then add phylink_sfp_select_interface_speed() which will select an
> appropriate interface from ->sfp_interfaces for the speed, and use that
> in our phylink_ethtool_ksettings_set() when a SFP bus is present on a
> directly connected host (not with a PHY.)
> 
> [...]

Here is the summary with links:
  - [net-next,1/3] net: phylink: restrict SFP interfaces to those that are supported
    https://git.kernel.org/netdev/net-next/c/ff1fce1bdd7b
  - [net-next,2/3] net: phylink: clear SFP interfaces when not in use
    https://git.kernel.org/netdev/net-next/c/b0fdff22d520
  - [net-next,3/3] net: phylink: add phylink_sfp_select_interface_speed()
    https://git.kernel.org/netdev/net-next/c/320164a6e172

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