Implement interrupt enable/disable as a counting semaphore

[bremoran]

Everywhere that we use __disable_interrupts and __enable_interrupts, we need to store the priority mask and check it later. It might be better to provide a standard API for this kind of task.

A nice abstraction to use in this sort of situation is a counting enable/disable. Each time disable is called, it increments a global counter. Each time enable is called, it decrements the counter, only enabling interrupts when the counter reaches 0.

These operations need not be atomic, since they disable interrupts.

Here is a sample implementation:

static volatile uint32_t interruptEnableCounter = 0;

void critical_section_enter() {
    __disable_interrupts();
    interruptEnableCounter++;
}

void critical_section_exit() {
    if (interruptEnableCounter) interruptEnableCounter--;
    if (!interruptEnableCounter) __enable_interrupts();
}

[bremoran]

@rgrover @bogdanm @hugovincent

[rgrover]

yes, this is useful.

shouldn't you be using ei_counting() for enable_interrupts() (as in the following)?

void enable_interrupts() {
    ei_counting();
}

[bremoran]

@rgrover enable_interrupts is a forcing instruction. Even if your have an existing disable interrupt count, enable interrupts is supposed to override everything. The point of disabling interrupts first is that an interrupt might occur while we're setting the count to 0.

[hugovincent]

If enable_interrupts doesn't always necessarily enable interrupts, I'd suggest a different name, such as critical_section_enter/critical_section_exit. [update] Erm, I mean ei_counting. Reading fail.

[bremoran]

Noted/updated

[rgrover]

how about renaming enable_interrupts as critical_section_abort()? and dropping disable_interrupts entirely?

[hugovincent]

+1

[bremoran]

Maybe critical_section_reset(). The problem is that this masks the behaviour and purpose of the function. enable_interrupts is a proxy for __enable_interrupts(), but it correctly updates the state of the interruptEnableCounter. This could be used, for example, in initialisation code or in exception handlers. In these situations, the existence or non-existence of a critical section doesn't bear on the operation that the code is trying to perform, so I'm not sure that calling it critical_section_abort is the right choice.

That being said, I prefer having consistent naming across the API. Maybe the right choice is:

void critical_section_abort() {
    __disable_interrupts();
    interruptEnableCounter = 0;
    __enable_interrupts();
}
void enable_interrupts() {
    critical_section_abort();
}

[hugovincent]

I'm confused what cases you would use this in I guess. Of the two mentioned, only early boot code makes sense to me (but that can call __enable_interrupts directly and the docs for criticalsection* can specify that these APIs are only for use after early boot has completed).

You're right that abort isn't the right name... it implies some kind of transactional semantics.

What use-cases are there to abort/reset/get-out-of a critical section, that aren't fatal bugs? If code thinks it needs to do something atomically, and somewhere (nested inside) it can have interrupts re-enabled without knowing, you have a bug.

[bremoran]

I think that it only matters in fault handlers and init code, both of which can use __enable_interrupts directly.

If any fault handlers require interrupts to be turned on and then use any library functions which, in turn, call critical_section_enter or critical_section_exit, then it's imperative that the counter be reinitialised.

I can imagine this sort of feature being needed in order to dump diagnostics over a network, etc.

[hugovincent]

Normally diagnostics would be dumped to the network after a reboot, and normally after a fatal error, any library functions are no longer safe to use (any global state, including the allocator, can't be trusted after a fatal error).

[bremoran]

I know that is the normal state; what I didn't know is whether we would try to do something more creative or not--a sort of partial reset, if you will. Barring that use case, I don't think there's a call for critical_section_abort()