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

arm: remove up_set_current_regs/up_current_regs #14865

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

Conversation

hujun260
Copy link
Contributor

Summary

remove up_set_current_regs/up_current_regs
reason:
up_set_current_regs initially had two functions:

1: To mark the entry into an interrupt state.
2: To record the context before an interrupt/exception. If we switch to
a new task, we need to store the upcoming context regs by calling up_set_current_regs(regs).

Currently, we record the context in other ways, so the second function is obsolete. Therefore, we need to rename up_set_current_regs to better reflect its actual meaning, which is solely to mark an interrupt.

Impact

arm arch

Testing

ostest
qemu
qemu-armv7a:smp
mps2-an500:nsh

@github-actions github-actions bot added Arch: arm Issues related to ARM (32-bit) architecture Board: arm Size: L The size of the change in this PR is large labels Nov 20, 2024
arch/arm/include/arm/irq.h Outdated Show resolved Hide resolved
arch/arm/include/arm/irq.h Outdated Show resolved Hide resolved
arch/arm/include/armv7-a/irq.h Outdated Show resolved Hide resolved
@nuttxpr
Copy link

nuttxpr commented Nov 20, 2024

[Experimental Bot, please feedback here]

Yes, this PR appears to meet the basic NuttX requirements, although some sections could be more thoroughly filled out. Specifically:

  • Summary: While the reasoning is provided, a clear statement of what up_set_current_regs is being renamed to is missing. This is crucial information.
  • Impact: While "arm arch" is mentioned, this needs to be more specific. Which ARM architectures? Are all ARM architectures affected? Also, explicitly answer the provided impact questions (user, build, hardware, documentation, security, compatibility). Even if the answer is "NO", stating it explicitly avoids ambiguity. For example, will this change require any documentation updates?
  • Testing: While target platforms are listed, the "Build Host(s)" information is missing. Also, the "Testing logs before change" and "Testing logs after change" sections are empty. These sections are crucial for demonstrating that the change works as intended and doesn't introduce regressions. Include actual log output, even if it's just a simple "ostest passed" message.

Recommendation: Revise the PR description to address the missing information highlighted above. A more complete description will make it easier for reviewers to understand and approve the changes.

@hujun260 hujun260 force-pushed the apache_18 branch 3 times, most recently from b317c40 to 8e07294 Compare November 20, 2024 13:46
@@ -148,11 +148,14 @@ uint32_t *arm_dataabort(uint32_t *regs, uint32_t dfar, uint32_t dfsr)

uint32_t *arm_dataabort(uint32_t *regs, uint32_t dfar, uint32_t dfsr)
{
struct tcb_s *tcb = this_task();

/* Save the saved processor context in current_regs where it can be
Copy link
Contributor

Choose a reason for hiding this comment

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

Update the comment (current_regs is wrong). See if current_regs is mentioned elsewhere too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

reason:
up_set_current_regs initially had two functions:

1: To mark the entry into an interrupt state.
2: To record the context before an interrupt/exception. If we switch to
   a new task, we need to store the upcoming context regs by calling up_set_current_regs(regs).

Currently, we record the context in other ways, so the second function is obsolete.
Therefore, we need to rename up_set_current_regs to better reflect its actual meaning,
which is solely to mark an interrupt.

Signed-off-by: hujun5 <[email protected]>
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 Board: arm 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