unconditional autoneg=off for pre-boot inserted SFP module

[plappermaul]

Taking over from https://marc.info/?l=linux-netdev&m=177659123124603&w=2 and directly adding @nbd168 @janh @jonasjelonek

During boot of a Realtek switch the SFP ports with pre-plugged modules are configured for autoneg=off. Issue can be reproduced with

• use standard initramfs image
• start switch with pre plugged SFP module
• ethtool lanX shows "Advertised auto-negotiation: No"
• unplug/replug the moduler
• ethtool lanX shows "Advertised auto-negotiation: Yes"

The kernel trace shows that the otherwise good autoneg=on setting is intercepted by an ethtool call. Most likely issued by netifd. Current assumption is: system_set_ethtool_settings() runs the ioctl unconditionally.

	if (s->flags & DEV_OPT_AUTONEG) {
		ecmd.req.autoneg = s->autoneg ? AUTONEG_ENABLE : AUTONEG_DISABLE;
		if (!s->autoneg) {
			if (s->flags & DEV_OPT_SPEED)
				ecmd.req.speed = s->speed;

			if (s->flags & DEV_OPT_DUPLEX)
				ecmd.req.duplex = s->duplex ? DUPLEX_FULL : DUPLEX_HALF;
		}
	}

+	POSSIBLE FIX FOR UNCONDITIONAL AUTONEG=OFF
+	if (!(s->flags & (DEV_OPT_AUTONEG | DEV_OPT_SPEED | DEV_OPT_DUPLEX | DEV_OPT_PAUSE | DEV_OPT_ASYM_PAUSE)))
+		return;

	ecmd.req.cmd = ETHTOOL_SLINKSETTINGS;
	ioctl(sock_ioctl, SIOCETHTOOL, &ifr);

The simple initramfs config of this test.

root@OpenWrt:~# uci show network
network.loopback=interface
network.loopback.device='lo'
network.loopback.proto='static'
network.loopback.ipaddr='127.0.0.1/8'
network.globals=globals
network.globals.dhcp_default_duid='000454fd41adbcf3401492054f65a7d15f8c'
network.globals.ula_prefix='fd59:80be:a7aa::/48'
network.switch=device
network.switch.name='switch'
network.switch.type='bridge'
network.switch.macaddr='fc:22:f4:ec:13:7f'
network.lan_vlan=bridge-vlan
network.lan_vlan.device='switch'
network.lan_vlan.vlan='1'
network.lan_vlan.ports='lan1 lan2 lan3 lan4 lan5 lan6 lan7 lan8 lan9 lan10 lan11 lan12'
network.@device[1]=device
network.@device[1].name='switch.1'
network.@device[1].macaddr='fc:22:f4:ec:13:7f'
network.lan=interface
network.lan.device='switch.1'
network.lan.proto='static'
network.lan.ipaddr='192.168.2.71/24'
network.lan.ip6assign='60'

Boot log for an affected interface.

[    3.301299] XXXX phylink_create lan12 set pl->link_config.advertising (autoneg = 1)
[    3.309954] XXXX phylink_parse_mode lan12 set pl->link_config.advertising (autoneg = 1)
[    3.318964] XXX sfp_module_insert lan12 called
[    3.323935] XXXX phylink_sfp_config_optical lan12 set config.advertising (autoneg = 1)
[    3.332815] XXXX phylink_validate_one lan12 set tmp_supported (autoneg = 1)
[    3.340629] XXXX phylink_validate_mask lan12 set supported (autoneg = 1)
[    3.348165] XXXX phylink_validate_mask lan12 set state->advertising (autoneg = 1)
[    3.356527] rtl83xx-switch 1b000000.switchcore:ethernet-switch lan12 (uninitialized): XXX phylink_sfp_set_config requesting link mode inband/1000base-x with support 0000000,00000000,00000200,00006440
--- ETHTOOL CALL HERE ---
[   81.213726] XXXX phylink_ethtool_ksettings_set lan12 start got config.advertising (autoneg = 1)
[   81.223542] XXXX phylink_ethtool_ksettings_set lan12 set accoring to kset->base.autoneg (autoneg = 0)
[   81.233961] CPU: 0 UID: 0 PID: 1470 Comm: netifd Tainted: G           O        6.18.21 #0 NONE
[   81.234010] Tainted: [O]=OOT_MODULE
[   81.234017] Hardware name: Zyxel XGS1210-12 A1 Switch
[   81.234026] Stack : 823a3bbc 80139d20 00000000 00000001 00000000 00000000 00000000 00000000
[   81.234094]         00000000 00000000 00000000 00000000 00000000 00000001 823a3b78 82040d00
[   81.234152]         00000000 00000000 80992870 823a3a10 00000000 ffffefff 00000001 00000224
[   81.234213]         00000226 823a39d4 00000226 000019c8 00000001 00000000 80992870 80a00000
[   81.234273]         82764648 00000016 00000016 82764628 00000000 80a913b0 00000000 81990000
[   81.234334]         ...
[   81.234350] Call Trace:
[   81.234356] [<80115e48>] show_stack+0x28/0xf0
[   81.234407] [<8010fc78>] dump_stack_lvl+0x70/0xb0
[   81.234432] [<80592718>] phylink_ethtool_ksettings_set+0x58c/0x6a4
[   81.234479] [<806bb890>] ethtool_set_link_ksettings+0xbc/0x198
[   81.234516] [<806be01c>] __dev_ethtool+0xfe0/0x1a1c
[   81.234550] [<806beb24>] dev_ethtool+0xcc/0x24c
[   81.234575] [<80678770>] dev_ioctl+0x30c/0x5f4
[   81.234616] [<80603ebc>] sock_ioctl+0x2bc/0x470
[   81.234642] [<8036e530>] sys_ioctl+0xb4/0x120
[   81.234683] [<8011edec>] syscall_common+0x34/0x58
[   81.234715]
[   81.234723] XXXX 2 phylink_ethtool_ksettings_set lan12 set pl->link_config.advertising (autoneg = 0)
-----------
[   81.375070] rtl83xx-switch 1b000000.switchcore:ethernet-switch lan12: XXX phylink_start configuring for inband/1000base-x link mode
[   81.388408] XXX phylink_start lan12 sfp_bus set and linkmode not empty -> would run linkmode_copy()
[   81.398688] XXX phylink_mac_initial_config lan12 called with force_restart = 1
[   81.406752] rtl83xx-switch 1b000000.switchcore:ethernet-switch lan12: XXX major config, requested inband/1000base-x
[   81.418459] XXXX major_config_entry lan12: autoneg_adv=0 autoneg_sfp=1 sfp_may_have_phy=0
[   81.427612] XXXX phylink_pcs_neg_mode ENTRY lan12: pl->pcs_neg_mode=0x0
[   81.435102] XXXX phylink_pcs_neg_mode lan12 advertising autoneg=0
[   81.442034] rtl83xx-switch 1b000000.switchcore:ethernet-switch lan12: XXX interface 1000base-x inband modes: pcs=03 phy=00
[   81.454495] XXXX phylink_pcs_neg_mode lan12 base-x without phy
[   81.461134] XXXX phylink_pcs_neg_mode EXIT lan12 pl->pcs_neg_mode = 0x40 pl->act_link_an_mode = 0x2
[   82.085493] XXXX phylink_mac_pcs_get_state lan12 set state->advertising (autoneg = 0)
[   82.094391] XXXX phylink_mac_pcs_get_state lan12 autoneg is 0

[plappermaul]

Added this to netifd

	if (s->flags & DEV_OPT_AUTONEG) {
		ecmd.req.autoneg = s->autoneg ? AUTONEG_ENABLE : AUTONEG_DISABLE;
		if (!s->autoneg) {
			if (s->flags & DEV_OPT_SPEED)
				ecmd.req.speed = s->speed;

			if (s->flags & DEV_OPT_DUPLEX)
				ecmd.req.duplex = s->duplex ? DUPLEX_FULL : DUPLEX_HALF;
		}
	}
	
+	netifd_log_message(L_CRIT, "call ethtool for interface %s autneg=%d\n", dev->ifname, ecmd.req.autoneg);

and I get on a Zyxel XGS1210-12

# logread | grep "ethtool" | grep lan
Thu Jan  1 00:00:30 1970 daemon.crit netifd: call ethtool for interface lan1 autneg=1
Thu Jan  1 00:00:30 1970 daemon.crit netifd: call ethtool for interface lan2 autneg=1
Thu Jan  1 00:00:30 1970 daemon.crit netifd: call ethtool for interface lan3 autneg=1
Thu Jan  1 00:00:30 1970 daemon.crit netifd: call ethtool for interface lan4 autneg=1
Thu Jan  1 00:00:30 1970 daemon.crit netifd: call ethtool for interface lan5 autneg=1
Thu Jan  1 00:00:31 1970 daemon.crit netifd: call ethtool for interface lan6 autneg=1
Thu Jan  1 00:00:31 1970 daemon.crit netifd: call ethtool for interface lan7 autneg=1
Thu Jan  1 00:00:31 1970 daemon.crit netifd: call ethtool for interface lan8 autneg=1
Thu Jan  1 00:00:31 1970 daemon.crit netifd: call ethtool for interface lan9 autneg=1
Thu Jan  1 00:00:32 1970 daemon.crit netifd: call ethtool for interface lan10 autneg=1
Thu Jan  1 00:00:32 1970 daemon.crit netifd: call ethtool for interface lan11 autneg=0
Thu Jan  1 00:00:33 1970 daemon.crit netifd: call ethtool for interface lan12 autneg=0

So for some reason it decides to set autoneg to 0 for the SerDes driven SFP ports.

[plappermaul]

Seems as if netifd gets the current setting from phylink_ethtool_ksettings_get() -> phylink_mac_pcs_get_state()

[dangowrt]

This has been a struggle in the past, as settings also have to be re-applied in case of a PHY on the SFP being discovered. As there is not (yet) a good way to tell ethtool netlink on which layer to apply a setting (MAC? PCS? PHY? if PHY, then which one, the one RGMII-connected PHY offering the SerDes to an SFP slot or the PHY of the SFP module?). Maxime is working on that, and I suppose until this work has fully landed it will have to somehow be based on heuristics -- which may be wrong or hitting a subtle driver difference here (while working fine on other platforms).

[plappermaul]

Totally understood. But that should be consistent. Further analysis showed: The first netifd call towards phylink_ethtool_ksettings_get() is "interrupted" here:

int phylink_ethtool_ksettings_get(struct phylink *pl,
				  struct ethtool_link_ksettings *kset)
{
	struct phylink_link_state link_state;
	
	phylink_info(pl, "XXX %s started\n", __func__);

	ASSERT_RTNL();

	if (pl->phydev) {
		phylink_info(pl, "XXX %s use phydev\n", __func__);
		phy_ethtool_ksettings_get(pl->phydev, kset);
	} else {
		phylink_info(pl, "XXX %s use base.port\n", __func__);
		kset->base.port = pl->link_port;
	}
	
	linkmode_copy(kset->link_modes.supported, pl->supported);

	switch (pl->act_link_an_mode) {
	case MLO_AN_FIXED:
		/* We are using fixed settings. Report these as the
		 * current link settings - and note that these also
		 * represent the supported speeds/duplex/pause modes.
		 */
		phylink_get_fixed_state(pl, &link_state);
		phylink_get_ksettings(&link_state, kset);
		break;

	case MLO_AN_INBAND:
		/* If there is a phy attached, then use the reported
		 * settings from the phy with no modification.
		 */
		if (pl->phydev)
			break; <--- ENDS HERE BECAUSE OF NO PHYDEV

		phylink_mac_pcs_get_state(pl, &link_state);

		/* The MAC is reporting the link results from its own PCS
		 * layer via in-band status. Report these as the current
		 * link settings.
		 */
		phylink_get_ksettings(&link_state, kset);
		break;
	}

	return 0;
}

So it returns autoneg=0 and netifd writes back that value into the kernel. Does it make sense to fill existing SFP supported data here?

[plappermaul]

Reading Andrew Lunns answer https://marc.info/?l=linux-netdev&m=177671053231375&w=2 I'm conviced the best solution will be to run ethtool ioctl only if really needed.

static void
system_set_ethtool_settings(struct device *dev, struct device_settings *s)
{
	struct {
		struct ethtool_link_settings req;
		__u32 link_mode_data[3 * 127];
	} ecmd;
	struct ifreq ifr = {
		.ifr_data = (caddr_t)&ecmd,
	};
	size_t i;
	__s8 nwords;
	__u32 *supported, *advertising;
	
	system_set_ethtool_pause(dev, s);

	if (s->flags & DEV_OPT_EEE)
		system_set_ethtool_eee_settings(dev, s);

	/* Apply PSE (PoE) settings if configured */
	if (s->flags & (DEV_OPT_PSE | DEV_OPT_PSE_PODL | DEV_OPT_PSE_POWER_LIMIT | DEV_OPT_PSE_PRIORITY))
		system_pse_set(dev, s);

+	/* use ethtool linksettings ioctl only if really needed. This
+	 * avoids inconsistencies when interface is in admin state.
+	 * At that time not all fields are filled during read properly
+	 * (e.g. autoneg).
+	 */
+	if (!(s->flags & (DEV_OPT_AUTONEG | DEV_OPT_SPEED | DEV_OPT_DUPLEX | DEV_OPT_PAUSE | DEV_OPT_ASYM_PAUSE)))
+		return;	

	memset(&ecmd, 0, sizeof(ecmd));
	ecmd.req.cmd = ETHTOOL_GLINKSETTINGS;
	strncpy(ifr.ifr_name, dev->ifname, sizeof(ifr.ifr_name) - 1);