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

cpu/cortexm: rework bkpt instruction call on panic #20616

Merged
merged 2 commits into from
Apr 24, 2024

Conversation

dylad
Copy link
Member

@dylad dylad commented Apr 23, 2024

Contribution description

Rework the way bkpt instruction is call on panic for Cortex-M MCU.
Currently, this instruction is called everytime on panic. However, when a debug session is not active (no GDB running), and if a panic occured, the bkpt instruction will either escalate the current fault to hardfault or the CPU will enter LOCKUP state (it may reset automatically depending on how vendors handle this signal internally).

To mitigate this issue, one can check the C_DEBUGEN in CoreDebug->DHCSR. If this bit is set, it means a debug session is running, thus bkpt instruction can be used safely. Otherwise, skip this instruction. The execution will then continue until pm_off() or while(1) {} is called.

However, this bit is NOT accessible by the CPU on CortexM0+ (Only accessible by GDB). So the bkpt instruction is completely skip for this one.

Testing procedure

Trigger a fault on any cortexm-based board
for instance

volatile uint32_t *pointer = (uint32_t*) 0x60000001;
uint32_t val = *pointer; 

Such code will trigger a hardfault (technically a busfault for ARMv7-M or ARMv8-M Mainline, but it escalates to hardfault as the busfault is currently not enable).
On panic, if GDB is NOT used, the execution will 'stop' on the bkpt instruction. On nRF5340DK, a reset is issued (Probably because the processor enters LOCKUP state). Nevertheless, the execution should continue up to here.

Issues/PRs references

None.

@github-actions github-actions bot added Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: cpu Area: CPU/MCU ports labels Apr 23, 2024
@kaspar030
Copy link
Contributor

I like this idea!

@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 24, 2024
@riot-ci
Copy link

riot-ci commented Apr 24, 2024

Murdock results

✔️ PASSED

e784794 cpu/cortexm: uncrustify panic.c

Success Failures Total Runtime
10066 0 10066 12m:33s

Artifacts

Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

lgtm, thx. Please squash the nitpicks right away.

When you have the file open anyway, care to sneak in a style fix as separate commit in line 30?

-     if(ipsr) {
+     if (ipsr) {

cpu/cortexm_common/panic.c Outdated Show resolved Hide resolved
cpu/cortexm_common/panic.c Outdated Show resolved Hide resolved
Only call this instruction if a debug session is active otherwise it will trigger a hardfault

Signed-off-by: Dylan Laduranty <dylan.laduranty@mesotic.com>
Signed-off-by: Dylan Laduranty <dylan.laduranty@mesotic.com>
@dylad dylad force-pushed the pr/cpu/cortexm/rework_break_on_panic branch from 679536e to e784794 Compare April 24, 2024 11:21
@dylad
Copy link
Member Author

dylad commented Apr 24, 2024

@maribu I've addressed your comments and squashed.

@maribu maribu enabled auto-merge April 24, 2024 11:36
@maribu maribu added this pull request to the merge queue Apr 24, 2024
Merged via the queue into RIOT-OS:master with commit 05034b5 Apr 24, 2024
26 checks passed
@dylad
Copy link
Member Author

dylad commented Apr 25, 2024

Thanks!

@dylad dylad deleted the pr/cpu/cortexm/rework_break_on_panic branch April 25, 2024 06:07
@maribu
Copy link
Member

maribu commented Apr 25, 2024

Thx for the enhancement :) It will be a real quality of life improvement when the MCU no longer gets locked up :)

@mguetschow mguetschow added this to the Release 2024.07 milestone Jul 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: cpu Area: CPU/MCU ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants