net: phylink: add missing supported link modes for the fixed-link

PATCH Superseded Wei Fang 4 participants 9 messages Nov 14, 2025 – Nov 15, 2025

Subsystems (5)

Networking Network drivers Arm i.MX and Layerscape Ethernet PHY +1 more subsystems

lore

Share & tools

lore

Wei Fang

author

2025-11-14 05:28 UTC #0 lore raw

PATCH 1/1 net: phylink: add missing supported link modes for the fixed-link

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.

Fixes de7d3f87be3c ("net: phylink: Use phy_caps_lookup for fixed-link configuration")

Signed-off-by Wei Fang <[email protected]>

---
 drivers/net/phy/phylink.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/net/phy/phylink.c +3 -0

index 9d7799ea1c17..918244308215 100644
@@ -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);

Signature · 1 line

2.34.1

Russell King (Oracle)

maintainer

2025-11-14 08:32 UTC #1 lore raw

Quoted 2 lines · Wei Fang

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.

What problem does this cause?

Signature · 2 lines

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

Wei Fang

author

2025-11-14 08:52 UTC #2 lore raw

Quoted 6 lines · Wei Fang

Quoted 2 lines
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.
What problem does this cause?

Our i.MX943 platform, the switch CPU port is connected to an ENETC MAC, the link speed is 2.5Gbps. And one user port of switch is 1Gbps. If the flow-control of internal link is not enabled, we can see the iperf performance of TCP packets is very low. Because of the congestion (ingress speed of CPU port is greater than egress speed of user port) in the switch, some TCP packets were dropped, resulting in the packets being retransmitted many times.

root@imx943evk:~# iperf3 -c 192.168.1.2%swp2
Connecting to host 192.168.1.2, port 5201
[  5] local 192.168.1.11 port 36558 connected to 192.168.1.2 port 5201
[ ID] Interval           Transfer     Bitrate         Retr  Cwnd
[  5]   0.00-1.00   sec  51.2 MBytes   429 Mbits/sec  379   24.0 KBytes
[  5]   1.00-2.00   sec  50.2 MBytes   422 Mbits/sec  401   24.0 KBytes
[  5]   2.00-3.00   sec  50.0 MBytes   419 Mbits/sec  405   24.0 KBytes
[  5]   3.00-4.00   sec  41.4 MBytes   347 Mbits/sec  395   24.0 KBytes
[  5]   4.00-5.00   sec  43.9 MBytes   368 Mbits/sec  368   21.2 KBytes
[  5]   5.00-6.00   sec  42.8 MBytes   359 Mbits/sec  346   24.0 KBytes
[  5]   6.00-7.00   sec  35.4 MBytes   297 Mbits/sec  317   21.2 KBytes
[  5]   7.00-8.00   sec  54.4 MBytes   456 Mbits/sec  389   26.9 KBytes
[  5]   8.00-9.00   sec  45.4 MBytes   381 Mbits/sec  359   22.6 KBytes
[  5]   9.00-10.00  sec  47.6 MBytes   399 Mbits/sec  350   21.2 KBytes

- - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval Transfer Bitrate Retr

[  5]   0.00-10.00  sec   462 MBytes   388 Mbits/sec  3709            sender
[  5]   0.00-10.00  sec   462 MBytes   387 Mbits/sec                  receiver

Russell King (Oracle)

maintainer

2025-11-14 09:08 UTC #3 lore raw

Quoted 7 lines · Wei Fang

Quoted 6 lines · Wei Fang
Quoted 2 lines
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.
What problem does this cause?

Quoted 7 lines

Our i.MX943 platform, the switch CPU port is connected to an ENETC MAC, the link speed is 2.5Gbps. And one user port of switch is 1Gbps. If the flow-control of internal link is not enabled, we can see the iperf performance of TCP packets is very low. Because of the congestion (ingress speed of CPU port is greater than egress speed of user port) in the switch, some TCP packets were dropped, resulting in the packets being retransmitted many times.

Please include this in the commit message, thanks.

Signature · 2 lines

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

Wei Fang

author

2025-11-14 09:18 UTC #4 lore raw

Quoted 16 lines · Wei Fang

Quoted 7 lines
Quoted 6 lines · Wei Fang
Quoted 2 lines
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.
What problem does this cause?
Quoted 7 lines
Our i.MX943 platform, the switch CPU port is connected to an ENETC MAC, the link speed is 2.5Gbps. And one user port of switch is 1Gbps. If the flow-control of internal link is not enabled, we can see the iperf performance of TCP packets is very low. Because of the congestion (ingress speed of CPU port is greater than egress speed of user port) in the switch, some TCP packets were dropped, resulting in the packets being retransmitted many times.

Quoted 2 lines

Please include this in the commit message, thanks.

Okay, I will add this to the commit message in v2, thanks.

Maxime Chevallier

Reviewed-by on v1 1/1

2025-11-14 08:38 UTC #5 lore raw

In reply to Wei Fang

Quoted 5 lines · Wei Fang

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.
Fixes de7d3f87be3c ("net: phylink: Use phy_caps_lookup for fixed-link configuration")
Signed-off-by Wei Fang <[email protected]>

I agree with the fact that these used to be set, and it's no longer the case since the blamed commit. My bad

Reviewed-by Maxime Chevallier <[email protected]>

Maxime

Quoted 18 lines

---
 drivers/net/phy/phylink.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/net/phy/phylink.c +3 -0

index 9d7799ea1c17..918244308215 100644
@@ -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);

Andrew Lunn

maintainer

2025-11-15 20:36 UTC #6 lore raw

In reply to Wei Fang

Quoted 14 lines · Wei Fang

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.
Fixes de7d3f87be3c ("net: phylink: Use phy_caps_lookup for fixed-link configuration")
Signed-off-by Wei Fang <[email protected]>
---
 drivers/net/phy/phylink.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/net/phy/phylink.c +0 -0
index 9d7799ea1c17..918244308215 100644

drivers/net/phy/phylink.c static int phylink_validate(struct phylink *pl, unsigned long *supported, · line 642

Quoted 6 lines

drivers/net/phy/phylink.c +3 -0

@@ -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);

Do these make sense? There is no PHY, so there is no autoneg? So why is autoneg in supported?

You can force pause at the MAC with ethtool:

ethtool -A|--pause devname [autoneg on|off] [rx on|off] [tx on|off]

with autoneg off, rx and tx are directly applied to the MAC. But to do that, are these bits needed in supported?

Maybe you should explain what problem you are seeing?

Andrew

Russell King (Oracle)

maintainer

2025-11-15 21:01 UTC #7 lore raw

Quoted 25 lines · Andrew Lunn

Quoted 14 lines · Wei Fang
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.
Fixes de7d3f87be3c ("net: phylink: Use phy_caps_lookup for fixed-link configuration")
Signed-off-by Wei Fang <[email protected]>
---
 drivers/net/phy/phylink.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/net/phy/phylink.c +0 -0
index 9d7799ea1c17..918244308215 100644
drivers/net/phy/phylink.c static int phylink_validate(struct phylink *pl, unsigned long *supported, · line 642
Quoted 6 lines
drivers/net/phy/phylink.c +3 -0
@@ -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);
Do these make sense? There is no PHY, so there is no autoneg? So why is autoneg in supported?

Quoted 3 lines

You can force pause at the MAC with ethtool:
ethtool -A|--pause devname [autoneg on|off] [rx on|off] [tx on|off]

No, not for fixed links. (I have a patch which enables this, but we mutually agreed not to push it into mainline - I think you've forgotten that discussion.)

Quoted 1 line

Maybe you should explain what problem you are seeing?

As explained in this thread, it's the lack of pause. Not having the pause bits in the supported mask means that phylink can't evaluate the pause modes to use, because even if they are present in DT, they get cleared because they're cleared in the supported mask.

At least Pause and Asym_Pause need to be set.

This is because the fixed-link pause specification is in terms of Pause and Asym_Pause. Having one set of these bits is meaningless in terms of "should we transmit pause frames" and "should we receive pause frames" which is what hardware wants to know. It would've been better had the DT binding defined pause in terms of tx/rx not the Pause/Asym_Pause bits.

However, with that definition, the only way to give these bits any sane meaning is to treat them as the capabilities of a virtual link partner, use the Pause and Asym_Pause capabilities of the local MAC, and evaluate them according to the 802.3 rules.

However, as we end up masking off the local MAC's Pause and Asym_Pause bits, this results in that evaluation deciding that pause is unsupported. This is a *regression* that needs fixing, caused by the blamed commit.

There is no question that this needs fixing.

The question is whether Autoneg should be set or not. As the advertising and lp_advertising bitmasks have to be non-empty, and the swphy reports aneg capable, aneg complete, and AN enabled, then for consistency with that state, Autoneg should be set. This is how it was prior to the blamed commit.

So, the patch is the correct approach. The only thing that is missing is the detailed explanation about the problem the regression is causing. We have too many patches that fix regressions without explaining what the effect of the regression was... which is bad when we look back at the history.

Signature · 2 lines

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

Andrew Lunn

maintainer

2025-11-15 21:15 UTC #8 lore raw

Quoted 3 lines

No, not for fixed links. (I have a patch which enables this, but we mutually agreed not to push it into mainline - I think you've forgotten that discussion.)

Not too surprising, given the number of discussions...

Quoted 5 lines

So, the patch is the correct approach. The only thing that is missing is the detailed explanation about the problem the regression is causing. We have too many patches that fix regressions without explaining what the effect of the regression was... which is bad when we look back at the history.

Agreed.

I often like Vladimirs patches. Two pages of commit message for a one line change.

Andrew

---
pw-bot: cr