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/esp32s3: Fix esp32s3 sched_lock crash #15691

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

Conversation

hujun260
Copy link
Contributor

@hujun260 hujun260 commented Jan 25, 2025

Summary

move the code to iram0 since sched_lock/sched_unlock is called in the early boot phase

Impact

xtensa

Testing

esp32s3-devkit:smp

@github-actions github-actions bot added Arch: arm Issues related to ARM (32-bit) architecture Arch: sparc Issues related to the SPARC architecture Arch: xtensa Issues related to the Xtensa architecture Size: S The size of the change in this PR is small labels Jan 25, 2025
@nuttxpr
Copy link

nuttxpr commented Jan 25, 2025

[Experimental Bot, please feedback here]

Yes, this PR appears to meet the basic NuttX requirements, but it is incomplete. While it provides a summary of the change, its impact, and some testing information, it lacks crucial details.

Missing Information/Weaknesses:

  • Summary: While the summary explains the why, it lacks sufficient detail on what precisely changed in the code. "Use raw_spin_[un]lock to replace spin_[un]lock" is a high-level description. More detail on the specific code modifications is needed.
  • Impact: Specifying "xtensa" is too broad. List the specific xtensa boards affected. Explain the original regression more clearly (what functionality was broken?). Address all impact points (user, build, hardware, documentation, security, compatibility). Even if the answer is "NO," explicitly state it for each. The current PR only addresses hardware impact.
  • Testing: The testing section is inadequate. "esp32s3-devkit:smp" is insufficient. Provide:
    • Complete build information: Host OS, compiler version, NuttX configuration.
    • Clearer test procedure: What steps were taken to test? How was the fix validated?
    • Actual logs: The placeholders for "Testing logs before change" and "Testing logs after change" need to be filled with real log output demonstrating the issue and the fix. Simply stating that something "works as intended" isn't convincing. Show evidence.
  • Incomplete Fix Acknowledgment: The statement "This patch hasn't completely resolved the boot-up issue" is a major red flag. A PR should generally fix the issue it claims to address. If it doesn't fully resolve the problem, explain why and what further work is needed. This might warrant splitting the fix into multiple, smaller PRs.

Recommendation:

The PR needs substantial revision before it can be considered for merging. Provide the missing information outlined above to make it complete and convincing. If the fix is indeed incomplete, clearly explain the next steps and consider breaking down the work into smaller, manageable PRs.

@xiaoxiang781216 xiaoxiang781216 linked an issue Jan 25, 2025 that may be closed by this pull request
1 task
@xiaoxiang781216
Copy link
Contributor

@eren-terzioglu could you verify this patch with espressif/esp-hal-3rdparty#8?

@hujun260
Copy link
Contributor Author

@eren-terzioglu could you verify this patch with espressif/esp-hal-3rdparty#8?

@eren-terzioglu

My verification steps are as follows:

make distclean -j25;./tools/configure.sh -E esp32s3-devkit:smp;make -j25
cd ./arch/xtensa/src/esp32s3/esp-hal-3rdparty
gh pr checkout -f 8
cd -
make -j25;make flash ESPTOOL_PORT=/dev/ttyUSB0 ESPTOOL_BINDIR=/home/hujun5/work/ssd/esp-bins

@eren-terzioglu
Copy link
Contributor

eren-terzioglu commented Jan 27, 2025

Hi,

Checking it in our internal CI. I will notify the result.

Update: I need to be sure about a some defconfigs, so tests are in progress. fyi

@eren-terzioglu
Copy link
Contributor

eren-terzioglu commented Jan 29, 2025

Checking finished, you can merge this if it is good for you too @xiaoxiang781216.

Thanks for your effort @hujun260.

arch/arm/src/cxd56xx/cxd56_cpustart.c Outdated Show resolved Hide resolved
@xiaoxiang781216
Copy link
Contributor

@hujun260 let's drop the spinlock change and use #15728 instead.

@tmedicci
Copy link
Contributor

tmedicci commented Jan 30, 2025

@hujun260 let's drop the spinlock change and use #15728 instead.

Please. I tested this PR without 9a61a1d + #15728 and it seems it solves #15688

@xiaoxiang781216
Copy link
Contributor

xiaoxiang781216 commented Jan 30, 2025

@hujun260 let's drop the spinlock change and use #15728 instead.

Please. I tested this PR without 9a61a1d + #15728 and it seems it solves #15688

Thanks @tmedicci, I think the better fix is that:

  1. Remove the bad spin lock usage by arch: Remove the abuse of spinlock in smp boot #15728
  2. Move sched_lock/sched_unlock to iram0 by updating this pr
  3. Close esp32: replace spin_lock_irqsave with raw_spin_lock_irqsave. espressif/esp-hal-3rdparty#8

So, I drop 9a61a1d from this pr, both pr is ready for merging now.

move the code to iram0 since sched_lock/sched_unlock
is called in the early boot phase

Signed-off-by: hujun5 <[email protected]>
@github-actions github-actions bot added Size: XS The size of the change in this PR is very small and removed Arch: arm Issues related to ARM (32-bit) architecture Arch: sparc Issues related to the SPARC architecture Arch: xtensa Issues related to the Xtensa architecture Size: S The size of the change in this PR is small labels Jan 30, 2025
@xiaoxiang781216 xiaoxiang781216 changed the title arch: use raw_spin_[un]lock to replace spin_[un]lock, fix regression b69111d16a2a330fa272af8175c832e08881844b of https://github.com/apache/nuttx/pull/14578 xtensa/esp32s3: Fix esp32s3 sched_lock crash Jan 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Board: xtensa Size: XS The size of the change in this PR is very small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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