From 0b23ae315850540c9078869862b29d494508f03f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 27 May 2020 13:50:40 +0200 Subject: device: reset original autoneg/speed/duplex setting on deactivate The autoneg/speed ethtool settings are important. If they are wrong, the device might not get any carrier. Having no carrier means that you may be unable to activate a profile (because depending on configuration, carrier is required to activate a profile). Since activating profiles are the means to configure the link settings in NetworkManager, and activating a profile can be hampered by wrong link settings, it's important to reset the "correct" settings, when deactivating a profile. "Correct" in this case means to restore the settings that were present before NM changed the settings. Presumably, these are the right once. Beyond that, in the future it might make sense to support configuring the default link settings per device. So that NM will always restore a defined, configured, working state. The problem is that per-device settings currently are only available via NetworkManager.conf, which is rather inflexible. Also, when you restart NetworkManager service, it leaves the interface up but forgets the previous setting. That possibly could be fixed by persisting the previous link state in /run. However, it's not implemented yet. https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/356 https://bugzilla.redhat.com/show_bug.cgi?id=1807171 --- src/devices/nm-device-ethernet.c | 59 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 58 insertions(+), 1 deletion(-) diff --git a/src/devices/nm-device-ethernet.c b/src/devices/nm-device-ethernet.c index 7556bb27ba..9e7601f005 100644 --- a/src/devices/nm-device-ethernet.c +++ b/src/devices/nm-device-ethernet.c @@ -100,7 +100,15 @@ typedef struct _NMDeviceEthernetPrivate { DcbWait dcb_wait; guint dcb_timeout_id; + guint32 ethtool_prev_speed; + + NMPlatformLinkDuplexType ethtool_prev_duplex:3; + bool dcb_handle_carrier_changes:1; + + bool ethtool_prev_set:1; + bool ethtool_prev_autoneg:1; + } NMDeviceEthernetPrivate; NM_GOBJECT_PROPERTIES_DEFINE (NMDeviceEthernet, @@ -882,6 +890,7 @@ static void link_negotiation_set (NMDevice *device) { NMDeviceEthernet *self = NM_DEVICE_ETHERNET (device); + NMDeviceEthernetPrivate *priv = NM_DEVICE_ETHERNET_GET_PRIVATE (self); NMSettingWired *s_wired; gboolean autoneg = TRUE; gboolean link_autoneg; @@ -925,7 +934,7 @@ link_negotiation_set (NMDevice *device) && !duplex) _LOGD (LOGD_DEVICE, "set-link: configure auto-negotiation"); else { - _LOGD (LOGD_DEVICE, "set-link: configure %snegotiation (%u Mbit%s - %s duplex%s)", + _LOGD (LOGD_DEVICE, "set-link: configure %snegotiation (%u Mbit%s, %s duplex%s)", autoneg ? "auto-" : "static ", speed ?: link_speed, speed ? "" : "*", @@ -935,6 +944,14 @@ link_negotiation_set (NMDevice *device) duplex ? "" : "*"); } + if (!priv->ethtool_prev_set) { + /* remember the values we had before setting it. */ + priv->ethtool_prev_autoneg = link_autoneg; + priv->ethtool_prev_speed = link_speed; + priv->ethtool_prev_duplex = link_duplex; + priv->ethtool_prev_set = TRUE; + } + if (!nm_platform_ethtool_set_link_settings (nm_device_get_platform (device), nm_device_get_ifindex (device), autoneg, @@ -964,6 +981,28 @@ act_stage1_prepare (NMDevice *device, NMDeviceStateReason *out_failure_reason) NMDeviceEthernet *self = NM_DEVICE_ETHERNET (device); NMDeviceEthernetPrivate *priv = NM_DEVICE_ETHERNET_GET_PRIVATE (self); + if (nm_device_sys_iface_state_is_external_or_assume (device)) { + if ( !priv->ethtool_prev_set + && !nm_device_sys_iface_state_is_external (device)) { + NMSettingWired *s_wired; + + /* During restart of NetworkManager service we forget the original auto + * negotiation settings. When taking over a device, remember to reset + * the "default" during deactivate. */ + s_wired = nm_device_get_applied_setting (device, NM_TYPE_SETTING_WIRED); + if ( s_wired + && ( nm_setting_wired_get_auto_negotiate (s_wired) + || nm_setting_wired_get_speed (s_wired) + || nm_setting_wired_get_duplex (s_wired))) { + priv->ethtool_prev_set = TRUE; + priv->ethtool_prev_autoneg = TRUE; + priv->ethtool_prev_speed = 0; + priv->ethtool_prev_duplex = NM_PLATFORM_LINK_DUPLEX_UNKNOWN; + } + } + return NM_ACT_STAGE_RETURN_SUCCESS; + } + link_negotiation_set (device); /* If we're re-activating a PPPoE connection a short while after @@ -1525,6 +1564,23 @@ deactivate (NMDevice *device) /* Set last PPPoE connection time */ if (nm_device_get_applied_setting (device, NM_TYPE_SETTING_PPPOE)) priv->last_pppoe_time = nm_utils_get_monotonic_timestamp_sec (); + + if (priv->ethtool_prev_set) { + priv->ethtool_prev_set = FALSE; + + _LOGD (LOGD_DEVICE, "set-link: reset %snegotiation (%u Mbit, %s duplex)", + priv->ethtool_prev_autoneg ? "auto-" : "static ", + priv->ethtool_prev_speed, + nm_platform_link_duplex_type_to_string (priv->ethtool_prev_duplex)); + if (!nm_platform_ethtool_set_link_settings (nm_device_get_platform (device), + nm_device_get_ifindex (device), + priv->ethtool_prev_autoneg, + priv->ethtool_prev_speed, + priv->ethtool_prev_duplex)) { + _LOGW (LOGD_DEVICE, "set-link: failure to reset link negotiation"); + return; + } + } } static gboolean @@ -1914,6 +1970,7 @@ nm_device_ethernet_class_init (NMDeviceEthernetClass *klass) device_class->complete_connection = complete_connection; device_class->new_default_connection = new_default_connection; + device_class->act_stage1_prepare_also_for_external_or_assume = TRUE; device_class->act_stage1_prepare = act_stage1_prepare; device_class->act_stage1_prepare_set_hwaddr_ethernet = TRUE; device_class->act_stage2_config = act_stage2_config; -- cgit v1.2.1