Merge branch 'bugfix/freertos_event_group_unblock_race_condition_v4.2' into 'release/v4.2'

FreeRTOS: Fix event group task list race condition (v4.2)

See merge request espressif/esp-idf!19105

components/freertos/event_groups.c

@@ -565,6 +565,10 @@ BaseType_t xMatchFound = pdFALSE;
 
 	vTaskSuspendAll();
 	taskENTER_CRITICAL(&pxEventBits->eventGroupMux);
+	/* The critical section above only takes the event groups spinlock. However, we are about to traverse a task list.
+	Thus we need call the function below to take the task list spinlock located in tasks.c. Not doing so will risk
+	the task list's being changed while be are traversing it. */
+	vTaskTakeEventListLock();
 	{
 		traceEVENT_GROUP_SET_BITS( xEventGroup, uxBitsToSet );
 
@@ -636,6 +640,8 @@ BaseType_t xMatchFound = pdFALSE;
 		bit was set in the control word. */
 		pxEventBits->uxEventBits &= ~uxBitsToClear;
 	}
+	/* Release the previously held task list spinlock, then release the event group spinlock. */
+	vTaskReleaseEventListLock();
 	taskEXIT_CRITICAL(&pxEventBits->eventGroupMux);
 	( void ) xTaskResumeAll();
 
@@ -650,6 +656,10 @@ void vEventGroupDelete( EventGroupHandle_t xEventGroup )
 
 	vTaskSuspendAll();
 	taskENTER_CRITICAL( &pxEventBits->eventGroupMux );
+	/* The critical section above only takes the event groups spinlock. However, we are about to traverse a task list.
+	Thus we need call the function below to take the task list spinlock located in tasks.c. Not doing so will risk
+	the task list's being changed while be are traversing it. */
+	vTaskTakeEventListLock();
 	{
 		traceEVENT_GROUP_DELETE( xEventGroup );
 
@@ -661,6 +671,9 @@ void vEventGroupDelete( EventGroupHandle_t xEventGroup )
 			( void ) xTaskRemoveFromUnorderedEventList( pxTasksWaitingForBits->xListEnd.pxNext, eventUNBLOCKED_DUE_TO_BIT_SET );
 		}
 
+		/* Release the previously held task list spinlock. */
+		vTaskReleaseEventListLock();
+
 		#if( ( configSUPPORT_DYNAMIC_ALLOCATION == 1 ) && ( configSUPPORT_STATIC_ALLOCATION == 0 ) )
 		{
 			/* The event group can only have been allocated dynamically - free

components/freertos/include/freertos/task.h

@@ -2152,6 +2152,23 @@ void vTaskPlaceOnUnorderedEventList( List_t * pxEventList, const TickType_t xIte
  */
 void vTaskPlaceOnEventListRestricted( List_t * const pxEventList, const TickType_t xTicksToWait ) PRIVILEGED_FUNCTION;
 
+/*
+ * THIS FUNCTION MUST NOT BE USED FROM APPLICATION CODE.  IT IS AN
+ * INTERFACE WHICH IS FOR THE EXCLUSIVE USE OF THE SCHEDULER.
+ *
+ * This function is a wrapper to take the "xTaskQueueMutex" spinlock of tasks.c.
+ * This lock is taken whenver any of the task lists or event lists are
+ * accessed/modified, such as when adding/removing tasks to/from the delayed
+ * task list or various event lists.
+ *
+ * This functions is meant to be called by xEventGroupSetBits() and
+ * vEventGroupDelete() as both those functions will access event lists (instead
+ * of delegating the entire responsibility to one of vTask...EventList()
+ * functions).
+ */
+void vTaskTakeEventListLock( void );
+void vTaskReleaseEventListLock( void );
+
 /*
  * THIS FUNCTION MUST NOT BE USED FROM APPLICATION CODE.  IT IS AN
  * INTERFACE WHICH IS FOR THE EXCLUSIVE USE OF THE SCHEDULER.

components/freertos/tasks.c

@@ -2865,11 +2865,11 @@ void vTaskSwitchContext( void )
 			--uxDynamicTopReady;
 		}
 
-#else 
+#else
 		//For Unicore targets we can keep the current FreeRTOS O(1)
-		//Scheduler. I hope to optimize better the scheduler for 
+		//Scheduler. I hope to optimize better the scheduler for
 		//Multicore settings -- This will involve to create a per
-		//affinity ready task list which will impact hugely on 
+		//affinity ready task list which will impact hugely on
 		//tasks module
 		taskSELECT_HIGHEST_PRIORITY_TASK();
 #endif
@@ -3168,6 +3168,18 @@ UBaseType_t i, uxTargetCPU;
 }
 /*-----------------------------------------------------------*/
 
+void vTaskTakeEventListLock( void )
+{
+	/* We call the tasks.c critical section macro to take xTaskQueueMutex */
+	taskENTER_CRITICAL(&xTaskQueueMutex);
+}
+
+void vTaskReleaseEventListLock( void )
+{
+	/* We call the tasks.c critical section macro to release xTaskQueueMutex */
+	taskEXIT_CRITICAL(&xTaskQueueMutex);
+}
+
 BaseType_t xTaskRemoveFromUnorderedEventList( ListItem_t * pxEventListItem, const TickType_t xItemValue )
 {
 TCB_t *pxUnblockedTCB;

components/freertos/test/test_freertos_eventgroups.c

@@ -7,6 +7,7 @@
 #include "freertos/event_groups.h"
 #include "driver/timer.h"
 #include "unity.h"
+#include "test_utils.h"
 
 #ifdef CONFIG_IDF_TARGET_ESP32S2
 #define int_clr_timers int_clr
@@ -40,7 +41,8 @@ static void task_event_group_call_response(void *param)
 
     printf("Task %d done\n", task_num);
     xSemaphoreGive(done_sem);
-    vTaskDelete(NULL);
+    // Wait to be deleted
+    vTaskSuspend(NULL);
 }
 
 TEST_CASE("FreeRTOS Event Groups", "[freertos]")
@@ -48,6 +50,8 @@ TEST_CASE("FreeRTOS Event Groups", "[freertos]")
     eg = xEventGroupCreate();
     done_sem = xSemaphoreCreateCounting(NUM_TASKS, 0);
 
+    TaskHandle_t task_handles[NUM_TASKS];
+
     /* Note: task_event_group_call_response all have higher priority than this task, so on this core
        they will always preempt this task.
 
@@ -55,7 +59,7 @@ TEST_CASE("FreeRTOS Event Groups", "[freertos]")
        or they get out of sync.
      */
     for (int c = 0; c < NUM_TASKS; c++) {
-        xTaskCreatePinnedToCore(task_event_group_call_response, "tsk_call_resp", 4096, (void *)c, configMAX_PRIORITIES - 1, NULL, c % portNUM_PROCESSORS);
+        xTaskCreatePinnedToCore(task_event_group_call_response, "tsk_call_resp", 4096, (void *)c, configMAX_PRIORITIES - 1, &task_handles[c], c % portNUM_PROCESSORS);
     }
 
     /* Tasks all start instantly, but this task will resume running at the same time as the higher priority tasks on the
@@ -66,15 +70,19 @@ TEST_CASE("FreeRTOS Event Groups", "[freertos]")
         /* signal all tasks with "CALL" bit... */
         xEventGroupSetBits(eg, BIT_CALL);
 
-        /* Only wait for 1 tick, the wakeup should be immediate... */
-        TEST_ASSERT_EQUAL_HEX16(ALL_RESPONSE_BITS, xEventGroupWaitBits(eg, ALL_RESPONSE_BITS, true, true, 1));
+        /* Wait until all tasks have set their respective response bits */
+        TEST_ASSERT_EQUAL_HEX16(ALL_RESPONSE_BITS, xEventGroupWaitBits(eg, ALL_RESPONSE_BITS, true, true, portMAX_DELAY));
     }
 
-    /* Ensure all tasks cleaned up correctly */
+    /* Ensure all tasks have suspend themselves */
     for (int c = 0; c < NUM_TASKS; c++) {
         TEST_ASSERT( xSemaphoreTake(done_sem, 100/portTICK_PERIOD_MS) );
     }
 
+    for (int c = 0; c < NUM_TASKS; c++) {
+        test_utils_task_delete(task_handles[c]);
+    }
+
     vSemaphoreDelete(done_sem);
     vEventGroupDelete(eg);
 }