[En-Nut-Discussion] NutExitCritical() behaviour on ARM Cortex-M

[Philipp Burch]

Hi Uwe, hi Dušan!

On 05.12.19 10:46, bon at elektron.ikp.physik.tu-darmstadt.de wrote:
> Philipp Burch writes:
>> Hi everyone,
> ... 
>> Am I just missing something in the current implementation?
>>
> Hi Philipp,
> 
> probably you do not miss something. There has been some discussion
> about that subject from time to time. There was no conclusive
> solution.

It is mostly that placing of PRIMASK into r0 from *both* macros that I
don't understand. What is the point of placing some read data in a
register that is not used afterwards?

> 
> How do other OSes (RTOS, mbed, etc) handle that situation?
> 
> What is the use case.

>From my understanding, the idea of a critical section is that it can be
nested safely, especially when used inside generic code (be it for
disabling a single interrupt or all interrupts). This is also what the
comment in sys/atom.h says:

 * NutExitCritical()
 * Finalize a critical section. Must be used only at the end of a
critical section
 * to restore the global interrupt flag to the state saved by
NutEnterCritical()

My use case leading me to digging into this was that I have some code
that accesses hardware resources which should be usable both from
interrupt context as well as from thread context. The current
implementation should not cause a problem in my case, but I got confused
by the code and the comment.

> 
> Some discussion also revolved around stacking critical section. I was
> not convinced that such stacking is really needed.

I agree that nesting critical sections (or calling functions from a
critical section in general) may not be a very good idea in the first
place. But sometimes it may lead to somewhat cleaner code (when building
an API to access shared resources while offering different levels of
abstraction for example). Combined with the comment that actually says
something else than what the code does, this could indeed lead to subtle
and hard-to-debug bugs in the code.

I also agree that changing the behaviour of such a low-level function
may cause collateral damage. So it is certainly sensible to add a new
API instead of updating the present one; but the comment should be fixed
to avoid being misleading.

A new interface could be named something like
NutEnterCriticalSectionNestable(). In my opinion, it should return an
unsigned int containing the previous state of the interrupt enable. This
could then passed to NutExitCriticalSectionNestable() to restore the
state. A typical use case would then look like this:

unsigned int iestate = NutEnterCriticalSectionNestable();
/* Perform shared resource access. */
NutExitCriticalSectionNestable(iestate);

DSB() and ISB() could also be integrated for ARM code. What do you think?

The FreeRTOS way with the global nesting counter would be another option
without the need to supply a local variable. But personally, I'd prefer
the explicit version over integrated global state...

> I think the first instruction only reads current priority mask to the function return code register.
> Then all ints are globally disabled.

Correct me if I'm wrong, but with the current implementation, isn't the
value stored in r0 lost anyway? r0 may be the function return value
register, but the code is implemented as a macro, so the using C code
can't really access the stored value. Or can it? I'm not too familiar
with inline assembly...

Best regards,
Philipp