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

riscv: add a return value to riscv_swint #13564

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

Conversation

hujun260
Copy link
Contributor

Summary

add a return value to riscv_swint indicating whether a context switch is required

This commit fixes the regression from #13561

Impact

Testing

ostest

@github-actions github-actions bot added Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Size: S The size of the change in this PR is small labels Sep 21, 2024
@nuttxpr
Copy link

nuttxpr commented Sep 21, 2024

[Experimental Bot, please feedback here]

Yes, the PR appears to meet the NuttX requirements.

Here's a breakdown:

  • Summary: Clearly explains the "why" (fixing a regression), "what" (adding a return value to a specific function), and "how" (referencing the original PR that caused the regression). It also links to the original PR.
  • Impact: While lacking details, it implies minimal impact since it's a fix for a regression. More specifics about the impact (e.g., on compatibility) would strengthen this section.
  • Testing: Provides basic testing information (platform: ostest). Including testing logs before and after the change would significantly improve the thoroughness of this section.

Recommendations for Improvement:

  • Impact: Expand on the potential impact areas. Even if the impact is minimal, explicitly stating this helps reviewers. For example:
    • Compatibility: "This change only affects internal scheduling logic and has no backward or forward compatibility impact."
    • Hardware/Build: "No changes to hardware or build process are required."
  • Testing:
    • Include actual "before" and "after" testing logs to demonstrate the fix.
    • If possible, test on more than one platform or configuration to increase confidence.

Overall: The PR provides a good starting point. Adding the recommended details will ensure a smoother review process and increase the likelihood of a quick merge.

Copy link
Member

@lupyuen lupyuen left a comment

Choose a reason for hiding this comment

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

OSTest seems to hang at vfork() for rv-virt:knsh? https://gist.github.com/lupyuen/8daee1123a52e222fb601287a00af149

$ git clone https://github.com/hujun260/nuttx --branch apache_10 nuttx
$ tools/configure.sh rv-virt:knsh
...
$ qemu-system-riscv32 -semihosting -M virt,aclint=on -cpu rv32 -kernel nuttx -bios opensbi-1.5-rv-bin/share/opensbi/ilp32/generic/firmware/fw_dynamic.bin -nographic
QEMU emulator version 9.0.2
OpenSBI v1.5
...
nsh> ostest
user_main: vfork() test
[Hangs here]

Update: rv-virt:knsh64 also hangs at vfork(): https://gist.github.com/lupyuen/ce2e2cb328073d25743adbd4bc4a9b87

… switch is required

This commit fixes the regression from apache#13561

Signed-off-by: hujun5 <[email protected]>
@hujun260
Copy link
Contributor Author

OSTest seems to hang at vfork() for rv-virt:knsh? https://gist.github.com/lupyuen/8daee1123a52e222fb601287a00af149

$ git clone https://github.com/hujun260/nuttx --branch apache_10 nuttx
$ tools/configure.sh rv-virt:knsh
...
$ qemu-system-riscv32 -semihosting -M virt,aclint=on -cpu rv32 -kernel nuttx -bios opensbi-1.5-rv-bin/share/opensbi/ilp32/generic/firmware/fw_dynamic.bin -nographic
QEMU emulator version 9.0.2
OpenSBI v1.5
...
nsh> ostest
user_main: vfork() test
[Hangs here]

Update: rv-virt:knsh64 also hangs at vfork(): https://gist.github.com/lupyuen/ce2e2cb328073d25743adbd4bc4a9b87

i fixed this issue

Copy link
Member

@lupyuen lupyuen left a comment

Choose a reason for hiding this comment

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

OSTest is now OK on rv-virt:knsh, rv-virt:knsh64 and milkv_duos:nsh thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) 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