PTP capable interface detection without ethtool (New)#2695
PTP capable interface detection without ethtool (New)#2695tomli380576 wants to merge 13 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2695 +/- ##
==========================================
+ Coverage 59.97% 60.10% +0.13%
==========================================
Files 487 489 +2
Lines 48918 49088 +170
Branches 8757 8793 +36
==========================================
+ Hits 29339 29506 +167
- Misses 18654 18655 +1
- Partials 925 927 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR adds a new checkbox-support helper module that detects whether a network interface is PTP-capable by querying ETHTOOL_GET_TS_INFO via ioctl, avoiding parsing ethtool CLI output (and issues like the ethtool v6.19+ field rename).
Changes:
- Add a new
checkbox_support.ethtool.ts_infomodule that issuesSIOCETHTOOL/ETHTOOL_GET_TS_INFOand interpretsphc_index. - Add helper logic to filter for “physical ethernet” interfaces via
/sys/class/net/...checks. - Provide a high-level
is_ptp_capable()API that mapsphc_indexto/dev/ptpX.
Reviewed changes
Copilot reviewed 2 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
checkbox-support/checkbox_support/ethtool/ts_info.py |
New ioctl-based implementation to retrieve timestamping/PHC information and determine PTP capability. |
checkbox-support/checkbox_support/ethtool/__init__.py |
Package marker for the new ethtool helper module. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fernando79513
left a comment
There was a problem hiding this comment.
Thanks a lot for the contribution!
The code looks fine. I don't get all the specifics of the ethtool structs but the utility looks ok.
I have a couple of more general questions:
Have you tested it on an actual device with PTP?
I get that calling directly the kernel witout relying on ehtool gives us more control about the test, but it also puts on us the "burden" of mantaining it every time there is a change in the kernel. Also users may be relying on the ethtool binary, so I think there is some value on testing it indirectly.
I'm okay with this solution for this very specific case, but as a general practice trying to "re-write" this kind of tools may cause us more work than adapting to the changes in the long run.
| # check for ARPHRD_ETHER | ||
| if not (sys_class_net_interface / "type").read_text().strip() == "1": | ||
| return False | ||
| # skip everything not associated with a physical device | ||
| if not (sys_class_net_interface / "device").exists(): | ||
| return False | ||
| # wifi interfaces (16.04+) | ||
| if (sys_class_net_interface / "phy80211").exists(): | ||
| return False |
There was a problem hiding this comment.
I think you can add here some extra debugging messages.
| # check for ARPHRD_ETHER | |
| if not (sys_class_net_interface / "type").read_text().strip() == "1": | |
| return False | |
| # skip everything not associated with a physical device | |
| if not (sys_class_net_interface / "device").exists(): | |
| return False | |
| # wifi interfaces (16.04+) | |
| if (sys_class_net_interface / "phy80211").exists(): | |
| return False | |
| # check for ARPHRD_ETHER | |
| if not (sys_class_net_interface / "type").read_text().strip() == "1": | |
| print("interface {} is not ARPHRD_ETHER".format(interface)) | |
| return False | |
| # skip everything not associated with a physical device | |
| if not (sys_class_net_interface / "device").exists(): | |
| print("interface {} is not a physical device".format(interface)) | |
| return False | |
| # wifi interfaces (16.04+) | |
| if (sys_class_net_interface / "phy80211").exists(): | |
| print("interface {} is a wifi device".format(interface)) | |
| return False |
Description
This PR implements a new module inside checkbox-support that can check whether an interface like
enp1s1supports PTP without relying onethtool. More functions like getting interface speeds natively are planned as well so a new directoryethtoolis added in checkbox support.This runs on ubuntu 16.04 too, albeit we don't have anything that supports PTP on 16.04.
Resolved issues
Ideally we stop parsing ethtool's text output to avoid problems like #2677 in the future.
Documentation
The comments in the code will point to the exact structs and macros we are re-implementing, but the basic idea is to copy this function from ethtool where we send the
SIOCETHTOOLioctl with an emptyethtool_ts_infothat only has.cmd = ETHTOOL_GET_TS_INFOset, wait for the kernel to fill in the data, then readphc_index. Ifphc_index == -1, that means the interface doesn't support PTP; ifphc_index >= 0, the interface supports PTP and a/dev/ptp{phc_index}device is expected to exist.Padding
There was 1 interesting footgun caught by copilot about struct padding. The
ifreqstruct in the kernel looks like this:where the ifr_ifru field is a union. This needs extra care because it actually has differently-sized union members. The largest one is
struct ifmap, which looks like:thus giving us the size of
sizeof(long)*2 + sizeof(short) + sizeof(char)*3 + 3 = 24bytes. Howeverlongandshortare not types with a fixed size, so this struct is included in the python version as well just to compute the padding size without doing brittlesizeofcalculations.Tests
Unit tests