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

arch/x86_64/intel64: up_disable_irq should work from any CPU #14958

Merged
merged 1 commit into from
Nov 26, 2024

Conversation

szafonimateusz-mi
Copy link
Contributor

Summary

  • arch/x86_64/intel64: up_disable_irq should work from any CPU
    simplify interrupt logic and allow any CPU to disable interrupt, not only CPU that enabled it.

Impact

up_disable_irq works from any CPU, not just the one that previously enabled a given interrupt

Testing

qemu

simplify interrupt logic and allow any CPU
to disable interrupt, not only CPU that enabled it.

Signed-off-by: p-szafonimateusz <[email protected]>
@github-actions github-actions bot added Arch: x86_64 Issues related to the x86_64 architecture Size: S The size of the change in this PR is small labels Nov 26, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 26, 2024

[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 required by the template. Here's a breakdown of missing information:

  • Summary: While the summary explains what was changed, it doesn't fully explain why. What problem did the previous behavior cause? What benefit does this simplification bring? It also needs links to related NuttX issues or PRs if applicable.
  • Impact: The impact section is too brief. It needs to explicitly answer each question with "YES" or "NO" and provide details where necessary. For example:
    • Is new feature added? NO
    • Is existing feature changed? YES ( up_disable_irq now works from any CPU. Previously it only worked from the CPU that enabled the interrupt.)
    • Impact on user (will user need to adapt to change)? Potentially YES (If users relied on the previous behavior where interrupts could only be disabled by the enabling CPU, they may need to adjust their code. Explain how.) Or NO (if no user code will be affected, explain why).
    • Impact on build (will build process change)? NO
    • Impact on hardware (will arch(s) / board(s) / driver(s) change)? YES (x86_64 architecture impacted). Describe how it's impacted.
    • Impact on documentation (is update required / provided)? YES/NO (Explain which and why. If YES, where is the updated documentation?)
    • Impact on security (any sort of implications)? YES/NO (Explain why)
    • Impact on compatibility (backward/forward/interoperability)? Potentially YES (Explain how).
    • Anything else to consider? (Anything unexpected or noteworthy)
  • Testing: The testing section is woefully inadequate.
    • Build Host(s): Needs full details of the build host operating system, CPU architecture, and compiler version. Example: Linux, x86_64, GCC 12.1
    • Target(s): Needs more specific information. Instead of just "qemu," specify the QEMU version and the specific NuttX configuration used for testing. Example: QEMU 7.0.0, x86_64:nsh
    • Testing logs before change: Provide actual logs demonstrating the previous behavior.
    • Testing logs after change: Provide actual logs demonstrating the new behavior. The logs should clearly show that the change has the intended effect.

In short, the PR needs significantly more detail to be considered complete. Fill out all sections of the template with specific information and supporting evidence. Clear, detailed information makes it easier for reviewers to understand the change and increases the chances of the PR being accepted.

@xiaoxiang781216 xiaoxiang781216 merged commit 894b0f9 into apache:master Nov 26, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: x86_64 Issues related to the x86_64 architecture 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.

3 participants