Skip to content

Conversation

@wangzhi16
Copy link
Contributor

Note: Please adhere to Contributing Guidelines.

Summary

Use small lock to protect resources related to ethernet.

Impact

The underlying implementation of Ethernet.

Testing

CI

@github-actions github-actions bot added Arch: arm Issues related to ARM (32-bit) architecture Arch: hc Issues related to HC architecture Arch: mips Issues related to the MIPS architecture Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Size: M The size of the change in this PR is medium labels Jan 23, 2025
@nuttxpr
Copy link

nuttxpr commented Jan 23, 2025

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements as described. While it provides some information, it's severely lacking in detail and completeness.

Here's a breakdown of what's missing:

  • Summary: "Use small lock to protect resources related to ethernet" is insufficient. It doesn't explain why this lock is necessary (bug fix? performance improvement? new feature enabling something else?), which resources are being protected, or how the locking mechanism is implemented.

  • Impact: "The underlying implementation of Ethernet" is vague. It must address all the specific impact points listed in the template (user impact, build impact, hardware impact, documentation, security, compatibility). Simply saying "implementation change" doesn't convey any useful information for reviewers. For almost all changes, several of these will be "NO" - but that still needs to be explicitly stated.

  • Testing: "CI" is not acceptable. Local testing is required before submitting a PR. The template specifically requests logs before and after the change to demonstrate the issue being fixed or the new functionality working. Relying solely on CI pushes the testing burden onto the project maintainers and delays the review process.

Example of a better Summary:

"Currently, access to the ethernet transmit buffer and descriptor list is not thread-safe, leading to data corruption under heavy load (see issue #123). This PR introduces a small mutex lock to protect these critical sections, ensuring exclusive access and preventing race conditions. The lock is acquired before any modifications to the buffer or descriptor list and released immediately afterwards. This change is also relevant to the nx_netdev application (see nuttx-apps PR #456)."

Example of a better Impact:

  • Impact on user: NO
  • Impact on build: NO
  • Impact on hardware: Specific to ethernet controllers. Tested on the STM32F429I-DISC1 board.
  • Impact on documentation: YES. Updated the networking documentation to describe the locking mechanism and its implications for driver developers.
  • Impact on security: Potentially YES. By preventing data corruption, this fix mitigates a possible denial-of-service vulnerability.
  • Impact on compatibility: NO.
  • Anything else to consider: Performance impact of the lock needs to be monitored.

Example of better Testing:

  • Build Host(s): Linux (Ubuntu 20.04), x86_64, GCC 9.4.0
  • Target(s): sim:nsh

Testing logs before change:

[logs showing data corruption/errors in ethernet transmission]

Testing logs after change:

[logs showing correct ethernet operation under the same load]

Signed-off-by: wangzhi16 <wangzhi16@xiaomi.com>
@xiaoxiang781216 xiaoxiang781216 merged commit 6012503 into apache:master Jan 24, 2025
20 of 39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Arch: arm Issues related to ARM (32-bit) architecture Arch: hc Issues related to HC architecture Arch: mips Issues related to the MIPS architecture Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture 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.

Crtical section should be replaced with spinlock as much as we can to improve SMP performance

3 participants