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:g_current_regs is only used to determine if we are in irq, #15325

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

Conversation

xianglyc
Copy link
Contributor

with other functionalities removed.

Change-Id: Ic4bdf0e28c2cc0ec462a7c941f7a7cf2b1b53313

Note: Please adhere to Contributing Guidelines.

Summary

g_current_regs is only used to determine if we are in irq,
with other functionalities removed.
Update this section with information on why change is necessary,
what it exactly does and how, if new feature shows up, provide
references (dependencies, similar problems and solutions), etc.

Impact

Update this section, where applicable, on how change affects users,
build process, hardware, documentation, security, compatibility, etc.

no

Testing

Update this section with details on how did you verify the change,
what Host was used for build (OS, CPU, compiler, ..), what Target was
used for verification (arch, board:config, ..), etc. Providing build
and runtime logs from before and after change is highly appreciated.

ostest

@github-actions github-actions bot added Arch: x86_64 Issues related to the x86_64 architecture Size: M The size of the change in this PR is medium labels Dec 24, 2024
@xianglyc xianglyc force-pushed the determine branch 4 times, most recently from c6170f1 to 8048158 Compare December 24, 2024 08:37
@nuttxpr
Copy link

nuttxpr commented Dec 24, 2024

[Experimental Bot, please feedback here]

This PR does not fully meet the NuttX requirements. While it provides some information, it lacks crucial details and context. Here's a breakdown:

Missing/Insufficient Information:

  • Summary: The summary is extremely brief and doesn't adequately explain the why behind the change. Simply stating that g_current_regs is only used to determine if we're in an IRQ and that other functionalities are removed isn't enough. What were the other functionalities? Why were they removed? What problem did this solve or what improvement did it create? What are the benefits of this change?
  • Impact: The impact section is almost entirely empty. While it says "no" seemingly in reference to adding a new feature, it needs to explicitly address all the points: impact on user, build, hardware, documentation, security, compatibility. Even if the answer is "NO" for each, it should be stated explicitly. If there is an impact, it needs to be described.
  • Testing: The testing section is severely lacking. It only mentions "ostest". This doesn't provide any useful information. What is "ostest"? What are the test results? It's crucial to provide:
    • Build Host Details: Operating System (e.g., Linux Ubuntu 20.04), CPU architecture (e.g., x86_64), Compiler (e.g., GCC 10.2.0).
    • Target Details: Architecture (e.g., ARM Cortex-M4), Board and Configuration (e.g., STM32F4 Discovery).
    • Logs: "Before" and "After" logs demonstrating the change's effect. These logs should be relevant to the functionality being modified and show that the intended behavior is achieved.

Recommendations for Improvement:

  1. Expand the Summary: Explain the rationale behind the change. Justify the removal of functionalities. Highlight the benefits (e.g., reduced code size, improved performance, bug fix). Provide context.
  2. Complete the Impact Section: Explicitly address all impact points, even if the answer is "NO". If there is an impact, describe it in detail.
  3. Provide Detailed Testing Information: Specify the build host and target environments completely. Include relevant "Before" and "After" logs that demonstrate the change's effect and verify that it works as intended.
  4. Consider adding a section on how to reproduce the bug if this is a bug fix.

Example of an Improved Testing Section:

## Testing

I confirm that changes are verified on my local setup and work as intended:

* **Build Host:** Linux Ubuntu 22.04, x86_64, GCC 11.2.0
* **Target:** ARM Cortex-M4, STM32F4 Discovery, `nsh` configuration

**Testing logs before change:**

nsh> some_command # Command that uses the removed functionality
Segmentation fault


**Testing logs after change:**

nsh> some_command
... (expected output showing the correct behavior without the removed functionality) ...
nsh> irqinfo # showing the irq state is still accessible
... (output showing interrupt information) ...

By providing more detail and following the NuttX PR requirements, the review process will be much smoother, and the changes will be more likely to be accepted.

@acassis acassis requested a review from raiden00pl December 25, 2024 07:03
with other functionalities removed.

Signed-off-by: liwenxiang1 <[email protected]>
int cpu = this_cpu();

tcb = current_task(cpu);
current_task(cpu);
Copy link
Contributor

Choose a reason for hiding this comment

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

why add this

nxsched_smp_call_handler(irq, c, arg);
tcb = current_task(cpu);
x86_64_restorestate(tcb->xcp.regs);
current_task(cpu);
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

int cpu = this_cpu();

nxsched_process_delivered(cpu);
tcb = current_task(cpu);
x86_64_restorestate(tcb->xcp.regs);
current_task(cpu);
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

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: M The size of the change in this PR is medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants