ioctl: add SIOCETHTOOL#102
Conversation
There was a problem hiding this comment.
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 by @julianuziemblo.
This pull request introduces support for the SIOCETHTOOL ioctl command, allowing userspace applications to query and potentially configure Ethernet device parameters using the standard ethtool interface. This is a significant step towards providing more standard network management capabilities.
The changes involve refactoring how certain ioctls with pointer arguments are handled in the sockets layer, ensuring they are correctly unpacked in userspace. It also includes updates to the network interface driver structure to accommodate a new function pointer for handling ethtool-specific ioctls. Finally, the imx-enet driver is updated to implement this new interface and provide basic ethtool functionality, such as retrieving link settings (speed, duplex, autonegotiation, MDI/MDIX state) and potentially running PHY self-tests.
This PR is marked as a new feature and requires companion changes in the kernel and libphoenix repositories.
Highlights
- Add SIOCETHTOOL ioctl: Introduces the
SIOCETHTOOLioctl command in the sockets layer, enabling standard ethtool operations for network interfaces. - Refactor ioctl handling: Adjusts the sockets implementation to correctly handle ioctls that involve pointer arguments or variable-length data in userspace, specifically fixing the handling of
SIOCGIFCONFto comply with POSIX (allowing size queries) andSIOCADDRT/SIOCDELRTto correctly parse the device name. - Driver ethtool support: Adds a new
do_ethtool_ioctlfunction pointer to thenetif_driver_tstructure, allowing individual network drivers to provide their own ethtool command handlers. - imx-enet driver implementation: Updates the
imx-enetdriver to implement the newdo_ethtool_ioctlinterface, providing support for several ethtool commands (ETHTOOL_GSET,ETHTOOL_GSSET_INFO,ETHTOOL_GSTRINGS,ETHTOOL_TEST,ETHTOOL_GLINKSETTINGS) by querying the PHY state.
Changelog
Click here to see the changelog
- drivers/ephy.c
- Renamed
full_duplexvariable toduplexfor clarity. - Updated
ephy_setLinkStateandephy_linkSpeedto useSPEED_*andDUPLEX_*constants and handle unknown states during auto-negotiation. - Added
ephy_getAN,ephy_setAN, andephy_getMdixfunctions to retrieve/set autonegotiation state and get MDI/MDIX status from the PHY.
- Renamed
- drivers/ephy.h
- Included
phoenix/ethtool.h. - Defined
EPHY_SPEEDSmacro for ethtool advertising flags. - Updated function declarations for
ephy_linkSpeed,ephy_getAN,ephy_setAN, andephy_getMdix.
- Included
- drivers/imx-enet.c
- Included
phoenix/ethtool.h. - Changed
ENET_SELFTESTdefine from 0 to 1. - Removed the direct call to
enet_phySelfTestfromenet_netifInit. - Updated
enet_mediato useSPEED_*andDUPLEX_*constants. - Implemented
enet_ethtoolIoctlto handle various ethtool commands by querying the PHY state. - Added
enet_ethtoolIoctlto theenet_drvstructure.
- Included
- include/netif-driver.h
- Added
do_ethtool_ioctlfunction pointer to thenetif_driver_tstructure.
- Added
- port/sockets.c
- Included
netif-driver.h. - Refactored
SIOCGIFCONFhandling to correctly support querying the required buffer size (POSIX compliance). - Refactored
SIOCADDRT/SIOCDELRThandling to correctly extract the device name from the input buffer. - Added handling for the new
SIOCETHTOOLioctl, delegating the call to the specific network driver's handler.
- Included
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 calls take flight,
Ioctl commands, a key,
Driver does the work.
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. ↩
4c3fbd9 to
0d9ce09
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces SIOCETHTOOL support, which is a valuable addition for network interface inspection and control. The changes also include refactoring for SIOCGIFCONF to align better with POSIX standards and adjustments to how routing ioctls handle their data. The introduction of more descriptive enums for speed and duplex in the PHY layer (ephy.c) improves code clarity.
Overall, the changes are well-structured. I've identified a few issues that should be addressed to ensure correctness and robustness.
Summary of Findings
- Duplicate Header Include: The file
drivers/imx-enet.cincludes<phoenix/ethtool.h>twice, which is redundant and could lead to issues. - Incorrect Media Strings: The
enet_mediafunction indrivers/imx-enet.creturns swapped strings for full/half duplex modes across different speeds (10/100/1000 Mbps). - Potential Buffer Overflow in ETHTOOL_GLINKSETTINGS: In
drivers/imx-enet.c, theETHTOOL_GLINKSETTINGShandler might write beyond the allocated buffer forlink_mode_masksif the caller provides an insufficientlink_mode_masks_nwords.
Merge Readiness
The pull request introduces valuable ethtool functionality and improves existing ioctl handling. However, there are a few high and medium severity issues identified (duplicate header, incorrect media strings, potential buffer overflow) that should be addressed before merging to ensure correctness and stability. I am not authorized to approve pull requests, so please ensure these changes are reviewed and approved by the appropriate maintainers after the suggested modifications are made.
83b5c93 to
0eeb14a
Compare
0eeb14a to
473a4bd
Compare
a261fd6 to
5f704f6
Compare
77ac270 to
d3cbf78
Compare
d3cbf78 to
2fe49ee
Compare
2fe49ee to
269f31d
Compare
3bf5a8a to
bef66ef
Compare
9a99139 to
218f0e9
Compare
218f0e9 to
e5a292a
Compare
00f4943 to
c6e22ce
Compare
76a8b6b to
f7dffef
Compare
ecc2d78 to
5306708
Compare
a01abc2 to
2e1b201
Compare
2e1b201 to
dc47a03
Compare
04711f7 to
57ebfea
Compare
5306708 to
93737e4
Compare
ethernet driver can now specify a function to handle the ethtool ioctl add handling for simple ethtool commands in imx-enet JIRA: RTOS-1014
JIRA: RTOS-1014
93737e4 to
2e64b2e
Compare
Selftest is a pretty generic procedure that can be done inside other drivers too (e.g. rtl8139cp driver). JIRA: RTOS-508
JIRA: RTOS-508
TODO: For now, selftest on this target is not working. Examine why or remove this commit for now. JIRA: RTOS-508
2e64b2e to
638873c
Compare
Description
Add ethtool handling by the driver
Add simple ethtool implementations to ethernet drivers
Motivation and Context
Types of changes
How Has This Been Tested?
armv7a7-imx6ull-evk,armv7m7-imxrt117x-evkb,armv7m7-imxrt106x-evk,ia32-generic-qemuChecklist:
Special treatment