portEXIT_CRITICAL() enables interrupts in ISR

johandc wrote on Thursday, February 04, 2010:

HI,

After 6 hours of debugging i finally found out why my ARM7 was crashing: (Short version below)

Scope: Inside USART ISR:
1) Call to portENTER_CRITICAL() - Increments ulCriticalNesting to 1
2) Magic happens, data is processed
3) Call to portEXIT_CRITCIAL() - decrements ulCriticalNesting to 0
4) PortEXIT_CRITICAL()  sees that nesting is 0 and calls portENABLE_INTERRUPT()    <-- This is the error!
5) Interrupts are now enabled, before exiting the USART ISR: Result :
6) Nested interrupt occurs, call to PortSAVE_CONTEXT() etc….

… At this point, the system has entered an invalid state since a nested interrupt occured, and invalid information was stored on the stack… The system will never return to running state, and in fact sooner or later (a couple of milliseconds later) a PREFETCH_ABORT interrupt is thrown by the processor.

This problem also exists in several other ports, for example the AVR32.

I think it is a general problem that portEXIT_CRITICAL() want’s to enable interrupts, disregarding the calling context. If FreeRTOS does not support nested interrupts, it should never enable interrupts unless the cpu is in system mode. The result is that no calls which are protected by critical sections can be called from within an iSR, which is a bit of a pain.

Please help me, is this a bug in AVR32 + ARM7 FreeRTOS?

Temporary FIx
I solved the problem by checking the status register for the running mode of the processor. If it runs in ISR mode, it won’t enable interrupts, and now it works… However, is this a good solution?

johandc wrote on Thursday, February 04, 2010:

This topic points out the exact same problem:
https://sourceforge.net/projects/freertos/forums/forum/382005/topic/1453677

However they don’t reach a conclusion on the problem, which i now think is a major bug in FreeRTOS.

jorick23 wrote on Thursday, February 04, 2010:

I would think that when portENTER_CRITICAL bumps ulCriticalNesting from 0 to 1, it should also save the current interrupt state (enabled or disabled).  Then when portEXIT_CRITICAL bumps ulCriticalNesting from 1 to 0, it should restore that saved state.

I fixed a problem similar to this in my project a while back by doing the same thing.

johandc wrote on Thursday, February 04, 2010:

It doesn’t seem to be that easy on all ports. The ARM7 and AVR32 does not store the interrupt state, but uses a nesting counter instead.

I personally think that the solution of adding “context-awareness”, by checking the CPU mode-register, is better than having all these “FromISR” functions. It could be a simple function like:
if (cpu_get_mode() != CPU_ISR) portENTER_CRITICAL();

But i suppose it takes too much overhead??

destremps wrote on Thursday, February 04, 2010:

I’m going to jump in without studying what you guys are talking about too hard and suggest what I did in the AVR32 environment to cover this. You must ensure that all interrupts (which make free_rtos calls) are AT THE SAME PRIORITY LEVEL. Inlcuding the rtos tick int. I believe this way they won’t nest (at least in AVR32). And your problems go away.

  

destremps wrote on Thursday, February 04, 2010:

Sorry I should’ve included this.

Here’s my thread from before

https://sourceforge.net/projects/freertos/forums/forum/382005/topic/3315259

johandc wrote on Thursday, February 04, 2010:

@destremps: Aha! That might explain why i’m seeing this issue only on the ARM7 and not the AVR32 even though i run the same code on both platforms.

I don’t know if this is an option for the ARM7 processor. Either way, i have decided to do it properly and never call portENTER/EXIT_CRITICAL() from an ISR. It takes a lot of work to wrap all my Malloc and Free calls in a vpPortMallocFromISR(), and even worse, i have functions like conn_close(), that gets called both by tasks and from ISR, that in turn calls vPortFree… So i now have to make versions like “conn_close_from_isr()” too… :frowning:

As a result i might even change to a pre-allocated buffer handling system. But i don’t like over-complicating my code when there are no hard deadlines, and malloc works fine in ISR.

rtel wrote on Thursday, February 04, 2010:

**Never, never, never **call portENTER_CRITICAL() or portEXIT_CRITICAL() from within an ARM7 interrupt unless you have changed the code yourself so that it would work.  It is pointless in any case as ARM7 interrupts do not nest and leave interrupts disabled for their entirety - so why try to disable them again?.  Nesting interrupts on an ARM7 is quite complex so by default it is not done - better to instead keep the interrupt really short and defer processing to a handler task.

Only API functions that end in FromISR or FROM_ISR can be called from an ISR

The critical nesting count is part of the task context and has no meaning in an interrupt.  For ports that support interrupt nesting there are the macros portSET_INTERRUPT_MASK_FROM_ISR() and portCLEAR_INTERRUPT_MASK_FROM_ISR() that save the current interrupt state then restore the original interrupt state respectively - leaving the critical nesting count untouched.  Take a look at ports such as the Cortex M3 and Coldfire V2 to see examples.

Regards.

rtel wrote on Thursday, February 04, 2010:

and malloc works fine in ISR

Only just read that line - calling malloc from inside an ISR is a really bad idea, its not deterministic, can take a long time to execute, is probably not reentrant, etc.

Regards.

johandc wrote on Friday, February 05, 2010:

@richardbarry. Thank you for the reminder. I simply got too comfortable using the FreeRTOS API that i started using it within ISR’s too. When i’m at it, perhaps i can write a buffer-handler to avoid calling pvPortMalloc from ISR, however i would also like to have ISR/Task safe versions of xTaskGetTickCount and vTaskList.

I’m using these functions as a part of a small “service-set” in a network stack, and i’m trying to avoid to ask all nodes on the network to include a “ServiceTask” to handle these simple commands (ping, process-list and memfree).

Regarding calling Malloc from within an ISR, if only all Task context calls vpPortMalloc() which disables interrupts, i think it should be safe. I know that it’s not deterministic and may take a “long time” - however everything that is critical runs with hardware DMA, so i have plenty of time to execute these ISR’s. - but for the main part i will change to a pre-allocated buffer handler in order to make all calls to my network stack both Task and interrupt safe.

What do you think about the idea that API calls would check which context they are called from and behave differently? I.e. QueueSend() would call some macro, say CPU_GET_MODE(), that returns CPU_ISR or CPU_TASK depending on the current execution context. Then the function QueueSend would behave as either QueueSend or QueueSendFromISR depending on this.??

This would simplify the API a lot, and make FreeRTOS more robust to lazy programmers that forget about their context, or have code which is called from multiple contexts (like a network stack)…

johandc wrote on Friday, February 05, 2010:

By the way, if i wanted to spare my subsystems from the pain of having another task using up stack-space. I could create a new task from the ISR and pass a pointer to the buffer to the task, which would then be schedules, run in task context and delete it self after finishing. In this way i defer the processing to a task, but it does not use up memory when not used…

The catch is: It’s impossible to create a task from within an ISR… Since there are no “unprotected” version, and since it calls pvPortMalloc in order to allocate stack space…

The problem is, i don’t have sufficient memory to have another task idling in the system just to answer ping-packets from the network stack. (and process-list and memfree calls too)

aturowski wrote on Friday, February 05, 2010:

What do you think about the idea that API calls would check which context they are called from and behave differently? I.e. QueueSend() would call some macro, say CPU_GET_MODE(), that returns CPU_ISR or CPU_TASK depending on the current execution context. Then the function QueueSend would behave as either QueueSend or QueueSendFromISR depending on this.??

Most small sized CPU are not able to find out if the code they are currently executing is  ISR or “task”. I mean there is no facilities in CPU itself to distringuish between these modes in run time.

This would simplify the API a lot, and make FreeRTOS more robust to lazy programmers that forget about their context, or have code which is called from multiple contexts (like a network stack)…

You can call the same code from multiple contexts provided that the calling routine provide correct context/protection.

And one more thing…. FreeRTOS is designed to run VERY close to the hardware with VERY limited resources available. So simply there is no resources available to protect against lazy programmers.

davedoors wrote on Friday, February 05, 2010:

What do you think about the idea that API calls would check which context they are called from and behave differently? I.e. QueueSend() would call some macro, say CPU_GET_MODE(), that returns CPU_ISR or CPU_TASK depending on the current execution context. Then the function QueueSend would behave as either QueueSend or QueueSendFromISR depending on this.??

People would very quickly complain about the non portability of the solution and the extra processing time taken.

By the way, if i wanted to spare my subsystems from the pain of having another task using up stack-space. I could create a new task from the ISR and pass a pointer to the buffer to the task, which would then be schedules, run in task context and delete it self after finishing. In this way i defer the processing to a task, but it does not use up memory when not used…

The catch is: It’s impossible to create a task from within an ISR… Since there are no “unprotected” version, and since it calls pvPortMalloc in order to allocate stack space…

There are lots of catches to doing that, the main one being that your system would crash immediately. Even if it didn’t crash it would be incredibly slow. Create a persistent handler task that uses a semaphore to be told when there is processing that needs to be done. How is creating and deleting it all the time going to make it use less memory?

johandc wrote on Friday, February 05, 2010:

The problem is, i want the network-stack API to be inherently thread and interrupt safe but without having to add _from_isr() to all my functions. Currently i have implemented a static memory allocation with a small pool handler which is faster and can be called from all contexts. Now i’m down to the QueueSend call’s which sometimes require from_isr (for example when a ping-reply is sent directly from the ISR of the network adapter)…

I’m still struggling to figure out how i can pull this off…

johandc wrote on Friday, February 05, 2010:

@davedoors

The problem is, i want to avoid having a persistent service task running, just in order to answer ping-packets. Especially since we have very low memory and adding another task would require at least 400b stack on AVR32. By creating and deleting the task, the 400 bytes stack used by the task could be used for 2 packet buffers for the router instead.

johandc wrote on Friday, February 05, 2010:

Okay, i solved the problem “by documentation”. It is now required that the user calls the serviceHandler from within one of his existing server tasks. This way the services do not use more stack than is used already, and now all the services execute in a task context, so i don’t have multiple context calls any longer.

(Okay, there are still some when routing with the loopback device, but it removed the biggest part of them)

Thank you for your help and support :slight_smile: - It’s the first time i have looked into this forum and it is a very good resource! I will recommend all our customers to use it as well.

jorick23 wrote on Friday, February 05, 2010:

What about something like this?

First, a little ARM assembler (can’t be in user mode to execute):

IsInIrq:
        mrs     r0, cpsr                // Get the program status register
        and     r0, r0, #0x1F           // Isolate the mode bits
        cmp     r0, #MODE_IRQ           // Check if in IRQ mode
        movne   r0, #0                  // If no, return a boolean false
        moveq   r0, #1                  // If yes, return a boolean true
        bx      lr                      // Return to the caller

And then some C code:

   if (IsInIsr ( )) {
      xQueueSendFromISR (…);
   } else {
      xQueueSend (…);
   }

Of course the assembler portion depends on your processor.

johandc wrote on Friday, February 05, 2010:

Yes, this was exactly what i meant!

Very nice ASM implementation of the “macro”, i had something like this:

int x;
asm volatile ("mrs %0,cpsr":"=r"(x));
if ((x & 0x1f) == 0x12) {
xQueueSendFromISR(...);
} else {
xQueueSend(...);
}

But your suggestion is a lot nicer…

Both for ARM7 and AVR32 it is possible to get the mode-information.
For AVR8, it doesn’t have different modes really, but the global interrupt flag is always disabled in ISR context so this flag might be used instead. As an alternative, each call to portSAVE_CONTEXT() could increment a variable that tells that the system is running in ISR mode.

richard_damon wrote on Saturday, February 06, 2010:

There are very good reason for having distinct routines for fromISR and fromTask contexts:
1) It is designed that ISRs normally want to execute as quick as possible and are normally very short, not using a lot of other code, so the code knows it is in an ISR when it makes the calls.

2) Because of the environment they execute in, they are optimized differently. Task level routines need to have the areas that need to be protected by critical sections as small as possible to minimize interrupt latency, maybe even at the cost of being a little slower, the ISR  variants know they are not going to be interrupted, so don’t need to be concerned about that, but want to be as fast as possible

3) The two sets of routines mayu do slightly different operations, the task level routines may need to suspend the current task if something isn’t ready for them, the ISR level don’t need to do this, but do need to defer task switching if needed and return a flag to indicate that a task switch is requested.

As you have seen, with the current set of routines, it is possible to add a layer on top that can handle making the decision for the programs that want that ability, If the base library always did this, then those applications that were sensitive to the extra overhead of that system would be stuck.

One advantage of an open source package like this is you are free to modify it, as long as you follow the rules on sharing the mods. One idea that would allow it to be done on any processor is to define a macro to be used at the start/end of every interrupt that wants to use this ability that sets/clears a flag, (and clears the task switch request variable). Since it would be defined in the port files, on those ports that have an easy test to see if you are in an interrupt, could omit the flag (this would say that the dual mode functions would call a port macro to test the flag, either checking the variable, or the hardware if available).