[PATCH net-next] net: phylink: Support disabling autonegotiation for PCS

[PATCH net-next] net: phylink: Support disabling autonegotiation for PCS

[Robert Hancock]

The auto-negotiation state in the PCS as set by
phylink_mii_c22_pcs_config was previously always enabled when the driver is
configured for in-band autonegotiation, even if autonegotiation was
disabled on the interface with ethtool. Update the code to set the
BMCR_ANENABLE bit based on the interface's autonegotiation enabled
state.

Update phylink_mii_c22_pcs_get_state to not check
autonegotiation-related fields when autonegotiation is disabled.

Update phylink_mac_pcs_get_state to initialize the state based on the
interface's configured speed, duplex and pause parameters rather than to
unknown when autonegotiation is disabled, before calling the driver's
pcs_get_state functions, as they are not likely to provide meaningful data
for these fields when autonegotiation is disabled. In this case the
driver is really just filling in the link state field.

Note that in cases where there is a downstream PHY connected, such as
with SGMII and a copper PHY, the configuration set by ethtool is handled by
phy_ethtool_ksettings_set and not propagated to the PCS. This is correct
since SGMII or 1000Base-X autonegotiation with the PCS should normally
still be used even if the copper side has disabled it.

Signed-off-by: Robert Hancock <robert.hancock@calian.com>
---
 drivers/net/phy/phylink.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index eb29ef53d971..4fc07d92f0c6 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -539,9 +539,15 @@ static void phylink_mac_pcs_get_state(struct phylink *pl,
 	linkmode_zero(state->lp_advertising);
 	state->interface = pl->link_config.interface;
 	state->an_enabled = pl->link_config.an_enabled;
-	state->speed = SPEED_UNKNOWN;
-	state->duplex = DUPLEX_UNKNOWN;
-	state->pause = MLO_PAUSE_NONE;
+	if  (state->an_enabled) {
+		state->speed = SPEED_UNKNOWN;
+		state->duplex = DUPLEX_UNKNOWN;
+		state->pause = MLO_PAUSE_NONE;
+	} else {
+		state->speed =  pl->link_config.speed;
+		state->duplex = pl->link_config.duplex;
+		state->pause = pl->link_config.pause;
+	}
 	state->an_complete = 0;
 	state->link = 1;
 
@@ -2422,7 +2428,10 @@ void phylink_mii_c22_pcs_get_state(struct mdio_device *pcs,
 
 	state->link = !!(bmsr & BMSR_LSTATUS);
 	state->an_complete = !!(bmsr & BMSR_ANEGCOMPLETE);
-	if (!state->link)
+	/* If there is no link or autonegotiation is disabled, the LP advertisement
+	 * data is not meaningful, so don't go any further.
+	 */
+	if (!state->link || !state->an_enabled)
 		return;
 
 	switch (state->interface) {
@@ -2545,7 +2554,9 @@ int phylink_mii_c22_pcs_config(struct mdio_device *pcs, unsigned int mode,
 	changed = ret > 0;
 
 	/* Ensure ISOLATE bit is disabled */
-	bmcr = mode == MLO_AN_INBAND ? BMCR_ANENABLE : 0;
+	bmcr = (mode == MLO_AN_INBAND &&
+		linkmode_test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, advertising)) ?
+		       BMCR_ANENABLE : 0;
 	ret = mdiobus_modify(pcs->bus, pcs->addr, MII_BMCR,
 			     BMCR_ANENABLE | BMCR_ISOLATE, bmcr);
 	if (ret < 0)
-- 
2.27.0

Re: [PATCH net-next] net: phylink: Support disabling autonegotiation for PCS

[Russell King (Oracle)]

On Wed, Jun 30, 2021 at 11:49:27AM -0600, Robert Hancock wrote:
> The auto-negotiation state in the PCS as set by
> phylink_mii_c22_pcs_config was previously always enabled when the driver is
> configured for in-band autonegotiation, even if autonegotiation was
> disabled on the interface with ethtool. Update the code to set the
> BMCR_ANENABLE bit based on the interface's autonegotiation enabled
> state.
> 
> Update phylink_mii_c22_pcs_get_state to not check
> autonegotiation-related fields when autonegotiation is disabled.
> 
> Update phylink_mac_pcs_get_state to initialize the state based on the
> interface's configured speed, duplex and pause parameters rather than to
> unknown when autonegotiation is disabled, before calling the driver's
> pcs_get_state functions, as they are not likely to provide meaningful data
> for these fields when autonegotiation is disabled. In this case the
> driver is really just filling in the link state field.
> 
> Note that in cases where there is a downstream PHY connected, such as
> with SGMII and a copper PHY, the configuration set by ethtool is handled by
> phy_ethtool_ksettings_set and not propagated to the PCS. This is correct
> since SGMII or 1000Base-X autonegotiation with the PCS should normally
> still be used even if the copper side has disabled it.
> 
> Signed-off-by: Robert Hancock <robert.hancock@calian.com>

I definitely want to think about this _before_ it gets applied to
net-next. It's a substantial change, not a "fix" or something simple.

-- 
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-next] net: phylink: Support disabling autonegotiation for PCS

[Russell King (Oracle)]

On Wed, Jun 30, 2021 at 11:49:27AM -0600, Robert Hancock wrote:
> The auto-negotiation state in the PCS as set by
> phylink_mii_c22_pcs_config was previously always enabled when the driver is
> configured for in-band autonegotiation, even if autonegotiation was
> disabled on the interface with ethtool. Update the code to set the
> BMCR_ANENABLE bit based on the interface's autonegotiation enabled
> state.
> 
> Update phylink_mii_c22_pcs_get_state to not check
> autonegotiation-related fields when autonegotiation is disabled.
> 
> Update phylink_mac_pcs_get_state to initialize the state based on the
> interface's configured speed, duplex and pause parameters rather than to
> unknown when autonegotiation is disabled, before calling the driver's
> pcs_get_state functions, as they are not likely to provide meaningful data
> for these fields when autonegotiation is disabled. In this case the
> driver is really just filling in the link state field.
> 
> Note that in cases where there is a downstream PHY connected, such as
> with SGMII and a copper PHY, the configuration set by ethtool is handled by
> phy_ethtool_ksettings_set and not propagated to the PCS. This is correct
> since SGMII or 1000Base-X autonegotiation with the PCS should normally
> still be used even if the copper side has disabled it.

In theory, this seems to be correct, but...

We do have some cases where, if a port is in 1000Base-X mode, the
documentation explicitly states that AN must be enabled. So, I think
if we are introducing the possibility to disable the negotiation in
1000Base-X mode, we need to give an option to explicitly reject that
configuration attempt.

We also need this to be consistently applied over all the existing
phylink-using drivers that support 1000Base-X without AN - we shouldn't
end up in the situation where we have different behaviours with
different network drivers.

So, we need mvneta and mvpp2 to reject such a configuration - with
these ports in 1000Base-X mode, the documentation states:

"Bit 2 Field InBandAnEn In-band Auto-Negotiation enable. ...
When <PortType> = 1 (1000BASE-X) this field must be set to 1."

We should be aware that there may be other hardware out there which
may not support 1000BASE-X without inband.

-- 
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-next] net: phylink: Support disabling autonegotiation for PCS

[Russell King - ARM Linux admin]

On Thu, Jul 01, 2021 at 03:52:22PM +0100, Russell King (Oracle) wrote:
> On Wed, Jun 30, 2021 at 11:49:27AM -0600, Robert Hancock wrote:
> > The auto-negotiation state in the PCS as set by
> > phylink_mii_c22_pcs_config was previously always enabled when the driver is
> > configured for in-band autonegotiation, even if autonegotiation was
> > disabled on the interface with ethtool. Update the code to set the
> > BMCR_ANENABLE bit based on the interface's autonegotiation enabled
> > state.
> > 
> > Update phylink_mii_c22_pcs_get_state to not check
> > autonegotiation-related fields when autonegotiation is disabled.
> > 
> > Update phylink_mac_pcs_get_state to initialize the state based on the
> > interface's configured speed, duplex and pause parameters rather than to
> > unknown when autonegotiation is disabled, before calling the driver's
> > pcs_get_state functions, as they are not likely to provide meaningful data
> > for these fields when autonegotiation is disabled. In this case the
> > driver is really just filling in the link state field.
> > 
> > Note that in cases where there is a downstream PHY connected, such as
> > with SGMII and a copper PHY, the configuration set by ethtool is handled by
> > phy_ethtool_ksettings_set and not propagated to the PCS. This is correct
> > since SGMII or 1000Base-X autonegotiation with the PCS should normally
> > still be used even if the copper side has disabled it.
> 
> In theory, this seems to be correct, but...
> 
> We do have some cases where, if a port is in 1000Base-X mode, the
> documentation explicitly states that AN must be enabled. So, I think
> if we are introducing the possibility to disable the negotiation in
> 1000Base-X mode, we need to give an option to explicitly reject that
> configuration attempt.
> 
> We also need this to be consistently applied over all the existing
> phylink-using drivers that support 1000Base-X without AN - we shouldn't
> end up in the situation where we have different behaviours with
> different network drivers.
> 
> So, we need mvneta and mvpp2 to reject such a configuration - with
> these ports in 1000Base-X mode, the documentation states:
> 
> "Bit 2 Field InBandAnEn In-band Auto-Negotiation enable. ...
> When <PortType> = 1 (1000BASE-X) this field must be set to 1."
> 
> We should be aware that there may be other hardware out there which
> may not support 1000BASE-X without inband.

Incidentally, this also means that when we're in 2500BASE-X mode on
mvneta and mvpp2, PortType is 1, and we must use autonegotiation.

I think we _really_ need to have a better discussion about the
presence of AN or not with 2500BASE-X as far as the kernel is concerned
because we have ended up in the situation where mvneta and mvpp2 always
enable it (through need) for 1000BASE-X and 2500BASE-X, whereas others
always disable it in 2500BASE-X. Meanwhile, Xilinx allows it to be
configured. We seem to have headed into a situation where different
SoCs from different manufacturers disagree on whether 2500BASE-X does
negotiation, and thus we've ended up with different kernel behaviours.
This is not sane.

-- 
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-next] net: phylink: Support disabling autonegotiation for PCS

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (master)
by David S. Miller <davem@davemloft.net>:

On Tue, 19 Oct 2021 11:24:50 +0100 you wrote:
> From: Robert Hancock <robert.hancock@calian.com>
> 
> The auto-negotiation state in the PCS as set by
> phylink_mii_c22_pcs_config was previously always enabled when the
> driver is configured for in-band autonegotiation, even if
> autonegotiation was disabled on the interface with ethtool. Update the
> code to set the BMCR_ANENABLE bit based on the interface's
> autonegotiation enabled state.
> 
> [...]

Here is the summary with links:
  - [net-next] net: phylink: Support disabling autonegotiation for PCS
    https://git.kernel.org/netdev/net-next/c/92817dad7dcb

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

[PATCH net-next] net: phylink: Support disabling autonegotiation for PCS

[Russell King]

From: Robert Hancock <robert.hancock@calian.com>

The auto-negotiation state in the PCS as set by
phylink_mii_c22_pcs_config was previously always enabled when the
driver is configured for in-band autonegotiation, even if
autonegotiation was disabled on the interface with ethtool. Update the
code to set the BMCR_ANENABLE bit based on the interface's
autonegotiation enabled state.

Update phylink_mii_c22_pcs_get_state to not check
autonegotiation-related fields when autonegotiation is disabled.

Update phylink_mac_pcs_get_state to initialize the state based on the
interface's configured speed, duplex and pause parameters rather than
to unknown when autonegotiation is disabled, before calling the
driver's pcs_get_state functions, as they are not likely to provide
meaningful data for these fields when autonegotiation is disabled. In
this case the driver is really just filling in the link state field.

Note that in cases where there is a downstream PHY connected, such as
with SGMII and a copper PHY, the configuration set by ethtool is
handled by phy_ethtool_ksettings_set and not propagated to the PCS.
This is correct since SGMII or 1000Base-X autonegotiation with the PCS
should normally still be used even if the copper side has disabled it.

Signed-off-by: Robert Hancock <robert.hancock@calian.com>
Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phylink.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index f6e848f1181c..5f0ecb43029f 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -552,9 +552,15 @@ static void phylink_mac_pcs_get_state(struct phylink *pl,
 	linkmode_zero(state->lp_advertising);
 	state->interface = pl->link_config.interface;
 	state->an_enabled = pl->link_config.an_enabled;
-	state->speed = SPEED_UNKNOWN;
-	state->duplex = DUPLEX_UNKNOWN;
-	state->pause = MLO_PAUSE_NONE;
+	if  (state->an_enabled) {
+		state->speed = SPEED_UNKNOWN;
+		state->duplex = DUPLEX_UNKNOWN;
+		state->pause = MLO_PAUSE_NONE;
+	} else {
+		state->speed =  pl->link_config.speed;
+		state->duplex = pl->link_config.duplex;
+		state->pause = pl->link_config.pause;
+	}
 	state->an_complete = 0;
 	state->link = 1;
 
@@ -2548,7 +2554,10 @@ void phylink_mii_c22_pcs_get_state(struct mdio_device *pcs,
 
 	state->link = !!(bmsr & BMSR_LSTATUS);
 	state->an_complete = !!(bmsr & BMSR_ANEGCOMPLETE);
-	if (!state->link)
+	/* If there is no link or autonegotiation is disabled, the LP advertisement
+	 * data is not meaningful, so don't go any further.
+	 */
+	if (!state->link || !state->an_enabled)
 		return;
 
 	switch (state->interface) {
@@ -2650,7 +2659,12 @@ int phylink_mii_c22_pcs_config(struct mdio_device *pcs, unsigned int mode,
 	changed = ret > 0;
 
 	/* Ensure ISOLATE bit is disabled */
-	bmcr = mode == MLO_AN_INBAND ? BMCR_ANENABLE : 0;
+	if (mode == MLO_AN_INBAND &&
+	    linkmode_test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, advertising))
+		bmcr = BMCR_ANENABLE;
+	else
+		bmcr = 0;
+
 	ret = mdiobus_modify(pcs->bus, pcs->addr, MII_BMCR,
 			     BMCR_ANENABLE | BMCR_ISOLATE, bmcr);
 	if (ret < 0)
-- 
2.30.2