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.
This commit is contained in:
Jeff Epler 2020-07-14 17:34:45 -05:00
parent 51b9a1aeca
commit dc74ae83da
2 changed files with 32 additions and 25 deletions

View File

@ -50,36 +50,34 @@ void common_hal_mcu_delay_us(uint32_t delay) {
static volatile uint32_t nesting_count = 0;
static uint8_t is_nested_critical_region;
static uint8_t sd_is_enabled = false;
void common_hal_mcu_disable_interrupts() {
sd_softdevice_is_enabled(&sd_is_enabled);
if (sd_is_enabled) {
if (nesting_count == 0) {
// Unlike __disable_irq(), this should only be called the first time
// "is_nested_critical_region" is sd's equivalent of our nesting count
// so a nested call would store 0 in the global and make the later
// exit call not actually reenable interrupts
//
// This only disables interrupts of priority 2 through 7; levels 0, 1,
// and 4, are exclusive to softdevice and should never be used, so
// this limitation is not important.
sd_nvic_critical_region_enter(&is_nested_critical_region);
} else {
__disable_irq();
__DMB();
nesting_count++;
}
__DMB();
nesting_count++;
}
void common_hal_mcu_enable_interrupts() {
// Don't check here if SD is enabled, because we'll crash if interrupts
// were turned off and sd_softdevice_is_enabled is called.
if (sd_is_enabled) {
sd_nvic_critical_region_exit(is_nested_critical_region);
} else {
if (nesting_count == 0) {
// This is very very bad because it means there was mismatched disable/enables so we
// crash.
reset_into_safe_mode(HARD_CRASH);
}
nesting_count--;
if (nesting_count > 0) {
return;
}
__DMB();
__enable_irq();
if (nesting_count == 0) {
// This is very very bad because it means there was mismatched disable/enables so we
// crash.
reset_into_safe_mode(HARD_CRASH);
}
nesting_count--;
if (nesting_count > 0) {
return;
}
__DMB();
sd_nvic_critical_region_exit(is_nested_critical_region);
}
void common_hal_mcu_on_next_reset(mcu_runmode_t runmode) {

View File

@ -313,12 +313,21 @@ void port_sleep_until_interrupt(void) {
// instruction will returned as long as an interrupt is
// available, even though the actual handler won't fire until
// we re-enable interrupts.
common_hal_mcu_disable_interrupts();
//
// We do not use common_hal_mcu_disable_interrupts here because
// we truly require that interrupts be disabled, while
// common_hal_mcu_disable_interrupts actually just masks the
// interrupts that are not required to allow the softdevice to
// function (whether or not SD is enabled)
int nested = __get_PRIMASK();
__disable_irq();
if (!tud_task_event_ready()) {
__DSB();
__WFI();
}
common_hal_mcu_enable_interrupts();
if (!nested) {
__enable_irq();
}
}
}