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

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

[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 <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

Re: [PATCH net] net: phylink: add missing supported link modes for the fixed-link

[Russell King (Oracle)]

On Fri, Nov 14, 2025 at 01:28:08PM +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.

What problem does this cause?

-- 
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] net: phylink: add missing supported link modes for the fixed-link

[Wei Fang]

> On Fri, Nov 14, 2025 at 01:28:08PM +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.
> 
> 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

Re: [PATCH net] net: phylink: add missing supported link modes for the fixed-link

[Russell King (Oracle)]

On Fri, Nov 14, 2025 at 08:52:07AM +0000, Wei Fang wrote:
> > On Fri, Nov 14, 2025 at 01:28:08PM +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.
> > 
> > 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.

Please include this in the commit message, thanks.

-- 
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] net: phylink: add missing supported link modes for the fixed-link

[Wei Fang]

> On Fri, Nov 14, 2025 at 08:52:07AM +0000, Wei Fang wrote:
> > > On Fri, Nov 14, 2025 at 01:28:08PM +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.
> > >
> > > 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.
> 
> Please include this in the commit message, thanks.
> 

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

Re: [PATCH net] net: phylink: add missing supported link modes for the fixed-link

[Maxime Chevallier]

On 14/11/2025 06:28, 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.
> 
> Fixes: de7d3f87be3c ("net: phylink: Use phy_caps_lookup for fixed-link configuration")
> Signed-off-by: Wei Fang <wei.fang@nxp.com>

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 <maxime.chevallier@bootlin.com>

Maxime

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

Re: [PATCH net] net: phylink: add missing supported link modes for the fixed-link

[Andrew Lunn]

On Fri, Nov 14, 2025 at 01:28:08PM +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.
> 
> Fixes: de7d3f87be3c ("net: phylink: Use phy_caps_lookup for fixed-link configuration")
> Signed-off-by: Wei Fang <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);

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

Re: [PATCH net] net: phylink: add missing supported link modes for the fixed-link

[Russell King (Oracle)]

On Sat, Nov 15, 2025 at 09:36:44PM +0100, Andrew Lunn wrote:
> On Fri, Nov 14, 2025 at 01:28:08PM +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.
> > 
> > Fixes: de7d3f87be3c ("net: phylink: Use phy_caps_lookup for fixed-link configuration")
> > Signed-off-by: Wei Fang <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);
> 
> 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]

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.)

> 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.

-- 
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] net: phylink: add missing supported link modes for the fixed-link

[Andrew Lunn]

> 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...

> 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