sched/spin_lock: continue work to rename raw_spin* to spin_*_notrace

[anchao]

Summary

sched/spin_lock: continue work to rename raw_spin* to spin_*_notrace

Some improvements are made to the following commits:
#15767

| commit f22b93b337c2f9de457904596688a04bd1ce12e5
| Author: hujun5 <[email protected]>
| Date:   Fri Jan 31 07:01:07 2025 +0800
|
|     sched/spin_lock: rename raw_spin_lock to spin_lock_notrace
|
|     Signed-off-by: hujun5 <[email protected]>

Signed-off-by: chao an [email protected]

Impact

N/A

Testing

ci-check

[nuttxpr]

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it provides a summary of the changes, it lacks crucial details.

Here's a breakdown of the missing information:

• Insufficient Summary: While the summary mentions renaming functions, it doesn't explain why this change is necessary. What problem does it solve? What are the benefits? The linked PR provides more context, but this PR should be understandable on its own. What does "_notrace" signify?
• Missing Functional Description: The summary vaguely mentions "improvements," but doesn't detail what functional part of the code is affected. Be more specific. Is this related to scheduling? Synchronization primitives? Kernel internals?
• How it Works: The summary lacks a clear explanation of how the change works. Just stating "rename" doesn't convey the technical implications. Does this affect the behavior of spinlocks? Are there performance implications?
• Impact Section is Inadequate: "N/A" is unacceptable. Even seemingly minor changes can have impacts. Consider:
  • Impact on Build: Does this renaming require any changes to build scripts or Kconfig options?
  • Impact on Compatibility: Does this change break any existing code that uses the old function names? If not, explicitly state that.
  • Impact on Documentation: Does this change necessitate updates to the documentation?
• Testing is Insufficient: "ci-check" is not enough detail. What specific tests were run? What platforms? Provide actual log snippets showing the behavior before and after the change. Ideally, demonstrate the problem the change solves with the "before" logs.

In short: This PR needs significant improvement in terms of clarity, detail, and justification for the changes. It needs to stand alone and provide all the necessary context for a reviewer to understand and evaluate the changes effectively. Simply linking to another PR shifts the burden of understanding to the reviewer and makes it much harder to assess the validity and impact of this specific PR.

[tmedicci]

Hi @anchao ,

I agree with these changes, but we have been discussing about code quality and how PRs should be provided (and reviewed).

In that sense, please adhere to the guidelines to fill the PR's descriptions: the bot provide useful instructions to fill it here: #15862 (comment)

The most important section is the Impact. Please provide how it impacts existing builds and how this impact could be mitigated. Also, provide a summary of why this change is happening (you can link previous discussions).

In general, act as if you were a beginner on NuttX willing to help review a merge request: the MR must provide all the necessary info to it, starting by clearly pointing out the motivation for the change, and how it impacts the RTOS and a testing section.

[anchao]

Hi @anchao ,

I agree with these changes, but we have been discussing about code quality and how PRs should be provided (and reviewed).

In that sense, please adhere to the guidelines to fill the PR's descriptions: the bot provide useful instructions to fill it here: #15862 (comment)

The most important section is the Impact. Please provide how it impacts existing builds and how this impact could be mitigated. Also, provide a summary of why this change is happening (you can link previous discussions).

In general, act as if you were a beginner on NuttX willing to help review a merge request: the MR must provide all the necessary info to it, starting by clearly pointing out the motivation for the change, and how it impacts the RTOS and a testing section.

I think you should understand my original intention of submitting this patch, which is to solve the consistency problem in the current code #15767. If you don't want to approve it, just leave it pending here. I don't want 90% of my work to be about introducing what I'm doing. Open source for Nuttx is just my interest. If it becomes a burden to me, I'd rather not do it.