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

spinlock: use spin_lock_init replace spin_initialize #14931

Merged
merged 1 commit into from
Nov 25, 2024

Conversation

hujun260
Copy link
Contributor

Summary

spinlock: use spin_lock_init replace spin_initialize
reason:
1: spin_lock_init and spin_initialize have similar functionalities.
2: spin_lock and spin_unlock should be called in matching pairs.

Impact

spinlock

Testing

ci ostest

reason:
1: spin_lock_init and spin_initialize have similar functionalities.
2: spin_lock and spin_unlock should be called in matching pairs.

Signed-off-by: hujun5 <[email protected]>
@github-actions github-actions bot added Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Arch: xtensa Issues related to the Xtensa architecture Area: Memory Management Memory Management issues Area: OS Components OS Components issues Size: S The size of the change in this PR is small labels Nov 25, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 25, 2024

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it provides some information, it lacks crucial details.

Here's a breakdown of what's missing:

  • Summary: While it states what was changed, it doesn't clearly explain why this change is necessary beyond superficial similarity. What problem does using spin_lock_init solve? Does spin_initialize have a bug? Is this a performance improvement? Is it a stylistic change for consistency? The rationale is weak. The mention of "spin_lock and spin_unlock should be called in matching pairs" is irrelevant to the stated change.
  • Impact: Simply stating "spinlock" is insufficient. It needs to address all the impact points explicitly (user, build, hardware, documentation, security, compatibility). Even if the answer is "NO" for most, it needs to be stated.
  • Testing: "ci ostest" is not sufficient testing information. What specific tests were run? What were the actual logs before and after the change? The provided code blocks are empty. It also needs information on the build host and target used for testing.

In short, the PR needs to be significantly more detailed and explicit to meet the NuttX requirements. It needs to provide a clear and compelling rationale for the change and demonstrate, with evidence, that the change works as intended and doesn't introduce regressions.

@xiaoxiang781216 xiaoxiang781216 merged commit 34e79f9 into apache:master Nov 25, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Arch: xtensa Issues related to the Xtensa architecture Area: Memory Management Memory Management issues Area: OS Components OS Components issues Size: S The size of the change in this PR is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants