Skip to content

Commit dc74ae8

Browse files
jeplerjepler
committed
nRF: Always use sd_nvic_critical_region calls
The motivation for doing this is so that we can allow common_hal_mcu_disable_interrupts in IRQ context, something that works on other ports, but not on nRF with SD enabled. This is because when SD is enabled, calling sd_softdevice_is_enabled in the context of an interrupt with priority 2 or 3 causes a HardFault. We have chosen to give the USB interrupt priority 2 on nRF, the highest priority that is compatible with SD. Since at least SoftDevice s130 v2.0.1, sd_nvic_critical_region_enter/exit have been implemented as inline functions and are safe to call even if softdevice is not enabled. Reference kindly provided by danh: https://devzone.nordicsemi.com/f/nordic-q-a/29553/sd_nvic_critical_region_enter-exit-missing-in-s130-v2 Switching to these as the default/only way to enable/disable interrupts simplifies things, and fixes several problems and potential problems: * Interrupts at priority 2 or 3 could not call common_hal_mcu_disable_interrupts because the call to sd_softdevice_is_enabled would HardFault * Hypothetically, the state of sd_softdevice_is_enabled could change from the disable to the enable call, meaning the calls would not match (__disable_irq() could be balanced with sd_nvic_critical_region_exit). This also fixes a problem I believe would exist if disable() were called twice when SD is enabled. There is a single "is_nested_critical_region" flag, and the second call would set it to 1. Both of the enable() calls that followed would call critical_region_exit(1), and interrupts would not properly be reenabled. In the new version of the code, we use our own nesting_count value to track the intended state, so now nested disable()s only call critical_region_enter() once, only updating is_nested_critical_region once; and only the second enable() call will call critical_region_exit, with the right value of i_n_c_r. Finally, in port_sleep_until_interrupt, if !sd_enabled, we really do need to __disable_irq, rather than using the common_hal_mcu routines; the reason why is documented in a comment.
1 parent 51b9a1a commit dc74ae8

2 files changed

Lines changed: 32 additions & 25 deletions

File tree

‎ports/nrf/common-hal/microcontroller/__init__.c‎

Lines changed: 21 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -50,36 +50,34 @@ void common_hal_mcu_delay_us(uint32_t delay) {
5050

5151
static volatile uint32_t nesting_count = 0;
5252
static uint8_t is_nested_critical_region;
53-
static uint8_t sd_is_enabled = false;
5453
void common_hal_mcu_disable_interrupts() {
55-
sd_softdevice_is_enabled(&sd_is_enabled);
56-
if (sd_is_enabled) {
54+
if (nesting_count == 0) {
55+
// Unlike __disable_irq(), this should only be called the first time
56+
// "is_nested_critical_region" is sd's equivalent of our nesting count
57+
// so a nested call would store 0 in the global and make the later
58+
// exit call not actually reenable interrupts
59+
//
60+
// This only disables interrupts of priority 2 through 7; levels 0, 1,
61+
// and 4, are exclusive to softdevice and should never be used, so
62+
// this limitation is not important.
5763
sd_nvic_critical_region_enter(&is_nested_critical_region);
58-
} else {
59-
__disable_irq();
60-
__DMB();
61-
nesting_count++;
6264
}
65+
__DMB();
66+
nesting_count++;
6367
}
6468

6569
void common_hal_mcu_enable_interrupts() {
66-
// Don't check here if SD is enabled, because we'll crash if interrupts
67-
// were turned off and sd_softdevice_is_enabled is called.
68-
if (sd_is_enabled) {
69-
sd_nvic_critical_region_exit(is_nested_critical_region);
70-
} else {
71-
if (nesting_count == 0) {
72-
// This is very very bad because it means there was mismatched disable/enables so we
73-
// crash.
74-
reset_into_safe_mode(HARD_CRASH);
75-
}
76-
nesting_count--;
77-
if (nesting_count > 0) {
78-
return;
79-
}
80-
__DMB();
81-
__enable_irq();
70+
if (nesting_count == 0) {
71+
// This is very very bad because it means there was mismatched disable/enables so we
72+
// crash.
73+
reset_into_safe_mode(HARD_CRASH);
8274
}
75+
nesting_count--;
76+
if (nesting_count > 0) {
77+
return;
78+
}
79+
__DMB();
80+
sd_nvic_critical_region_exit(is_nested_critical_region);
8381
}
8482

8583
void common_hal_mcu_on_next_reset(mcu_runmode_t runmode) {

‎ports/nrf/supervisor/port.c‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -313,12 +313,21 @@ void port_sleep_until_interrupt(void) {
313313
// instruction will returned as long as an interrupt is
314314
// available, even though the actual handler won't fire until
315315
// we re-enable interrupts.
316-
common_hal_mcu_disable_interrupts();
316+
//
317+
// We do not use common_hal_mcu_disable_interrupts here because
318+
// we truly require that interrupts be disabled, while
319+
// common_hal_mcu_disable_interrupts actually just masks the
320+
// interrupts that are not required to allow the softdevice to
321+
// function (whether or not SD is enabled)
322+
int nested = __get_PRIMASK();
323+
__disable_irq();
317324
if (!tud_task_event_ready()) {
318325
__DSB();
319326
__WFI();
320327
}
321-
common_hal_mcu_enable_interrupts();
328+
if (!nested) {
329+
__enable_irq();
330+
}
322331
}
323332
}
324333

0 commit comments

Comments
 (0)