pci: fix FindPCIAddress for NICs behind bridges#6
pci: fix FindPCIAddress for NICs behind bridges#6christoph-zededa wants to merge 3 commits intozededa:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates PCI address extraction from Linux sysfs paths so devices behind PCIe bridges resolve to the correct (deepest) PCI BDF, and adds unit coverage for common sysfs path shapes.
Changes:
- Update
pci.FindPCIAddressto find all PCI BDFs in a path and return the last one. - Add a table-driven test suite covering root-complex devices, bridged devices, and non-PCI tail components.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
pkg/pci/pci_linux.go |
Changes PCI BDF extraction logic to return the deepest match in a sysfs path. |
pkg/pci/pci_linux_test.go |
Adds table-driven tests for FindPCIAddress across several representative sysfs paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // first element is the full string match like /devices/pci0000:00/0000:00:0d.0 | ||
| // but we need the first substring match so we take the second element | ||
| if len(matches) != 2 { | ||
| re := regexp.MustCompile(`\b\d{4}:[a-f\d]{2}:[a-f\d]{2}\.[a-f\d]\b`) |
There was a problem hiding this comment.
The BDF regex restricts the PCI domain to decimal digits only (\d{4}). PCI domains are hex, so domains like 000a:... won't be detected and FindPCIAddress will incorrectly return "". Consider matching hex digits for the domain (and ideally make the whole match case-insensitive to align with other PCI address parsing in the repo).
| re := regexp.MustCompile(`\b\d{4}:[a-f\d]{2}:[a-f\d]{2}\.[a-f\d]\b`) | |
| re := regexp.MustCompile(`(?i)\b[0-9a-f]{4}:[0-9a-f]{2}:[0-9a-f]{2}\.[0-9a-f]\b`) |
| func FindPCIAddress(path string) string { | ||
| re := regexp.MustCompile(`\/?devices\/pci[\d:.]*\/(\d{4}:[a-f\d:\.]+)`) | ||
|
|
||
| matches := re.FindStringSubmatch(path) | ||
| // first element is the full string match like /devices/pci0000:00/0000:00:0d.0 | ||
| // but we need the first substring match so we take the second element | ||
| if len(matches) != 2 { | ||
| re := regexp.MustCompile(`\b\d{4}:[a-f\d]{2}:[a-f\d]{2}\.[a-f\d]\b`) | ||
| matches := re.FindAllString(path, -1) | ||
| if len(matches) == 0 { |
There was a problem hiding this comment.
FindPCIAddress currently recompiles the regexp on every call. This function is called in device enumeration loops (e.g. net/usb), so it would be better to compile the regexp once at package scope and reuse it to avoid repeated allocations/CPU.
| name: "ARM platform bus - NIC behind bridge", | ||
| path: "/devices/platform/4080000000.pcie/pci0001:00/0001:00:00.0/0001:01:00.0", | ||
| expected: "0001:01:00.0", | ||
| }, |
There was a problem hiding this comment.
The new test cases don't cover hex letters in the PCI domain/bus (e.g. 000a:8b:00.0). Since FindPCIAddress is intended to parse PCI addresses from sysfs paths, adding a case with a-f in the domain or bus would prevent regressions in address matching.
| }, | |
| }, | |
| { | |
| name: "hex letters in PCI domain", | |
| path: "/sys/devices/pci000a:00/000a:00:14.3", | |
| expected: "000a:00:14.3", | |
| }, | |
| { | |
| name: "hex letters in PCI bus behind bridge", | |
| path: "/sys/devices/pci000a:8b/000a:8b:00.0", | |
| expected: "000a:8b:00.0", | |
| }, |
christoph-zededa
force-pushed
the
fix_network_pci_addressses
branch
2 times, most recently
from
dc04b9b to
6e7daee
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Match an exact BDF component: domain:bus:device.function (all hex). | ||
| re := regexp.MustCompile(`^[0-9a-f]{4}:[0-9a-f]{2}:[0-9a-f]{2}\.[0-9a-f]$`) | ||
| for { | ||
| dir, file := filepath.Split(path) | ||
| if re.MatchString(file) { | ||
| return file |
There was a problem hiding this comment.
FindPCIAddress only matches lowercase hex ([0-9a-f]). Other PCI address parsing in this repo lowercases input (e.g. pkg/pci/address.FromString), so a sysfs path containing uppercase A-F would incorrectly return an empty string. Consider making the match case-insensitive or lowercasing file before matching.
There was a problem hiding this comment.
@christoph-zededa this is also a good point I think
There was a problem hiding this comment.
the newest iteration checks for uppercase hex now
Some arm64 devices might have additional lines on /proc/cpuinfo containing additional information in a non "<field>: <value>" format, so these lines must be ignored. Example of a /proc/cpuinfo from a Qualcomm device: processor : 7 BogoMIPS : 38.40 Features : fp asimd evtstrm aes pmull sha1 sha2 crc32 cpuid CPU implementer : 0x51 CPU architecture: 8 CPU variant : 0xa CPU part : 0x800 CPU revision : 2 Qualcomm Technologies, Inc QCM6125 Signed-off-by: Renê de Souza Pinto <rene@renesp.com.br>
add a simple test for a cpuinfo file that has lines without ':' and therefore crashed the code. This test is based on the description of the commit with the fix Signed-off-by: Christoph Ostarek <christoph@zededa.com>
The previous regex relied on the sysfs path structure and only captured the first PCI address after the root bus, so network cards behind a PCIe bridge had no PCI address associated with their bus parent. The new implementation finds all BDF addresses in the path and returns the last (deepest) one, correctly handling both direct PCI devices and NICs sitting behind a bridge. Signed-off-by: Christoph Ostarek <christoph@zededa.com> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
christoph-zededa
force-pushed
the
fix_network_pci_addressses
branch
from
6e7daee to
add9869
Compare
The previous regex relied on the sysfs path structure and only captured the first PCI address after the root bus, so network cards behind a PCIe bridge had no PCI address associated with their bus parent. The new implementation finds all BDF addresses in the path and returns the last (deepest) one, correctly handling both direct PCI devices and NICs sitting behind a bridge.