Pause, Asym_Pause and Autoneg bits are not set when pl->supported is
initialized, so these link modes will not work for the fixed-link. This
leads to a TCP performance degradation issue observed on the i.MX943
platform.

The switch CPU port of i.MX943 is connected to an ENETC MAC, this link
is a fixed link and the link speed is 2.5Gbps. And one of the switch
user ports is the RGMII interface, and its link speed is 1Gbps. If the
flow-control of the fixed link is not enabled, we can easily observe
the iperf performance of TCP packets is very low. Because the inbound
rate on the CPU port is greater than the outbound rate on the user port,
the switch is prone to congestion, leading to the loss of some TCP
packets and requiring multiple retransmissions.

Solving this problem should be as simple as setting the Asym_Pause and
Pause bits. The reason why the Autoneg bit needs to be set is because
it was already set before the blame commit. Moreover, Russell provides
a very good explanation of why it needs to be set in the thread [1].

[1] https://lore.kernel.org/all/aRjqLN8eQDIQfBjS@shell.armlinux.org.uk/

Fixes: de7d3f87be3c ("net: phylink: Use phy_caps_lookup for fixed-link configuration")
Signed-off-by: Wei Fang <wei.fang@nxp.com>
Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
v2 changes:
1. Improve the commit message
2. Collect the Reviewed-by tag
v1: https://lore.kernel.org/imx/20251114052808.1129942-1-wei.fang@nxp.com/
---
 drivers/net/phy/phylink.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 9d7799ea1c17..918244308215 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -637,6 +637,9 @@ static int phylink_validate(struct phylink *pl, unsigned long *supported,
 
 static void phylink_fill_fixedlink_supported(unsigned long *supported)
 {
+	linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT, supported);
+	linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, supported);
+	linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, supported);
 	linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT, supported);
 	linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT, supported);
 	linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT, supported);
-- 
2.34.1

On Sun, Nov 16, 2025 at 10:38:23AM +0800, Wei Fang wrote:
> Pause, Asym_Pause and Autoneg bits are not set when pl->supported is
> initialized, so these link modes will not work for the fixed-link. This
> leads to a TCP performance degradation issue observed on the i.MX943
> platform.
> 
> The switch CPU port of i.MX943 is connected to an ENETC MAC, this link
> is a fixed link and the link speed is 2.5Gbps. And one of the switch
> user ports is the RGMII interface, and its link speed is 1Gbps. If the
> flow-control of the fixed link is not enabled, we can easily observe
> the iperf performance of TCP packets is very low. Because the inbound
> rate on the CPU port is greater than the outbound rate on the user port,
> the switch is prone to congestion, leading to the loss of some TCP
> packets and requiring multiple retransmissions.
> 
> Solving this problem should be as simple as setting the Asym_Pause and
> Pause bits. The reason why the Autoneg bit needs to be set is because
> it was already set before the blame commit. Moreover, Russell provides
> a very good explanation of why it needs to be set in the thread [1].
> 
> [1] https://lore.kernel.org/all/aRjqLN8eQDIQfBjS@shell.armlinux.org.uk/
> 
> Fixes: de7d3f87be3c ("net: phylink: Use phy_caps_lookup for fixed-link configuration")
> Signed-off-by: Wei Fang <wei.fang@nxp.com>
> Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Even though discussion is still going on, we do need to fix this
regression. So:

Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Thanks!

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

Hi,

On 17/11/2025 16:17, Russell King (Oracle) wrote:
> On Sun, Nov 16, 2025 at 10:38:23AM +0800, Wei Fang wrote:
>> Pause, Asym_Pause and Autoneg bits are not set when pl->supported is
>> initialized, so these link modes will not work for the fixed-link. This
>> leads to a TCP performance degradation issue observed on the i.MX943
>> platform.
>>
>> The switch CPU port of i.MX943 is connected to an ENETC MAC, this link
>> is a fixed link and the link speed is 2.5Gbps. And one of the switch
>> user ports is the RGMII interface, and its link speed is 1Gbps. If the
>> flow-control of the fixed link is not enabled, we can easily observe
>> the iperf performance of TCP packets is very low. Because the inbound
>> rate on the CPU port is greater than the outbound rate on the user port,
>> the switch is prone to congestion, leading to the loss of some TCP
>> packets and requiring multiple retransmissions.
>>
>> Solving this problem should be as simple as setting the Asym_Pause and
>> Pause bits. The reason why the Autoneg bit needs to be set is because
>> it was already set before the blame commit. Moreover, Russell provides
>> a very good explanation of why it needs to be set in the thread [1].
>>
>> [1] https://lore.kernel.org/all/aRjqLN8eQDIQfBjS@shell.armlinux.org.uk/
>>
>> Fixes: de7d3f87be3c ("net: phylink: Use phy_caps_lookup for fixed-link configuration")
>> Signed-off-by: Wei Fang <wei.fang@nxp.com>
>> Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> 
> Even though discussion is still going on, we do need to fix this
> regression. So:
> 
> Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

I already reviewed the patch, but I agree with Russell. Being the author
of the blamed commit, I can say that this change in behavior was not
intentional :(

Maxime

On Sun, Nov 16, 2025 at 10:38:23AM +0800, Wei Fang wrote:
> Pause, Asym_Pause and Autoneg bits are not set when pl->supported is
> initialized, so these link modes will not work for the fixed-link. This
> leads to a TCP performance degradation issue observed on the i.MX943
> platform.
> 
> The switch CPU port of i.MX943 is connected to an ENETC MAC, this link
> is a fixed link and the link speed is 2.5Gbps. And one of the switch
> user ports is the RGMII interface, and its link speed is 1Gbps. If the
> flow-control of the fixed link is not enabled, we can easily observe
> the iperf performance of TCP packets is very low. Because the inbound
> rate on the CPU port is greater than the outbound rate on the user port,
> the switch is prone to congestion, leading to the loss of some TCP
> packets and requiring multiple retransmissions.
> 
> Solving this problem should be as simple as setting the Asym_Pause and
> Pause bits. The reason why the Autoneg bit needs to be set is because
> it was already set before the blame commit. Moreover, Russell provides
> a very good explanation of why it needs to be set in the thread [1].
> 
> [1] https://lore.kernel.org/all/aRjqLN8eQDIQfBjS@shell.armlinux.org.uk/

There is no limit on commit message length, but URLs sometimes
die. Please just make use of Russells explanation. You can say: As
explained by Russell King, and just quote it, etc.

This also seems like two fixes: a regression for the AUTONEG bit, and
allowing pause to be set. So maybe this should be two patches?

I'm also surprised TCP is collapsing. This is not an unusual setup,
e.g. a home wireless network feeding a cable modem. A high speed link
feeding a lower speed link. What RTT is there when TCP gets into
trouble? TCP should be backing off as soon as it sees packet loss, so
reducing the bandwidth it tries to consume, and so emptying out the
buffers. But if you have big buffers in the ENETC causing high
latency, that might be an issue?  Does ENETC have BQL? It is worth
implementing, just to avoid bufferbloat problems.

	Andrew

---
pw-bot: cr

On Sun, Nov 16, 2025 at 05:10:19PM +0100, Andrew Lunn wrote:
> There is no limit on commit message length, but URLs sometimes
> die. Please just make use of Russells explanation. You can say: As
> explained by Russell King, and just quote it, etc.

I think one of the reasons lore exists is to provide a stable and
reliable source of URLs into archives - it's maintained by the
kernel.org folk.

> This also seems like two fixes: a regression for the AUTONEG bit, and
> allowing pause to be set. So maybe this should be two patches?

The blamed commit caused both to change.

The old code:

	linkmode_fill(pl->supported);
	linkmode_copy(pl->link_config.advertising, pl->supported);
	phylink_validate(pl, pl->supported, &pl->link_config);

This results in pl->supported and pl->link_config.advertising being
the fullest capabilities that the MAC supports.

	s = phy_lookup_setting(pl->link_config.speed, pl->link_config.duplex,
                               pl->supported, true);

This gets the linkmode bit corresponding to the speed/duplex. We then
construct a mask:

	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
	...
	linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT, mask);
	linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, mask);
	linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, mask);

which ends up passing through just these three bits, provided the MAC
supports them:

	linkmode_and(pl->supported, pl->supported, mask);

We ensure that a port type is set:

	phylink_set(pl->supported, MII);

and then we explicitly set a single bit for the speed/duplex:

	if (s) {
		__set_bit(s->bit, pl->supported);
		__set_bit(s->bit, pl->link_config.lp_advertising);
	} else ...

lp_advertising doesn't make sense without Autoneg bit set.

After the blamed commit, rather than pl->supported being filled
prior to the phylink_Validate() call, it is now cleared and the
baseT modes are populated. Everything else is clear. This is
incorrect.

So, I think fixing this breakage in a single patch is justified.
It is restoring the old behaviour which we've had for a long time.
It isn't two bugs, it's one mistake.

> I'm also surprised TCP is collapsing. This is not an unusual setup,
> e.g. a home wireless network feeding a cable modem. A high speed link
> feeding a lower speed link. What RTT is there when TCP gets into
> trouble? TCP should be backing off as soon as it sees packet loss, so
> reducing the bandwidth it tries to consume, and so emptying out the
> buffers. But if you have big buffers in the ENETC causing high
> latency, that might be an issue?  Does ENETC have BQL? It is worth
> implementing, just to avoid bufferbloat problems.

I'd also suggest further evaluating the performance of other ports
when flow control is enabled when one port gets overwhelmed with
packets (and thus the switch starts sending pause frames to the
host port, slowing _all_ traffic to _all_ ports.)

Depending on the application, it may be better to let the congested
port drop packets (Ethernet was designed to drop packets after all)
while allowing other ports to operate, rather than throttling all
ports.

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