Mbed-os: NRF51_DK: core_util_are_interrupts_enabled wrong behavior when inside critical section

[maciejbocianski]

Description

• Type: Bug
• Priority: Major

---

Bug

Target
NRF51_DK

Toolchain:
GCC_ARM|ARM|IAR

mbed-os sha:
c6f655c02

Expected behavior
core_util_are_interrupts_enabled return false when interupt disabled

Actual behavior
core_util_are_interrupts_enabled return true when interupt disabled

Steps to reproduce

...
printf("interrupts_enabled (1): %s\r\n", core_util_are_interrupts_enabled() ? "yes" : "no");
core_util_critical_section_enter();
printf("interrupts_enabled (2): %s\r\n", core_util_are_interrupts_enabled() ? "yes" : "no");
core_util_critical_section_exit();
printf("interrupts_enabled (3): %s\r\n", core_util_are_interrupts_enabled() ? "yes" : "no");
printf("\r\n");
...

Result

interrupts_enabled (1): yes
interrupts_enabled (2): yes
interrupts_enabled (3): yes

Expected result

interrupts_enabled (1): yes
interrupts_enabled (2): no
interrupts_enabled (3): yes

[maciejbocianski]

@bulislaw @0xc0170

[0xc0170]

cc @nvlsianpu @anangl

[pan-]

On the NRF51, core_util_critical_section_enter doesn't disable interrupts instead it mask all interrupts which are not reserved for BLE processing. core_util_are_interrupts_enabled look after PRIMASK to assess if the interrupts are enabled or not. In that case it will always return true.

[bulislaw]

@maciejbocianski ping

[maciejbocianski]

So assuming that BLE interrupts are out of scope of our interest maybe we should also modify a behaviour of core_util_are_interrupts_enabled to align it to core_util_critical_section_enter and core_util_critical_section_exit for this platform?
I think that some expert on NRF51(BLE) should look on it

Other solution could be introduction of another function core_util_in_critical_section which will be complementation of core_util_critical_section_enter and core_util_critical_section_exit and to check if we are in critical secition we will use core_util_in_critical_section not core_util_are_interrupts_enabled.

[pan-]

So assuming that BLE interrupts are out of scope of our interest maybe we should also modify a behaviour of core_util_are_interrupts_enabled to align it to core_util_critical_section_enter and core_util_critical_section_exit for this platform?

If on this platform core_util_critical_section_enter and core_util_critical_section_exit disable interrupts then BLE connection may drop unexpectedly and won't be reliable anymore. The main reason for this chip to exist being to execute BLE applications, it is critical that the OS doesn't interfere in BLE handling. Otherwise there is little point of running mbed OS on this chip and even less for Nordic to partner with us.

Other solution could be introduction of another function core_util_in_critical_section which will be complementation of core_util_critical_section_enter and core_util_critical_section_exit and to check if we are in critical secition we will use core_util_in_critical_section not core_util_are_interrupts_enabled.

@bulislaw What do you think of this proposition ?

[bulislaw]

I think it makes sense, we can't remove core_util_are_interrupts_enabled, but we can add extra function. Bu i don't like idea of quick fixing it and relying on platform code to set some flags.
Maybe we could make it a non-mandatory HAL API? Something like:

/* in hal_critical_section.c */
MBED_WEAK hal_critical_section_enter(void)
{
    __disable_irq();
}

/* in mbed_critical.c */
void core_util_critical_section_enter(void)
{
    bool interrupts_disabled = !core_util_in_critical_section();
    hal_critical_section_enter();

    /* Save the interrupt disabled state as it was prior to any nested critical section lock use */
    if (!interrupt_enable_counter) {
        critical_interrupts_disabled = interrupts_disabled;
    }

    /* If the interrupt_enable_counter overflows or we are in a nested critical section and interrupts
       are enabled, then something has gone badly wrong thus assert an error.
    */
    MBED_ASSERT(interrupt_enable_counter < UINT32_MAX); 
// FIXME
#ifndef   FEATURE_UVISOR
    if (interrupt_enable_counter > 0) {
        MBED_ASSERT(interrupts_disabled);
    }
#else
#warning "core_util_critical_section_enter needs fixing to work from unprivileged code"
#endif /* FEATURE_UVISOR */
    interrupt_enable_counter++;
}

@0xc0170 @pan- @c1728p9 what do you think?

[0xc0170]

Other solution could be introduction of another function core_util_in_critical_section which will be complementation of core_util_critical_section_enter and core_util_critical_section_exit and to check if we are in critical secition we will use core_util_in_critical_section not core_util_are_interrupts_enabled.
@bulislaw What do you think of this proposition ?

wouldn't be better to have target specific implementation ? As I understand if softdevice is active, its own interrupt are enabled and do not interfere with an application, thus if an app asks if interrupts are disabled for this platform, what should be returned? app interrupts state? or include also softdevice interrupts state? I would say just app interrupts. Is something like softdevice common (having app interrupts + specific chip interrupts that are separated from app space thus no conflicts there) ?
I believe this is what was proposed above (https://github.com/ARMmbed/mbed-os/issues/5198#issuecomment-332529282). Or might be better just leave it as it , interrupt status (all of them), and having a new api. Anyway, this nordic specific behaviour should be documented in critical section (generic description that in some cases critical section might just disable only app interrupts, having this new indirection via hal, it would be obvious that there are special cases).

MBED_WEAK hal_critical_section_enter(void) - I do not follow how this solves the issue we are having? That nordic would provide own implementation as it does now, but in hal for enter/exit, that would be good.

[pan-]

I like the idea of hal_critical_section_enter and hal_critical_section_exit overridden if required by the platform and called indirectly by core_util_in_critical_section_enter and core_util_in_critical_section_exit.

Thanks to the introduction of that level of indirection core_util_in_critical_section might be defined outside platform code and wouldn't need any action from partner.

core_util_are_interrupts_enabled and core_util_in_critical_section can coexist together.

[bulislaw]

@scartmell-arm will propose solution, based on comments here.

[maciejbocianski]

fixed by adding new API
HAL: hal_in_critical_section()
platform: core_util_in_critical_section()