[En-Nut-Discussion] NutEnterCriticalLevel

[Dušan]

Uwe,

thanks for the discussion.
I found probably a reason, why we needed to preserve a priority level - 
see below the second section.
Third section is about how we tried to implement the nesting mechanism.
Maybe I am wrong, so please share your opinion.

-------------------------
_ARM implementation_
First I checked the SVN #1897 for the include\arch\arm\atom.h.
The previous version of this file was only manipulating status word (SW) 
via r0.
The #1897 changed r0 to using a temp_ variable, also current SW was 
stored onto !sp, preserving it until NutExitCritical.
#2040 added "cc" clobbers for proper compiler optimization.

The #2150 finally left this critical state entry  implementation to a 
stack version - NUT_CRITICAL_NESTING_STACK,
added critical_nesting_level for faster switching, if critical state was 
already entered.
I found e.g. code in NutThreadCreate, where NutThreadAddPriQueue is 
called after NutEnterCritical invocation.

Then it was realized, there is no need to preserve SW.

-------------------------
A note below, from the Coldfire MCF52259 CPU manual, says, it can happen 
a spurious interrupt can occur.
To avoid it, there is a need to increase priority level to block such a 
potential interrupt, when you manipulate the peripheral module IMR.
To accomplish this, we need to run critical sections in some drivers at 
lower int priorities and increase this priority to highest (7) while 
manipulating with the module's IMR
You can check code at the beginning for PREVENT_SPURIOUS_INTERRUPT 
macros, which enclose some code between NutEnterCritical and 
NutExitCritical.
Mechanism is exception -> enter a certain int priority level, want to 
manipulate uimr -> increase to highest priority.

Idea: I can think about it - maybe, we can use a special variant of 
NutEnterCritical with nesting only for a driver implementation.
Then the rest of OS can stay with NutEnterCritical only setting / 
clearing the SW bits in a similar way, you do it in the ARM platform.
What do you think?

> /*
>  * MCF52259RM.pdf - 16.3.2 Interrupt Mask Registers (IMRHn, IMRLn)
>  *
>  * NOTE
>  *
>  * A spurious interrupt may occur if an interrupt source is being 
> masked in the
>  * interrupt controller mask register (IMR) or a module’s interrupt mask
>  * register while the interrupt mask in the status register (SR[I]) is 
> set to a value
>  * lower than the interrupt’s level. This is because by the time the 
> status
>  * register acknowledges this interrupt, the interrupt has been masked. A
>  * spurious interrupt is generated because the CPU cannot determine the
>  * interrupt source.
>  * To avoid this situation for interrupts sources with levels 1–6, 
> first write a
>  * higher level interrupt mask to the status register, before setting 
> the mask in
>  * the IMR or the module’s interrupt mask register. After the mask is 
> set, return
>  * the interrupt mask in the status register to its previous value. 
> Because level
>  * 7 interrupts cannot be disabled in the status register prior to 
> masking, use of
>  * the IMR or module interrupt mask registers to disable level 7 
> interrupts is
>  * not recommended.
>  */


-------------------------
Our idea was not to store SW on stack, but use a register for preserving it.
In this case, the block entry parenthesis becomes a problem.
We tried first to use it with only 1 parenthesis and use a local variable:
> #define NutEnterCritical() \
> { \
>     uint16_t volatile _savpri; \
>     asm volatile ( \
>         "move.w    %%sr, %%d0\n" \
>         "move.w    %%d0,%[savpri]\n" \
>         "ori.l #0x700, %%d0\n" \
>         "move.w %%d0, %%sr\n" :: [savpri] "m" (_savpri) : "d0");
>
>
> #define NutExitCritical() \
>  \
>     asm volatile ( \
>         "move.w %[savpri], %%d0\n" \
>         "move.w %%d0,%%sr\n" :: [savpri] "m" (_savpri) : "d0"); \
> }

Then we tried following:
> #define NutEnterCritical() \
> { \
>     uint16_t register _savpri; \
>     asm volatile ( \
>         "move.w    %%sr, %%d0\n" \
>         "move.w    %%d0,%[savpri]\n" \
>         "ori.l #0x700, %%d0\n" \
>         "move.w %%d0, %%sr\n" : [savpri] "=d" (_savpri) :: "d0");
>
> #define NutJumpOutCritical() \
>  \
>     asm volatile ( \
>         "move.w %[savpri], %%sr\n" :: [savpri] "d" (_savpri)); \
>
> #define NutExitCritical() NutJumpOutCritical() }

The NutUseCritical was a final solution.

*Dušan*

On 16.1.2018 13:41, Uwe Bonnes wrote:
> Dear Dušan,
>>>>>> "Dušan" == Dušan  <dferbas at etech.cz> writes:
>      Dušan> Uwe, I feel we've got on a wrong path :-).
>
>      Dušan> Why you concentrate on the NutEnterCriticalLevel ? Let's forget
>      Dušan> about it for a while.  I suggest to start with NutEnterCritical
>      Dušan> for m68k platform. This is the initially issue, we started to
>      Dušan> talk about.  I attached 2 emails below.
>
>      Dušan> Let's look at the include\arch\arm\ atom.h.  Here ccodes are also
>      Dušan> stored into a temp variable.
>
> No, this temp variable is only used to not destroy R0. Look at the log entry
> for Rev 1897:
> "Bug #1757410 fixed. NutEnter/ExitCritical destroyed ARM register R0."
>
> temp is also only inside a stack frame, so it is lost when NutEnter|Exit
> Critical() when the stack frame and that function is exited.
>
>      Dušan> That's exactly what we are doing on the m68k platform.
>
>      Dušan> ---------- But with m68k gcc, we have a problem, when there is a
>      Dušan> nested NutEnterCritical.  The temp variable is compiled as 2
>      Dušan> different registers, 1st level goes e.g. into d1, 2nd level into
>      Dušan> d2. Then leaving the 2nd level restores status word from d1,
>      Dušan> which is a problem This is why we started to use NutUseCritical
>      Dušan> macro.
>
> I don't see any nested NutEnterCritical in the common code. Can you point
> me at such a use?
>
> If not, I think that some form of nested critical section is a case for a
> new function, but not for NutEnterCritical().
> You can implement that new function for your arch as you like. Name it like
> NutEnterCriticalLevel or NutEnterNestedCritical, etc.
>
> Bye
>