Skip to content
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

Various improvements for e1000 and igc #14953

Merged
merged 5 commits into from
Nov 26, 2024

Conversation

szafonimateusz-mi
Copy link
Contributor

@szafonimateusz-mi szafonimateusz-mi commented Nov 26, 2024

Summary

  • drivers/net/{e1000|igc}: configure RX/TX descriptors from Kconfig
    configure E1000/IGC RX/TX descriptors from Kconfig
  • drivers/net/igc: make Interrupt Throttle configurable
  • drivers/net/{e1000|igc}: fix link status crash
    netdev_lower_carrier_xxx API can't be used in interrupt context
  • drivers/net/{e1000|igc}: reset TX ring when disconnected
  • drivers/net/{e1000|igc}: limit no packet is transmit after carrier off

Impact

  1. moved some hardcoded values to Kconfig
  2. fixed various issues when repeatedly pluggin and unplugging the eth cable

Testing

intel hardware

szafonimateusz-mi and others added 5 commits November 26, 2024 12:44
configure E1000/IGC RX/TX descriptors from Kconfig

Signed-off-by: p-szafonimateusz <[email protected]>
make Interrupt Throttle configurable for igc

Signed-off-by: p-szafonimateusz <[email protected]>
netdev_lower_carrier_xxx API can't be used in interrupt context

Signed-off-by: p-szafonimateusz <[email protected]>
drivers/net/{e1000|igc}: reset TX ring when disconnected

Signed-off-by: p-szafonimateusz <[email protected]>
if the drvier tx queue is full up during the network cable unplugging,
there will be no txdone interrupt after inserting the network cable,
transmit cannot be recovered.

Modified to no longer fill the driver with packet when link down.

Signed-off-by: zhanghongyu <[email protected]>
@github-actions github-actions bot added Area: Networking Effects networking subsystem Size: M The size of the change in this PR is medium labels Nov 26, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 26, 2024

[Experimental Bot, please feedback here]

Yes, this PR appears to meet the basic NuttX requirements, but could be improved with more detail. Here's a breakdown:

Strengths:

  • Summary: Provides a decent overview of the changes, touching upon the "what" and "why."
  • Impact: Identifies key impacts (configuration changes, bug fixes).
  • Testing: Confirms testing was done on Intel hardware.

Weaknesses & Areas for Improvement:

  • Summary: Lacks sufficient detail on how the changes work. More explanation is needed for each bullet point, particularly the fixes. What was the exact nature of the crash? How was the TX ring reset implemented? What was the limitation applied to prevent transmissions after carrier off? Linking to a related NuttX issue would greatly strengthen this section.
  • Impact: Needs more specific YES/NO answers to the listed questions. For example:
    • Impact on user: YES (Users can now configure RX/TX descriptors via Kconfig)
    • Impact on build: Potentially YES (If Kconfig options are added, users might need to reconfigure). Explain what configurations are impacted.
    • Impact on hardware: YES (Specifically affects Intel network drivers - e1000 and igc).
    • Impact on documentation: Potentially YES (If Kconfig options are added, documentation should be updated). Explicitly state if documentation updates were included.
    • Impact on security: NO (or YES if applicable – explain why).
    • Impact on compatibility: Potentially YES (if changing descriptor counts could affect existing users).
  • Testing: Severely lacking detail.
    • Build Host(s): Be specific. "Linux" is insufficient. Provide distro, version, kernel version, compiler version (e.g., "Ubuntu 22.04, Linux 5.15, GCC 11.2.0").
    • Target(s): Which Intel hardware? Be precise (e.g., "Intel i210, using the xyz board configuration").
    • Logs: Absolutely essential to include before and after logs demonstrating the fixes. Showing the crash before and its absence after, the TX ring behavior, etc., is crucial for reviewers. Without these, the PR is difficult to evaluate properly.

In short: The PR provides a starting point, but needs significant expansion, particularly in the Impact and Testing sections, to meet the NuttX standards fully. Providing clear, specific information is key to a successful review process.

@xiaoxiang781216 xiaoxiang781216 merged commit ba419cc into apache:master Nov 26, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Networking Effects networking subsystem Size: M The size of the change in this PR is medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants