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

xtensa/esp32[s2|c3|c6|h2]: fix sched_[lock|unlock] boot crash #15736

Merged

Conversation

tmedicci
Copy link
Contributor

Summary

  • xtensa/esp32[s2|c3|c6|h2]: fix sched_[lock|unlock] boot crash
  • Move the code to iram0 since sched_lock/sched_unlock is called in the early boot phase.

Impact

Closes #15688 fixing the boot problem

Testing

Internal CI testing + building the following defconfigs ensuring they are booting successfully:

  • esp32s2-saola-1:nsh
  • esp32c3-generic:nsh
  • esp32c6-devkitc:nsh
  • esp32h2-devkit:nsh

Move the code to iram0 since sched_lock/sched_unlock is called in
the early boot phase.
@github-actions github-actions bot added Board: risc-v Board: xtensa Size: S The size of the change in this PR is small labels Jan 31, 2025
@nuttxpr
Copy link

nuttxpr commented Jan 31, 2025

[Experimental Bot, please feedback here]

The PR mostly meets the NuttX requirements, but it is missing some crucial details. Here's a breakdown:

Strengths:

  • Clear Summary: The summary clearly states the problem, the affected area, and the solution. The link to the related issue is also provided.
  • Impact Addresses the Core Issue: Clearly states the positive impact of fixing the boot problem and references the issue it resolves.
  • Testing Specifies Targets: Lists specific defconfigs used for testing, which is helpful.

Weaknesses:

  • Incomplete Impact Assessment: The impact section needs to explicitly address all the points listed in the requirements template with "YES" or "NO" and provide descriptions where necessary. Even if the answer is "NO," it should be stated. For example:
    • Impact on user: NO
    • Impact on build: NO (or YES if something changed related to the build process).
    • Impact on hardware: YES (affects esp32[s2|c3|c6|h2] architectures).
    • Impact on documentation: NO (or YES if documentation needs updating).
    • Impact on security: NO
    • Impact on compatibility: NO (or YES, and explain if it breaks any existing compatibility).
    • Anything else to consider: NO
  • Missing Build Host Information: The testing section doesn't provide details about the build host environment. Information like the OS, CPU architecture, and compiler used is necessary for reproducibility.
  • Lack of "Before/After" Logs: The testing section claims logs are included, but they're placeholder comments. Actual logs demonstrating the issue before the change and the successful boot after the change are vital. These logs should ideally show the boot process halting before the fix and completing successfully afterward.

Recommendation:

To fully meet the requirements, the PR needs to:

  1. Complete the Impact Assessment: Fill in all the "NO/YES" sections in the impact assessment with explanations where applicable.
  2. Provide Build Host Details: Specify the OS, CPU architecture, and compiler version used for testing.
  3. Include Real Test Logs: Replace the placeholder comments with actual logs demonstrating the issue before and the successful operation after applying the change.

By addressing these points, the PR will be much clearer, easier to review, and provide better context for future maintainers.

@xiaoxiang781216 xiaoxiang781216 merged commit 08fe636 into apache:master Feb 1, 2025
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Board: risc-v Board: xtensa 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.

[BUG] Espressif devices are not booting after PR 14578
5 participants