From 58ec9ec8b4f5a566c219f556b8a37f421363f44f Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Tue, 1 Aug 2023 03:22:57 +0900 Subject: [PATCH 1/4] udev-builtin-net_id: skip non-directory entry earlier In the below, we will try to read 'address' file in the directory, hence the entry must be a directory. No functional change, just a tiny optimization. (cherry picked from commit 4103dca1b5664f937ce125219ca70ea54f810ac8) Related: RHEL-50103 --- src/udev/udev-builtin-net_id.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/udev/udev-builtin-net_id.c b/src/udev/udev-builtin-net_id.c index c20df41c37..40f7454ba0 100644 --- a/src/udev/udev-builtin-net_id.c +++ b/src/udev/udev-builtin-net_id.c @@ -441,6 +441,9 @@ static int dev_pci_slot(sd_device *dev, const LinkInfo *info, NetNames *names) { if (dot_or_dot_dot(de->d_name)) continue; + if (de->d_type != DT_DIR) + continue; + r = safe_atou32(de->d_name, &i); if (r < 0 || i <= 0) continue; From 6c2da263c8683847b81d7902d6954559fec36fad Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Tue, 1 Aug 2023 03:25:39 +0900 Subject: [PATCH 2/4] udev-builtin-net_id: return earlier when hotplug slot is not found Then we can reduce indentation. No functional change, just refactoring. (cherry picked from commit 73fb4b20c1f653619286b2e9ce51c19169ccbfc6) Related: RHEL-50103 --- src/udev/udev-builtin-net_id.c | 45 +++++++++++++++++----------------- 1 file changed, 23 insertions(+), 22 deletions(-) diff --git a/src/udev/udev-builtin-net_id.c b/src/udev/udev-builtin-net_id.c index 40f7454ba0..5e76a894f7 100644 --- a/src/udev/udev-builtin-net_id.c +++ b/src/udev/udev-builtin-net_id.c @@ -487,28 +487,29 @@ static int dev_pci_slot(sd_device *dev, const LinkInfo *info, NetNames *names) { rewinddir(dir); } - if (hotplug_slot > 0) { - s = names->pci_slot; - l = sizeof(names->pci_slot); - if (domain > 0) - l = strpcpyf(&s, l, "P%u", domain); - l = strpcpyf(&s, l, "s%"PRIu32, hotplug_slot); - if (func > 0 || is_pci_multifunction(names->pcidev) > 0) - l = strpcpyf(&s, l, "f%u", func); - if (naming_scheme_has(NAMING_SR_IOV_R) && info->vf_representor_id >= 0) - /* For VF representor append 'r' and not phys_port_name */ - l = strpcpyf(&s, l, "r%d", info->vf_representor_id); - else if (!isempty(info->phys_port_name)) - l = strpcpyf(&s, l, "n%s", info->phys_port_name); - else if (dev_port > 0) - l = strpcpyf(&s, l, "d%lu", dev_port); - if (l == 0) - names->pci_slot[0] = '\0'; - - log_device_debug(dev, "Slot identifier: domain=%u slot=%"PRIu32" func=%u phys_port=%s dev_port=%lu %s %s", - domain, hotplug_slot, func, strempty(info->phys_port_name), dev_port, - special_glyph(SPECIAL_GLYPH_ARROW_RIGHT), empty_to_na(names->pci_slot)); - } + if (hotplug_slot == 0) + return 0; + + s = names->pci_slot; + l = sizeof(names->pci_slot); + if (domain > 0) + l = strpcpyf(&s, l, "P%u", domain); + l = strpcpyf(&s, l, "s%"PRIu32, hotplug_slot); + if (func > 0 || is_pci_multifunction(names->pcidev) > 0) + l = strpcpyf(&s, l, "f%u", func); + if (naming_scheme_has(NAMING_SR_IOV_R) && info->vf_representor_id >= 0) + /* For VF representor append 'r' and not phys_port_name */ + l = strpcpyf(&s, l, "r%d", info->vf_representor_id); + else if (!isempty(info->phys_port_name)) + l = strpcpyf(&s, l, "n%s", info->phys_port_name); + else if (dev_port > 0) + l = strpcpyf(&s, l, "d%lu", dev_port); + if (l == 0) + names->pci_slot[0] = '\0'; + + log_device_debug(dev, "Slot identifier: domain=%u slot=%"PRIu32" func=%u phys_port=%s dev_port=%lu %s %s", + domain, hotplug_slot, func, strempty(info->phys_port_name), dev_port, + special_glyph(SPECIAL_GLYPH_ARROW_RIGHT), empty_to_na(names->pci_slot)); return 0; } From e690f4b27c1cb31d521e5770bd274fb551f628c0 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Tue, 1 Aug 2023 03:27:33 +0900 Subject: [PATCH 3/4] udev-builtin-net_id: split-out pci_get_hotplug_slot() and pci_get_hotplug_slot_from_address() No functional changes, just refactoring. (cherry picked from commit f1e3eaa730190a60fdb780be26ee331b8c811e34) Related: RHEL-50103 --- src/udev/udev-builtin-net_id.c | 199 +++++++++++++++++++-------------- 1 file changed, 117 insertions(+), 82 deletions(-) diff --git a/src/udev/udev-builtin-net_id.c b/src/udev/udev-builtin-net_id.c index 5e76a894f7..16c9971876 100644 --- a/src/udev/udev-builtin-net_id.c +++ b/src/udev/udev-builtin-net_id.c @@ -332,14 +332,121 @@ static int parse_hotplug_slot_from_function_id(sd_device *dev, int slots_dirfd, return 1; } -static int dev_pci_slot(sd_device *dev, const LinkInfo *info, NetNames *names) { - const char *sysname, *attr; +static int pci_get_hotplug_slot_from_address( + sd_device *dev, + sd_device *pci, + DIR *dir, + uint32_t *ret) { + + const char *sysname; + int r; + + assert(dev); + assert(pci); + assert(dir); + assert(ret); + + r = sd_device_get_sysname(dev, &sysname); + if (r < 0) + return log_device_debug_errno(dev, r, "Failed to get sysname: %m"); + + rewinddir(dir); + FOREACH_DIRENT_ALL(de, dir, break) { + _cleanup_free_ char *path = NULL; + const char *address; + uint32_t slot; + + if (dot_or_dot_dot(de->d_name)) + continue; + + if (de->d_type != DT_DIR) + continue; + + r = safe_atou32(de->d_name, &slot); + if (r < 0 || slot <= 0) + continue; + + path = path_join("slots", de->d_name, "address"); + if (!path) + return -ENOMEM; + + if (sd_device_get_sysattr_value(pci, path, &address) < 0) + continue; + + /* match slot address with device by stripping the function */ + if (!startswith(sysname, address)) + continue; + + *ret = slot; + return 1; /* found */ + } + + *ret = 0; + return 0; /* not found */ +} + +static int pci_get_hotplug_slot(sd_device *dev, uint32_t *ret) { _cleanup_(sd_device_unrefp) sd_device *pci = NULL; _cleanup_closedir_ DIR *dir = NULL; + int r; + + assert(dev); + assert(ret); + + /* ACPI _SUN — slot user number */ + r = sd_device_new_from_subsystem_sysname(&pci, "subsystem", "pci"); + if (r < 0) + return log_debug_errno(r, "Failed to create sd_device object for pci subsystem: %m"); + + r = device_opendir(pci, "slots", &dir); + if (r < 0) + return log_device_debug_errno(dev, r, "Cannot open 'slots' subdirectory: %m"); + + for (sd_device *slot_dev = dev; slot_dev; ) { + uint32_t slot = 0; /* avoid false maybe-uninitialized warning */ + + r = parse_hotplug_slot_from_function_id(slot_dev, dirfd(dir), &slot); + if (r < 0) + return r; + if (r > 0) { + *ret = slot; + return 1; /* domain should be ignored. */ + } + + r = pci_get_hotplug_slot_from_address(slot_dev, pci, dir, &slot); + if (r < 0) + return r; + if (r > 0) { + /* We found the match between PCI device and slot. However, we won't use the slot + * index if the device is a PCI bridge, because it can have other child devices that + * will try to claim the same index and that would create name collision. */ + if (naming_scheme_has(NAMING_BRIDGE_NO_SLOT) && is_pci_bridge(slot_dev)) { + if (naming_scheme_has(NAMING_BRIDGE_MULTIFUNCTION_SLOT) && is_pci_multifunction(dev) <= 0) + return log_device_debug_errno(dev, SYNTHETIC_ERRNO(ESTALE), + "Not using slot information because the PCI device associated with " + "the hotplug slot is a bridge and the PCI device has a single function."); + + if (!naming_scheme_has(NAMING_BRIDGE_MULTIFUNCTION_SLOT)) + return log_device_debug_errno(dev, SYNTHETIC_ERRNO(ESTALE), + "Not using slot information because the PCI device is a bridge."); + } + + *ret = slot; + return 0; /* domain can be still used. */ + } + + if (sd_device_get_parent_with_subsystem_devtype(slot_dev, "pci", NULL, &slot_dev) < 0) + break; + } + + return -ENOENT; +} + +static int dev_pci_slot(sd_device *dev, const LinkInfo *info, NetNames *names) { + const char *sysname, *attr; unsigned domain, bus, slot, func; - sd_device *hotplug_slot_dev; unsigned long dev_port = 0; - uint32_t hotplug_slot = 0; + uint32_t hotplug_slot = 0; /* avoid false maybe-uninitialized warning */ size_t l; char *s; int r; @@ -410,85 +517,13 @@ static int dev_pci_slot(sd_device *dev, const LinkInfo *info, NetNames *names) { domain, bus, slot, func, strempty(info->phys_port_name), dev_port, special_glyph(SPECIAL_GLYPH_ARROW_RIGHT), empty_to_na(names->pci_path)); - /* ACPI _SUN — slot user number */ - r = sd_device_new_from_subsystem_sysname(&pci, "subsystem", "pci"); - if (r < 0) - return log_debug_errno(r, "sd_device_new_from_subsystem_sysname() failed: %m"); - - r = device_opendir(pci, "slots", &dir); + r = pci_get_hotplug_slot(names->pcidev, &hotplug_slot); if (r < 0) - return log_device_debug_errno(dev, r, "Cannot access 'slots' subdirectory: %m"); - - hotplug_slot_dev = names->pcidev; - while (hotplug_slot_dev) { - r = parse_hotplug_slot_from_function_id(hotplug_slot_dev, dirfd(dir), &hotplug_slot); - if (r < 0) - return 0; - if (r > 0) { - domain = 0; /* See comments in parse_hotplug_slot_from_function_id(). */ - break; - } - - r = sd_device_get_sysname(hotplug_slot_dev, &sysname); - if (r < 0) - return log_device_debug_errno(hotplug_slot_dev, r, "Failed to get sysname: %m"); - - FOREACH_DIRENT_ALL(de, dir, break) { - _cleanup_free_ char *path = NULL; - const char *address; - uint32_t i; - - if (dot_or_dot_dot(de->d_name)) - continue; - - if (de->d_type != DT_DIR) - continue; - - r = safe_atou32(de->d_name, &i); - if (r < 0 || i <= 0) - continue; - - path = path_join("slots", de->d_name, "address"); - if (!path) - return -ENOMEM; - - if (device_get_sysattr_value_filtered(pci, path, &address) < 0) - continue; - - /* match slot address with device by stripping the function */ - if (!startswith(sysname, address)) - continue; - - hotplug_slot = i; - - /* We found the match between PCI device and slot. However, we won't use the slot - * index if the device is a PCI bridge, because it can have other child devices that - * will try to claim the same index and that would create name collision. */ - if (naming_scheme_has(NAMING_BRIDGE_NO_SLOT) && is_pci_bridge(hotplug_slot_dev)) { - if (naming_scheme_has(NAMING_BRIDGE_MULTIFUNCTION_SLOT) && is_pci_multifunction(names->pcidev) <= 0) { - log_device_debug(dev, - "Not using slot information because the PCI device associated with " - "the hotplug slot is a bridge and the PCI device has a single function."); - return 0; - } - - if (!naming_scheme_has(NAMING_BRIDGE_MULTIFUNCTION_SLOT)) { - log_device_debug(dev, "Not using slot information because the PCI device is a bridge."); - return 0; - } - } - - break; - } - if (hotplug_slot > 0) - break; - if (sd_device_get_parent_with_subsystem_devtype(hotplug_slot_dev, "pci", NULL, &hotplug_slot_dev) < 0) - break; - rewinddir(dir); - } - - if (hotplug_slot == 0) - return 0; + return r; + if (r > 0) + /* If the hotplug slot is found through the function ID, then drop the domain from the name. + * See comments in parse_hotplug_slot_from_function_id(). */ + domain = 0; s = names->pci_slot; l = sizeof(names->pci_slot); From 856330ce58f8460d8475db6bbcfb0e49952eba77 Mon Sep 17 00:00:00 2001 From: Etienne Champetier Date: Tue, 9 Jul 2024 11:53:50 -0400 Subject: [PATCH 4/4] udev-builtin-net_id: use firmware_node/sun for ID_NET_NAME_SLOT pci_get_hotplug_slot() has the following limitations: - if slots are not hotpluggable, they are not in /sys/bus/pci/slots. - the address at /sys/bus/pci/slots/X/addr doesn't contains the function part, so on some system, 2 different slots with different _SUN end up with the same hotplug_slot, leading to naming conflicts. - it tries all parent devices until it finds a slot number, which is incorrect, and what led to NAMING_BRIDGE_MULTIFUNCTION_SLOT being disabled. The use of PCI hotplug to find the slot (ACPI _SUN) was introduced in https://github.com/systemd/systemd/commit/0035597a30d120f70df2dd7da3d6128fb8ba6051 "udev: net_id - export PCI hotplug slot names" on 2012/11/26. At the same time on the kernel side we got https://github.com/torvalds/linux/commit/bb74ac23b10820d8722c3e1f4add9ef59e703f63 "ACPI: create _SUN sysfs file" on 2012/11/16. Using PCI hotplug was the only way at the time, but now 12 years later we can use firmware_node/sun sysfs file. Looking at a small selection of server HW, for HPE (Gen10 DL325), the _SUN is attached to the NIC device, whereas for Dell (R640/R6515/R6615) and Cisco (UCSC-C220-M5SX), the _SUN is on the first parent pcieport. We still fallback to pci_get_hotplug_slot() to handle the s390 case and maybe some other coner cases (_SUN on grand parent device that is not a bridge ?). (cherry picked from commit 0a4ecc54cb9f2d3418b970c51bfadb69c34ae9eb) Resolves: RHEL-50103 --- man/systemd.net-naming-scheme.xml | 5 ++- src/shared/netif-naming-scheme.h | 3 +- src/udev/udev-builtin-net_id.c | 66 +++++++++++++++++++++++++++---- 3 files changed, 64 insertions(+), 10 deletions(-) diff --git a/man/systemd.net-naming-scheme.xml b/man/systemd.net-naming-scheme.xml index 83293e5636..c9a7e1e493 100644 --- a/man/systemd.net-naming-scheme.xml +++ b/man/systemd.net-naming-scheme.xml @@ -510,9 +510,10 @@ to distinguish between devices. However, name conflict can occur if these devices are not children of the same PCI bridge, e.g. there are multiple PCI bridges in the same slot. + + PCI slot number is now read from firmware_node/sun sysfs file. - By default rhel-9.0 is used. @@ -666,7 +667,7 @@ ID_NET_NAME_ONBOARD_LABEL=Ethernet Port 1 - PCI Ethernet card in hotplug slot with firmware index number + PCI Ethernet card in slot with firmware index number # /sys/devices/pci0000:00/0000:00:1c.3/0000:05:00.0/net/ens1 ID_NET_NAME_MAC=enx000000000466 diff --git a/src/shared/netif-naming-scheme.h b/src/shared/netif-naming-scheme.h index 5bc071f8db..b1ac2e4320 100644 --- a/src/shared/netif-naming-scheme.h +++ b/src/shared/netif-naming-scheme.h @@ -42,6 +42,7 @@ typedef enum NamingSchemeFlags { * This is disabled since rhel-9.5, as it seems not to work at least for some setups. See upstream issue #28929. */ NAMING_DEVICETREE_ALIASES = 1 << 15, /* Generate names from devicetree aliases */ NAMING_SR_IOV_R = 1 << 17, /* Use "r" suffix for SR-IOV VF representors */ + NAMING_FIRMWARE_NODE_SUN = 1 << 18, /* Use firmware_node/sun to get PCI slot number */ /* And now the masks that combine the features above */ NAMING_V238 = 0, @@ -73,7 +74,7 @@ typedef enum NamingSchemeFlags { NAMING_RHEL_9_2 = NAMING_RHEL_9_0, NAMING_RHEL_9_3 = NAMING_RHEL_9_0 | NAMING_SR_IOV_R, NAMING_RHEL_9_4 = NAMING_RHEL_9_3, - NAMING_RHEL_9_5 = NAMING_RHEL_9_4 & ~NAMING_BRIDGE_MULTIFUNCTION_SLOT, + NAMING_RHEL_9_5 = (NAMING_RHEL_9_4 & ~NAMING_BRIDGE_MULTIFUNCTION_SLOT) | NAMING_FIRMWARE_NODE_SUN, EXTRA_NET_NAMING_SCHEMES diff --git a/src/udev/udev-builtin-net_id.c b/src/udev/udev-builtin-net_id.c index 16c9971876..291fb4ba36 100644 --- a/src/udev/udev-builtin-net_id.c +++ b/src/udev/udev-builtin-net_id.c @@ -442,6 +442,51 @@ static int pci_get_hotplug_slot(sd_device *dev, uint32_t *ret) { return -ENOENT; } +static int get_device_firmware_node_sun(sd_device *dev, uint32_t *ret) { + const char *attr; + int r; + + assert(dev); + assert(ret); + + r = device_get_sysattr_value_filtered(dev, "firmware_node/sun", &attr); + if (r < 0) + return log_device_debug_errno(dev, r, "Failed to read firmware_node/sun, ignoring: %m"); + + r = safe_atou32(attr, ret); + if (r < 0) + return log_device_warning_errno(dev, r, "Failed to parse firmware_node/sun '%s', ignoring: %m", attr); + + return 0; +} + +static int pci_get_slot_from_firmware_node_sun(sd_device *dev, uint32_t *ret) { + int r; + sd_device *slot_dev; + + assert(dev); + assert(ret); + + /* Try getting the ACPI _SUN for the device */ + if (get_device_firmware_node_sun(dev, ret) >= 0) + return 0; + + r = sd_device_get_parent_with_subsystem_devtype(dev, "pci", NULL, &slot_dev); + if (r < 0) + return log_device_debug_errno(dev, r, "Failed to find pci parent, ignoring: %m"); + + if (is_pci_bridge(slot_dev) && is_pci_multifunction(dev) <= 0) + return log_device_debug_errno(dev, SYNTHETIC_ERRNO(ESTALE), + "Not using slot information because the parent pcieport " + "is a bridge and the PCI device is not multifunction."); + + /* Try getting the ACPI _SUN from the parent pcieport */ + if (get_device_firmware_node_sun(slot_dev, ret) >= 0) + return 0; + + return -ENOENT; +} + static int dev_pci_slot(sd_device *dev, const LinkInfo *info, NetNames *names) { const char *sysname, *attr; unsigned domain, bus, slot, func; @@ -517,13 +562,20 @@ static int dev_pci_slot(sd_device *dev, const LinkInfo *info, NetNames *names) { domain, bus, slot, func, strempty(info->phys_port_name), dev_port, special_glyph(SPECIAL_GLYPH_ARROW_RIGHT), empty_to_na(names->pci_path)); - r = pci_get_hotplug_slot(names->pcidev, &hotplug_slot); - if (r < 0) - return r; - if (r > 0) - /* If the hotplug slot is found through the function ID, then drop the domain from the name. - * See comments in parse_hotplug_slot_from_function_id(). */ - domain = 0; + if (naming_scheme_has(NAMING_FIRMWARE_NODE_SUN)) + r = pci_get_slot_from_firmware_node_sun(names->pcidev, &hotplug_slot); + else + r = -1; + /* If we don't find a slot using firmware_node/sun, fallback to hotplug_slot */ + if (r < 0) { + r = pci_get_hotplug_slot(names->pcidev, &hotplug_slot); + if (r < 0) + return r; + if (r > 0) + /* If the hotplug slot is found through the function ID, then drop the domain from the name. + * See comments in parse_hotplug_slot_from_function_id(). */ + domain = 0; + } s = names->pci_slot; l = sizeof(names->pci_slot);