-
Notifications
You must be signed in to change notification settings - Fork 9
add ethtool port #102
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
add ethtool port #102
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @julianuziemblo, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
Hello! Gemini or gemini-code-assist here, providing a summary of this pull request.
This pull request, authored by julianuziemblo, aims to add a port of the ethtool command-line utility to the repository. The primary motivation is to provide a tool for testing and configuring Ethernet drivers, specifically mentioning its utility on armv7a7-imx6ull-evk, armv7m7-imxrt106x-evk, and armv7m7-imxrt1170-evkb boards.
The changes involve integrating the ethtool build process into the main build system and providing the necessary build script and patches to compile and install ethtool for the target environment. The patch specifically addresses compatibility issues by conditionally including Phoenix-RTOS specific headers and adjusting symbol names where they differ from the standard Linux kernel headers that ethtool typically expects.
Highlights
- New Port: ethtool: Adds the
ethtoolcommand-line utility as a new port. This tool is essential for querying and controlling network driver and hardware settings. - Build System Integration: Includes a new build script (
ethtool/build.sh) that handles downloading the ethtool source (version 6.14), applying necessary patches, configuring the build usingautogen.shandconfigure, compiling, and installing the binary. - Phoenix-RTOS Compatibility Patch: Introduces a patch (
ethtool/patches/01-replace-symbols.patch) that modifies the ethtool source code to be compatible with the Phoenix-RTOS environment. This involves using conditional compilation (#ifdef __phoenix__) to include Phoenix-specific headers and adjust struct member names and definitions (likeAF_NETLINK,ether_shost,ether_dhost,ether_type) that differ from standard Linux headers.
Changelog
Click here to see the changelog
- build.sh
- Added 'ethtool' to the list of available ports.
- ethtool/build.sh
- Added a new build script for the ethtool port.
- Handles downloading ethtool source version 6.14.
- Applies patches from the
patches/directory. - Runs
autogen.shandconfigurewith appropriate flags for cross-compilation and disabling netlink. - Builds, installs, strips, and copies the
ethtoolbinary to the target program directory.
- ethtool/patches/01-replace-symbols.patch
- Added a new patch file.
- Modifies
ethtool.c,internal.h,rxclass.c,uapi/linux/net_tstamp.h,uapi/linux/netlink.h, anduapi/linux/types.h. - Adds
#ifdef __phoenix__blocks to include Phoenix-specific headers and define/redefine symbols for compatibility. - Adjusts struct member access for Ethernet flow specs in
rxclass.c(h_dest->ether_dhost,h_source->ether_shost,h_proto->ether_type) under the Phoenix build.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Network tool arrives,
Check the speed, the link, the stats,
Ethernet now seen.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a port for the ethtool command-line utility, which is a valuable addition for testing and configuring ethernet drivers on Phoenix-RTOS. The overall approach, including the build script and the patch for adapting ethtool to the Phoenix-RTOS environment, is generally well-structured.
I've identified a critical issue in the build script and a couple of medium-severity points that would benefit from clarification or refinement to ensure robustness and maintainability. The patch file itself appears to make necessary adaptations for the Phoenix-RTOS environment.
Summary of Findings
- Build Script Typo: A critical typo (
PREFIX_ETHTOOL_SCRinstead ofPREFIX_ETHTOOL_SRC) inethtool/build.shneeds correction as it likely affects the configure step. - Download Robustness: The download logic in
ethtool/build.shexits on primary URL failure without attempting the mirror, despite a TODO for mirror support. This should be improved for build resilience. - Build Process Clarity: The
cpcommand aftermake installinethtool/build.shmight be redundant or copying from an incorrect intermediate location. Its necessity should be reviewed. - TODO: Mirror URL Configuration: The
ethtool/build.shscript has a TODO comment (line 7) to add a mirror URL (PKG_MIRROR_URL). This was not commented on directly due to review settings (low severity). - TODO: Mirror Fallback Implementation: The
ethtool/build.shscript has a TODO comment (line 21) related to implementing the mirror fallback logic. This was not commented on directly due to review settings (low severity), though the surrounding logic was flagged as medium severity.
Merge Readiness
This pull request makes a good step towards adding ethtool support. However, due to a critical typo in the build script and a couple of medium-severity issues that need addressing for robustness and clarity, I recommend that these changes be made before merging. As a reviewer, I am not authorized to approve pull requests, so please ensure further review and approval from authorized maintainers after addressing the feedback. No specific style guide was provided, so feedback is based on general shell scripting best practices and C porting conventions.
7a52fd1 to
0881afc
Compare
0881afc to
d84b877
Compare
d84b877 to
0e086e6
Compare
0e086e6 to
0120ca7
Compare
0120ca7 to
148921e
Compare
207e43d to
d8a6758
Compare
d8a6758 to
999c693
Compare
999c693 to
cebb260
Compare
cebb260 to
d87156c
Compare
d87156c to
1a36e2c
Compare
JIRA: RTOS-1064
JIRA: RTOS-1064
1a36e2c to
6089bb6
Compare
Description
Add port of
ethtoolcmdline utility for testing and configuring ethernet drivers.Motivation and Context
Will be useful in Ethernet test on armv7a7-imx6ull-evk, armv7m7-imxrt106x-evk, armv7m7-imxrt1170-evkb
Types of changes
How Has This Been Tested?
armv7m7-imxrt106x-evk,ia32-generic-qemuChecklist:
Special treatment