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

Use small lock to protect resources related to irq in arch ARM. #15625

Merged
merged 1 commit into from
Jan 23, 2025

Conversation

wangzhi-art
Copy link
Contributor

Note: Please adhere to Contributing Guidelines.

Summary

Use small lock to protect resources about irq in arch ARM.

Impact

Some logic related to irq under arm architecture.

Testing

CI

@github-actions github-actions bot added Arch: arm Issues related to ARM (32-bit) architecture Size: L The size of the change in this PR is large labels Jan 21, 2025
@nuttxpr
Copy link

nuttxpr commented Jan 21, 2025

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements as described. While it provides some information, it lacks crucial details and context. Here's a breakdown of the deficiencies:

Summary:

  • Insufficient Detail: "Use small lock to protect resources about irq in arch ARM" is vague. Which resources are being protected? Why was this lock necessary (what race condition or bug did it fix)? How does the locking mechanism work (spinlock, mutex, etc.)?

Impact:

  • Too Brief: "Some logic related to irq under arm architecture" is far too general. Be specific about which logic is affected. This section requires YES/NO answers for each impact category, followed by descriptions if YES. This PR omits all of that. Consider the impact on:
    • Users: Will any user-facing APIs change? Will applications need to be modified?
    • Build: Are there any new configuration options? Do existing Kconfig files need modification?
    • Hardware: Does this change only affect specific ARM architectures or boards? Does it affect any drivers?
    • Documentation: Does this change require updates to the NuttX documentation? If so, have these updates been made?
    • Security: Does this lock mitigate a potential vulnerability?
    • Compatibility: Does this change break compatibility with any existing code or configurations?

Testing:

  • Insufficient Information: "CI" is not enough. While CI testing is important, local testing verification is required. List the specific build hosts (OS, CPU, compiler version) and targets (architecture, board, configuration) used for testing.
  • Missing Logs: The PR provides no "before" and "after" logs to demonstrate the change's effect. Include relevant log snippets showing the issue before the change and the improved behavior after the change. If the change addresses a race condition, demonstrate that the race condition no longer occurs.

In short, the PR needs significantly more detail to meet the NuttX requirements. It should clearly explain the problem being solved, the solution implemented, and the impact of the change on different aspects of the system, backed up by concrete testing results.

@wangzhi-art wangzhi-art force-pushed the lock_12 branch 3 times, most recently from 682cacc to bad22a0 Compare January 22, 2025 01:51
@github-actions github-actions bot added Size: XL The size of the change in this PR is very large. Consider breaking down the PR into smaller pieces. and removed Size: L The size of the change in this PR is large labels Jan 22, 2025
@wangzhi-art wangzhi-art force-pushed the lock_12 branch 3 times, most recently from bcaa392 to 49faf60 Compare January 22, 2025 03:27
arch/arm/src/a1x/a1x_irq.c Outdated Show resolved Hide resolved
@wangzhi-art wangzhi-art force-pushed the lock_12 branch 3 times, most recently from 5829997 to 85c4971 Compare January 22, 2025 08:08
@github-actions github-actions bot added Size: L The size of the change in this PR is large and removed Size: XL The size of the change in this PR is very large. Consider breaking down the PR into smaller pieces. labels Jan 23, 2025
@@ -41,8 +41,10 @@
* Private Data
****************************************************************************/
#ifdef CONFIG_ARCH_IRQPRIO
#if defined(CONFIG_DEBUG_IRQ_INFO)
Copy link
Contributor

Choose a reason for hiding this comment

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

merge with previous line

* Private Data
****************************************************************************/

#if defined(CONFIG_DEBUG_IRQ_INFO)
Copy link
Contributor

Choose a reason for hiding this comment

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

dup

@xiaoxiang781216 xiaoxiang781216 merged commit c754019 into apache:master Jan 23, 2025
25 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 Size: L The size of the change in this PR is large
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants