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

[Dušan]

Hi Philipp,

when I wrote about "driver" code,
I meant everything, which works with a low level, peripheral part of 
your application.

C compiler mostly uses r0 to pass return value to a calling code.
On avr it is r0, r1 for int as int is 16 bit there. On 32 bit cpus, I 
think it is the r0.
You can check assembler listings, how it is compiled.
E.g. in Linux kernel or U-Boot, you can insert in a makefile cflags-y += 
-Wa,-ahlms=$(@:.o=.lst)
to generate listings, to see how compiler compiles to instructions.

So r0 was probably intended to define something like int 
NutEnterCritical(void),
i.e. a function, which returns a value of interrupt priority mask,
which is modified with cpsid, cpsie instructions.
As per 
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dai0321a/BIHBFEIB.html,
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0552a/CHDBIBGJ.html
this should return 1 for disabled ints and 0 for enabled ints.

---
I suggest to redefine NutEnterCritical to become an inline function.
This allows to stay with its definition in a header.
It will be nice to allow compiler to check arguments type for case, when 
we will add an argument.

I think we can stay with current cm3 code at first with primask,
later we can remove the (I think) unneeded mrs r0,PRIMASK.
For levels, we need to operate with basemask.
https://github.com/jameswalmsley/FreeRTOS/blob/a7152a969b2b49fce50d759b3972f17bf3b18ed7/FreeRTOS/Source/portable/GCC/ARM_CM4F/portmacro.h

http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0552a/CHDBIBGJ.html, 
link to PRIMASK register explanation
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dai0321a/BIHBFEIB.html

Something like below.
I do not know if we need to know a prio mask level when we return from 
protected part.
In this case, we can redefine NutExitProtectedLevel not to read basepri 
and return a value.

I only wrote the code, I did not compile it,
so I apologize if there is a mistake.

Usage
     NutUseProtected();

NutEnterProtected(PRIO_MASK_LEVEL_UART);
...
NutExitProtected();

Definition

static inline void NutEnterCritical(void) {
     asm volatile (
         "@ NutEnterCritical"    "\n\t" \
         "mrs     r0, PRIMASK"   "\n\t" \
         "cpsid   i"             "\n\t" \
         :::"r0" \
     );
}

static inline NutExitCritical(void) {
     asm volatile (
         "@ NutExitCritical"     "\n\t" \
         "mrs     r0, PRIMASK"   "\n\t" \
         "cpsie   i"             "\n\t" \
         "isb"                   "\n\t" \
         "dsb"                   "\n\t" \
         :::"r0" \
     );
}

static inline uint32_t NutEnterProtectedLevel(uint32_t 
requested_prio_mask) {
     uint32_t prev_prio_mask, new_prio_mask;
     asm volatile (
         "@ NutEnterCritical"    "\n\t" \
         "mrs     %0, basepri"   "\n\t" \
         "mov     %1, %2"        "\n\t" \
         "msr     basepri,%1"    "\n\t" \
         "isb"                   "\n\t" \
         "dsb"                   "\n\t" \
         :"=r" (prev_prio_mask), "=r" (new_prio_mask): "rm" 
(requested_prio_mask) \
     );
     return prev_prio_mask;
}

#define NutExitProtectedLevel(mask) (void)NutEnterProtectedLevel(mask)

#define NutUseProtected() \
     uint32_t register _saved_prio_mask;

#define NutEnterProtected(mask) \
     _saved_prio_mask = NutEnterProtectedLevel(mask)

#define NutExitProtected() \
     NutExitProtectedLevel(_saved_prio_mask)


*Dušan*

On 5.12.2019 19:54, Philipp Burch wrote:
> 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
> _______________________________________________
> http://lists.egnite.de/mailman/listinfo/en-nut-discussion