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.

Sign up for GitHub

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

Jump to bottom

sched/spin_lock: rename raw_*lock to *_notrace #15729

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

sched/spin_lock: rename raw_*lock to *_notrace #15729

wants to merge 1 commit into from

Conversation

hujun260
Copy link
Contributor

@hujun260 hujun260 commented Jan 30, 2025 •
edited
Loading

Summary

sched/spin_lock: rename raw_*lock to *_notrace
We made the name of spinlock more meaningful and easier to understand.

Impact

spin_lock

Testing

ci

@github-actions github-actions bot added Area: OS Components OS Components issues Size: S The size of the change in this PR is small labels Jan 30, 2025
@nuttxpr
Copy link

nuttxpr commented Jan 30, 2025

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it provides a summary of the change, it lacks crucial details. Here's why:

  • Insufficient Summary: While the title describes what changed, it doesn't explain why. What problem did renaming raw_spin_lock to spin_lock_prempt solve? What was the motivation? How does this change work?

  • Incomplete Impact Assessment: Simply stating "spin_lock" doesn't provide enough information. The impact section needs to explicitly address all the points listed in the requirements: impact on the user, build, hardware, documentation, security, and compatibility. Even if the answer is "NO" for most of these, it needs to be stated explicitly.

  • Inadequate Testing: "ci" is not sufficient. The testing section requires details about the build host and target environments where the changes were tested. It also requires actual testing logs from before and after the change to demonstrate the effect of the modification. Just saying "ci" doesn't prove that the change was properly tested.

To meet the requirements, the PR needs to be significantly expanded to provide all the necessary information.

xiaoxiang781216
xiaoxiang781216 reviewed Jan 31, 2025
View reviewed changes
include/nuttx/spinlock.h
@@ -241,7 +241,7 @@ static inline_function void spin_lock(FAR volatile spinlock_t *lock)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's move sched_note to spin_lock_prempt

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

include/nuttx/spinlock.h Outdated
@@ -456,7 +456,7 @@ irqstate_t raw_spin_lock_irqsave(FAR volatile spinlock_t *lock)
irqstate_t flags;
flags = up_irq_save();

raw_spin_lock(lock);
spin_lock_prempt(lock);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

raw_spin_lock_irqsave need change to spin_lock_irqsave_prempt

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all raw_ need be renamed to _prempt

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@github-actions github-actions bot added Area: Drivers Drivers issues Area: File System File System issues Size: M The size of the change in this PR is medium and removed Size: S The size of the change in this PR is small labels Jan 31, 2025
@hujun260 hujun260 changed the title sched/spin_lock: rename raw_spin_lock to spin_lock_prempt sched/spin_lock: rename raw_*lock to *_prempt Jan 31, 2025
@tmedicci
Copy link
Contributor

@hujun260 , can you please add more information about the proposed changes?

  • The PR's description should be updated to include more information about the proposed changes, why it's being done, previous discussions, dependencies (if existing), and how to test it locally.
  • The commit itself should contain a title and a description. Let's adhere to NuttX's Contribution Guide

Can you please take a look on that?

anchao
anchao requested changes Jan 31, 2025
View reviewed changes
drivers/note/note_driver.c Outdated
@@ -1841,7 +1841,7 @@ void sched_note_filter_mode(FAR struct note_filter_named_mode_s *oldm,
irqstate_t irq_mask;
FAR struct note_driver_s **driver;

irq_mask = raw_spin_lock_irqsave(&g_note_lock);
irq_mask = spin_lock_irqsave_preempt(&g_note_lock);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

notrace not equal preempt

@hujun260
sched/spin_lock: rename raw_spin_lock to spin_lock_notrace
59ebddf 
Signed-off-by: hujun5 <hujun5@xiaomi.com>
@hujun260 hujun260 changed the title sched/spin_lock: rename raw_*lock to *_prempt sched/spin_lock: rename raw_*lock to *_notrace Feb 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xiaoxiang781216 xiaoxiang781216 xiaoxiang781216 left review comments

@anchao anchao anchao requested changes

Assignees
No one assigned
Labels
Area: Drivers Drivers issues Area: File System File System issues Area: OS Components OS Components issues Size: M The size of the change in this PR is medium
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@hujun260 @nuttxpr @tmedicci @anchao @xiaoxiang781216