[PATCH] net: phylink: Update SFP selected interface on advertising changes

[PATCH] net: phylink: Update SFP selected interface on advertising changes

[Nathan Rossi]

From: Nathan Rossi <nathan.rossi@digi.com>

Currently changes to the advertising state via ethtool do not cause any
reselection of the configured interface mode after the SFP is already
inserted and initially configured.

While it is not typical to change the advertised link modes for an
interface using an SFP in certain use cases it is desirable. In the case
of a SFP port that is capable of handling both SFP and SFP+ modules it
will automatically select between 1G and 10G modes depending on the
supported mode of the SFP. However if the SFP module is capable of
working in multiple modes (e.g. a SFP+ DAC that can operate at 1G or
10G), one end of the cable may be attached to a SFP 1000base-x port thus
the SFP+ end must be manually configured to the 1000base-x mode in order
for the link to be established.

This change causes the ethtool setting of advertised mode changes to
reselect the interface mode so that the link can be established.

Signed-off-by: Nathan Rossi <nathan.rossi@digi.com>
---
 drivers/net/phy/phylink.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 2cdf9f989d..8986c7a0c5 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1525,6 +1525,22 @@ int phylink_ethtool_ksettings_set(struct phylink *pl,
 	if (config.an_enabled && phylink_is_empty_linkmode(config.advertising))
 		return -EINVAL;
 
+	/* If this link is with an SFP, ensure that changes to advertised modes
+	 * also cause the associated interface to be selected such that the
+	 * link can be configured correctly.
+	 */
+	if (pl->sfp_port && pl->sfp_bus) {
+		config.interface = sfp_select_interface(pl->sfp_bus,
+							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);
+			return -EINVAL;
+		}
+	}
+
 	mutex_lock(&pl->state_mutex);
 	pl->link_config.speed = config.speed;
 	pl->link_config.duplex = config.duplex;
---
2.33.0

Re: [PATCH] net: phylink: Update SFP selected interface on advertising changes

[Russell King (Oracle)]

On Thu, Aug 26, 2021 at 07:12:30AM +0000, Nathan Rossi wrote:
> From: Nathan Rossi <nathan.rossi@digi.com>
> 
> Currently changes to the advertising state via ethtool do not cause any
> reselection of the configured interface mode after the SFP is already
> inserted and initially configured.
> 
> While it is not typical to change the advertised link modes for an
> interface using an SFP in certain use cases it is desirable. In the case
> of a SFP port that is capable of handling both SFP and SFP+ modules it
> will automatically select between 1G and 10G modes depending on the
> supported mode of the SFP. However if the SFP module is capable of
> working in multiple modes (e.g. a SFP+ DAC that can operate at 1G or
> 10G), one end of the cable may be attached to a SFP 1000base-x port thus
> the SFP+ end must be manually configured to the 1000base-x mode in order
> for the link to be established.
> 
> This change causes the ethtool setting of advertised mode changes to
> reselect the interface mode so that the link can be established.

This may be a better solution than the phylink_helper_basex_speed()
approach used to select between 2500 and 1000 base-X, but the config
needs to be validated after selecting a different interface mode, as
per what phylink_sfp_config() does.

I also suspect that this will allow you to select e.g. 1000base-X but
there will be no way back to 10G modes as they will be masked out of
the advertising mask from that point on, as the 1000base-X interface
mode does not allow them.

So, I think the suggested code is problematical as it stands.

phylink_sfp_config() uses a multi-step approach to selecting the
interface mode for a reason - first step is to discover what the MAC
is capable of in _any_ interface mode using _NA to reduce the supported
and advertised mask down, and then to select the interface from that.
I'm not entirely convinced that is a good idea here yet though.

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

Re: [PATCH] net: phylink: Update SFP selected interface on advertising changes

[Nathan Rossi]

On Thu, 26 Aug 2021 at 19:25, Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
>
> On Thu, Aug 26, 2021 at 07:12:30AM +0000, Nathan Rossi wrote:
> > From: Nathan Rossi <nathan.rossi@digi.com>
> >
> > Currently changes to the advertising state via ethtool do not cause any
> > reselection of the configured interface mode after the SFP is already
> > inserted and initially configured.
> >
> > While it is not typical to change the advertised link modes for an
> > interface using an SFP in certain use cases it is desirable. In the case
> > of a SFP port that is capable of handling both SFP and SFP+ modules it
> > will automatically select between 1G and 10G modes depending on the
> > supported mode of the SFP. However if the SFP module is capable of
> > working in multiple modes (e.g. a SFP+ DAC that can operate at 1G or
> > 10G), one end of the cable may be attached to a SFP 1000base-x port thus
> > the SFP+ end must be manually configured to the 1000base-x mode in order
> > for the link to be established.
> >
> > This change causes the ethtool setting of advertised mode changes to
> > reselect the interface mode so that the link can be established.
>
> This may be a better solution than the phylink_helper_basex_speed()
> approach used to select between 2500 and 1000 base-X, but the config
> needs to be validated after selecting a different interface mode, as
> per what phylink_sfp_config() does.

I will add this in a v2. But will wait to get your feedback on the below before
sending that out.

>
> I also suspect that this will allow you to select e.g. 1000base-X but
> there will be no way back to 10G modes as they will be masked out of
> the advertising mask from that point on, as the 1000base-X interface
> mode does not allow them.

Because only the advertising bitmap is changed it is possible to revert any
changes because the supported bitmap is not affected. Although I may be
misunderstanding the exact details of the issue you are describing?

Note, the phylink_sfp_config phylink_validate call after sfp_select_interface
does not modify the supported bitmap so adding that validate call in this change
will not affect the ability to reselect changed advertised bits.

I did some additional testing and noticed that the advertising mask is not
updated in phylink_sfp_config if supported does not change (e.g. SFP reinsert),
which leads to the advertising state mismatching (e.g. advertising at 1G, but
actually operating at 10G). This just needs to check if the advertising also
mismatches in phylink_sfp_config.

Thanks,
Nathan

>
> So, I think the suggested code is problematical as it stands.
>
> phylink_sfp_config() uses a multi-step approach to selecting the
> interface mode for a reason - first step is to discover what the MAC
> is capable of in _any_ interface mode using _NA to reduce the supported
> and advertised mask down, and then to select the interface from that.
> I'm not entirely convinced that is a good idea here yet though.
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

[PATCH v2] net: phylink: Update SFP selected interface on advertising changes

[Nathan Rossi]

From: Nathan Rossi <nathan.rossi@digi.com>

Currently changes to the advertising state via ethtool do not cause any
reselection of the configured interface mode after the SFP is already
inserted and initially configured.

While it is not typical to change the advertised link modes for an
interface using an SFP in certain use cases it is desirable. In the case
of a SFP port that is capable of handling both SFP and SFP+ modules it
will automatically select between 1G and 10G modes depending on the
supported mode of the SFP. However if the SFP module is capable of
working in multiple modes (e.g. a SFP+ DAC that can operate at 1G or
10G), one end of the cable may be attached to a SFP 1000base-x port thus
the SFP+ end must be manually configured to the 1000base-x mode in order
for the link to be established.

This change causes the ethtool setting of advertised mode changes to
reselect the interface mode so that the link can be established.
Additionally when a module is inserted the advertising mode is reset to
match the supported modes of the module.

Signed-off-by: Nathan Rossi <nathan.rossi@digi.com>
---
Changes in v2:
- Revalidate the configuration after the interface mode change (as done
  in phylink_sfp_config).
- In phylink_sfp_config, update the current config advertising state
  when it differs. This makes sure that the advertising state matches
  what is applied during module insert/start.
---
 drivers/net/phy/phylink.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 2cdf9f989d..ea4bdc9cf5 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1525,6 +1525,32 @@ int phylink_ethtool_ksettings_set(struct phylink *pl,
 	if (config.an_enabled && phylink_is_empty_linkmode(config.advertising))
 		return -EINVAL;
 
+	/* If this link is with an SFP, ensure that changes to advertised modes
+	 * also cause the associated interface to be selected such that the
+	 * link can be configured correctly.
+	 */
+	if (pl->sfp_port && pl->sfp_bus) {
+		config.interface = sfp_select_interface(pl->sfp_bus,
+							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);
+			return -EINVAL;
+		}
+
+		/* Revalidate with the selected interface */
+		linkmode_copy(support, pl->supported);
+		if (phylink_validate(pl, support, &config)) {
+			phylink_err(pl, "validation of %s/%s with support %*pb failed\n",
+				    phylink_an_mode_str(pl->cur_link_an_mode),
+				    phy_modes(config.interface),
+				    __ETHTOOL_LINK_MODE_MASK_NBITS, support);
+			return -EINVAL;
+		}
+	}
+
 	mutex_lock(&pl->state_mutex);
 	pl->link_config.speed = config.speed;
 	pl->link_config.duplex = config.duplex;
@@ -2104,7 +2130,9 @@ static int phylink_sfp_config(struct phylink *pl, u8 mode,
 	if (phy_interface_mode_is_8023z(iface) && pl->phydev)
 		return -EINVAL;
 
-	changed = !linkmode_equal(pl->supported, support);
+	changed = !linkmode_equal(pl->supported, support) ||
+		  !linkmode_equal(pl->link_config.advertising,
+				  config.advertising);
 	if (changed) {
 		linkmode_copy(pl->supported, support);
 		linkmode_copy(pl->link_config.advertising, config.advertising);
---
2.33.0