Message-ID: <aRoK2A4qh-vLKhpZ@shell.armlinux.org.uk>
Date: Sun, 16 Nov 2025 17:33:12 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Andrew Lunn <andrew@...n.ch>
Cc: Wei Fang <wei.fang@....com>, hkallweit1@...il.com, davem@...emloft.net,
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
	eric@...int.com, maxime.chevallier@...tlin.com, imx@...ts.linux.dev,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] net: phylink: add missing supported link modes for
 the fixed-link

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!

Powered by blists - more mailing lists