Fix race condition when tracing is enabled (#95)
* Update port.c
I discovered a very snicky and tricky race condition scenario when integrating tracealyzer code into our project.
A little background on CortexR5 : When the IRQ line (comming from the interrupt controller, to which every peripheral IRQ lines connect) of the processor rises and the IRQ Enable bit in the status register of the CPU permits it, the CPU traps into interrupt mode. Several things happen. First, the CPU finishes the instruction it was performing. Second, it places the content of the CPSR register into the SPSR_irq register. And third, the mode of the CPU is changed to IRQ_Mode and /!\ THE IRQ ENABLE BIT IN CPSR_irq IS AUTOMATICALLY CLEARED /!\. The reason is to ensure that upon landing into IRQ code, we find ourselves automatically in a critical section because we cannot be interrupted again because the bit is cleared. The programmer can, if he wants, re-enable IRQs inside IRQ code itself to allow interrupt nesting. But it has to be wanted and meditated.
Now, inside portASM.S, at the end of 'FreeRTOS_IRQ_Handler' assembly function, a call to 'vTaskSwitchContextConst' is made if the variable 'ulPortYieldRequired' was set by someone while executing the interrupt. Before branching to that function, a 'CPSID i' instruction was placed to ensure that interrupts are disabled in case someone re-enabled it. Inside 'vTaskSwitchContext', there is the macro 'traceTASK_SWITCHED_OUT' that gets populated when tracing is enabled.
The bug is right there.. If the macro is populated and inside that macro there is a matching call to 'ulPortSetInterruptMask' and 'vPortClearInterruptMask', a race condition can occure is there is a OS Tick timer interrupt waiting at the interrupt controller's door. Upon calling 'vTaskSwitchContext', the interrupts are not masked in the interrupt controller, the only barrier against the CPU servicing that tick interrupt while already performing the function is that the IRQ Enable bit cleared. 'ulPortSetInterruptMask'
does what's its supposed to do, but doesn't take into account the IRQ Enable bit in CPSR. Wheter or not the bit was cleared, the function sets it at the end. When calling the matching 'vPortClearInterruptMask', the function clears the interrupt mask in the interrupt controller. Because the IRQ Enable bit (that was cleared) has been set no matter what in 'ulPortSetInterruptMask', the CPU services the OS Tick Interrupt right away.
The bug is there : instead of completing the 'vTaskSwitchContext' function, the CPU re-enters the switch context path right after 'traceTASK_SWITCHED_OUT' thus corrupting the CPU state and eventually triggering either an undefined instruction, data or instruction abort.
* Update port.c
Error on my part, this is the right inline asm code to retreive CPSR register
* Update port.c
Forgot an * while writing comment..
diff --git a/portable/GCC/ARM_CR5/port.c b/portable/GCC/ARM_CR5/port.c
index 418020f..0658218 100644
--- a/portable/GCC/ARM_CR5/port.c
+++ b/portable/GCC/ARM_CR5/port.c
@@ -147,6 +147,19 @@
#define portTASK_RETURN_ADDRESS prvTaskExitError
#endif
+
+/* Adding the necessary stuff in order to be able to determine from C code wheter or not the IRQs are enabled at the processor level (not interrupt controller level) */
+#define GET_CPSR() ({u32 rval = 0U; \
+ __asm__ __volatile__(\
+ "mrs %0, cpsr\n"\
+ : "=r" (rval)\
+ );\
+ rval;\
+ })
+
+#define CPSR_IRQ_ENABLE_MASK 0x80U
+
+#define IS_IRQ_DISABLED() ({unsigned int val = 0; val = (GET_CPSR() & CPSR_IRQ_ENABLE_MASK) ? 1 : 0; val;})
/*-----------------------------------------------------------*/
/*
@@ -468,7 +481,13 @@
uint32_t ulPortSetInterruptMask( void )
{
uint32_t ulReturn;
-
+ uint32_t wasIRQDisabled;
+
+ /* We keep track of if the IRQ are enabled in the CPU (as opposed to interrupts masked in the interrupt controller, like the intend of this function).
+ * This is very important because when the CPU is interrupted, among other things, the hardware clears the IRQ Enable bit in the CPSR of the IRQ CPU Mode in which
+ * we enter. */
+ wasIRQDisabled = IS_IRQ_DISABLED();
+
/* Interrupt in the CPU must be turned off while the ICCPMR is being
* updated. */
portCPU_IRQ_DISABLE();
@@ -486,7 +505,18 @@
"isb \n"::: "memory" );
}
- portCPU_IRQ_ENABLE();
+ /* Just like this function returns a value of wether or not the interrupts where masked in the interrupt controller in order to avoid race condition when
+ * calling its matching vPortClearInterruptMask function, we needed a 'wasIRQDisabled' variable holding the state of the IRQ Enable bit in the CPSR in order
+ * to leave that bit in it's original state. Like mentioned above, hardware automatically clear the IRQEnable bit upon trapping into IRQ Mode, so the programmer
+ * cannot make assumption about it's state. Very rare, but very important race condition is avoided with this when this function is called in an ISR. The race
+ * condition in question was discovered when integrating tracealyzer code. Inside the function 'void vTaskSwitchContext( void )' in tasks.c, there is a macro 'traceTASK_SWITCHED_IN();'
+ * which gets replaced by something when using the tracing capabilities. That macro protects some critical section with matching calls to 'ulPortSetInterruptMask'
+ * and 'vPortClearInterruptMask'. At the time of calling those functions, the interrupt mask is not set in the interrupt controller, thus the only protecting barrier
+ * against the CPU traping into recursive interrupt was the IRQ Enable bit in the CPSR. By not taking it into acount, the very code that protects the CPU against
+ * critical section violation just enabled it to happen : A SysTick was waiting to happen, and calling 'portCPU_IRQ_ENABLE' would enable it to occur... Thus triggering a
+ * switch of context while already performing a switch context. */
+ if(!wasIRQDisabled)
+ portCPU_IRQ_ENABLE();
return ulReturn;
}
