Skip to content

riscv: Some judgments are missing#15030

Merged
xiaoxiang781216 merged 1 commit into
apache:masterfrom
hujun260:apache_46
Dec 4, 2024
Merged

riscv: Some judgments are missing#15030
xiaoxiang781216 merged 1 commit into
apache:masterfrom
hujun260:apache_46

Conversation

@hujun260

@hujun260 hujun260 commented Dec 3, 2024

Copy link
Copy Markdown
Contributor

Summary

This commit fixes the regression from #14984

Impact

riscv

Testing

ostest rv-virt:knsh

@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 Dec 3, 2024
@hujun260 hujun260 mentioned this pull request Dec 3, 2024
@nuttxpr

nuttxpr commented Dec 3, 2024

Copy link
Copy Markdown

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it provides a summary and identifies the impacted architecture, it lacks crucial information.

Here's what's missing:

  • Detailed Summary: The summary needs to explain what the regression was, what functional part of the code was changed to fix it, and how the fix works. Simply referencing another PR isn't sufficient.
  • Complete Impact Assessment: While "riscv" is mentioned, the impact assessment needs to be more thorough. Even if the answer is "NO" for most categories, it should be explicitly stated (e.g., "Impact on user: NO"). It's unclear if there are build, hardware, documentation, security, or compatibility implications.
  • Comprehensive Testing Information: The testing section is inadequate. It needs to specify the build host details (OS, CPU, compiler) and provide complete target information (architecture, board, configuration). Most importantly, the "Testing logs before change" and "Testing logs after change" sections are empty. Actual logs demonstrating the issue and the fix are required.

The PR needs to be expanded to address these missing points before it can be considered complete.

@lupyuen lupyuen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Tested OK with OSTest on rv-virt:knsh, rv-virt:knsh64 and rv-virt:nsh64. Thanks! :-)

Comment thread arch/risc-v/src/common/supervisor/riscv_perform_syscall.c Outdated
Comment thread arch/risc-v/src/common/supervisor/riscv_perform_syscall.c Outdated
@hujun260 hujun260 force-pushed the apache_46 branch 2 times, most recently from 9187345 to 7a2a52a Compare December 3, 2024 11:08
Comment thread arch/risc-v/src/common/supervisor/riscv_perform_syscall.c Outdated
Comment thread arch/risc-v/src/common/supervisor/riscv_perform_syscall.c Outdated
@hujun260 hujun260 force-pushed the apache_46 branch 3 times, most recently from 00acae1 to cb4e3c2 Compare December 4, 2024 01:35
Comment thread arch/risc-v/src/common/riscv_doirq.c Outdated
Comment thread arch/risc-v/src/common/riscv_doirq.c Outdated
Comment thread arch/risc-v/src/common/riscv_doirq.c Outdated
This commit fixes the regression from apache#14984

Signed-off-by: hujun5 <hujun5@xiaomi.com>
@xiaoxiang781216 xiaoxiang781216 merged commit 3e3701b into apache:master Dec 4, 2024
@hujun260 hujun260 deleted the apache_46 branch December 5, 2024 04:23
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.

7 participants