From 29b202a41e0e2810272b5765805d9fa4af247736 Mon Sep 17 00:00:00 2001 From: Gabriel Staples Date: Mon, 13 May 2024 04:19:50 -0700 Subject: [PATCH 1/3] list.c: improve code comments to point to official documentation about problems which may cause code to get stuck inside of list.c (#1051) list.c: improve documentation about initializing binary semaphores --- list.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/list.c b/list.c index fc99538b24..e6dbaaa3ab 100644 --- a/list.c +++ b/list.c @@ -184,7 +184,14 @@ void vListInsert( List_t * const pxList, * 4) Using a queue or semaphore before it has been initialised or * before the scheduler has been started (are interrupts firing * before vTaskStartScheduler() has been called?). - * 5) If the FreeRTOS port supports interrupt nesting then ensure that + * 5) Attempting to 'take' binary semaphores created using + * `xSemaphoreCreateBinary()` or `xSemaphoreCreateBinaryStatic()` + * APIs, before 'giving' them. Binary semaphores created using + * `xSemaphoreCreateBinary()` or `xSemaphoreCreateBinaryStatic()`, + * are created in a state such that the semaphore must first be + * 'given' using xSemaphoreGive() API before it can be 'taken' using + * xSemaphoreTake() API. + * 6) If the FreeRTOS port supports interrupt nesting then ensure that * the priority of the tick interrupt is at or below * configMAX_SYSCALL_INTERRUPT_PRIORITY. **********************************************************************/ @@ -192,7 +199,9 @@ void vListInsert( List_t * const pxList, for( pxIterator = ( ListItem_t * ) &( pxList->xListEnd ); pxIterator->pxNext->xItemValue <= xValueOfInsertion; pxIterator = pxIterator->pxNext ) { /* There is nothing to do here, just iterating to the wanted - * insertion position. */ + * insertion position. + * IF YOU FIND YOUR CODE STUCK HERE, SEE THE NOTE JUST ABOVE. + */ } } From 2e0c623351f03f2fb94c5aab151b78e8315edb19 Mon Sep 17 00:00:00 2001 From: Gaurav-Aggarwal-AWS <33462878+aggarg@users.noreply.github.com> Date: Tue, 14 May 2024 16:15:54 +0530 Subject: [PATCH 2/3] Fix race in prvProcessSimulatedInterrupts (#1055) Earlier the code was suspending the current thread after calling vTaskSwitchContext. This left a gap where the current thread could access incorrect pxCurrentTCB after it was changed by vTaskSwitchContext. This commit addresses the problem by suspending the current thread before calling vTaskSwitchContext. It was reported here - https://github.com/FreeRTOS/FreeRTOS-Kernel/issues/1054. Signed-off-by: Gaurav Aggarwal --- portable/MSVC-MingW/port.c | 59 ++++++++++++++------------------------ 1 file changed, 21 insertions(+), 38 deletions(-) diff --git a/portable/MSVC-MingW/port.c b/portable/MSVC-MingW/port.c index 8b8289b8fe..7c34aabd6a 100644 --- a/portable/MSVC-MingW/port.c +++ b/portable/MSVC-MingW/port.c @@ -435,48 +435,31 @@ static void prvProcessSimulatedInterrupts( void ) if( ulSwitchRequired != pdFALSE ) { - void * pvOldCurrentTCB; - - pvOldCurrentTCB = pxCurrentTCB; + /* Suspend the old thread. */ + pxThreadState = ( ThreadState_t * ) *( ( size_t * ) pxCurrentTCB ); + SuspendThread( pxThreadState->pvThread ); + + /* Ensure the thread is actually suspended by performing a + * synchronous operation that can only complete when the thread + * is actually suspended. The below code asks for dummy register + * data. Experimentation shows that these two lines don't appear + * to do anything now, but according to + * https://devblogs.microsoft.com/oldnewthing/20150205-00/?p=44743 + * they do - so as they do not harm (slight run-time hit). */ + xContext.ContextFlags = CONTEXT_INTEGER; + ( void ) GetThreadContext( pxThreadState->pvThread, &xContext ); /* Select the next task to run. */ vTaskSwitchContext(); - /* If the task selected to enter the running state is not the task - * that is already in the running state. */ - if( pvOldCurrentTCB != pxCurrentTCB ) - { - /* Suspend the old thread. In the cases where the (simulated) - * interrupt is asynchronous (tick event swapping a task out rather - * than a task blocking or yielding) it doesn't matter if the - * 'suspend' operation doesn't take effect immediately - if it - * doesn't it would just be like the interrupt occurring slightly - * later. In cases where the yield was caused by a task blocking - * or yielding then the task will block on a yield event after the - * yield operation in case the 'suspend' operation doesn't take - * effect immediately. */ - pxThreadState = ( ThreadState_t * ) *( ( size_t * ) pvOldCurrentTCB ); - SuspendThread( pxThreadState->pvThread ); - - /* Ensure the thread is actually suspended by performing a - * synchronous operation that can only complete when the thread is - * actually suspended. The below code asks for dummy register - * data. Experimentation shows that these two lines don't appear - * to do anything now, but according to - * https://devblogs.microsoft.com/oldnewthing/20150205-00/?p=44743 - * they do - so as they do not harm (slight run-time hit). */ - xContext.ContextFlags = CONTEXT_INTEGER; - ( void ) GetThreadContext( pxThreadState->pvThread, &xContext ); - - /* Obtain the state of the task now selected to enter the - * Running state. */ - pxThreadState = ( ThreadState_t * ) ( *( size_t * ) pxCurrentTCB ); - - /* pxThreadState->pvThread can be NULL if the task deleted - * itself - but a deleted task should never be resumed here. */ - configASSERT( pxThreadState->pvThread != NULL ); - ResumeThread( pxThreadState->pvThread ); - } + /* Obtain the state of the task now selected to enter the + * Running state. */ + pxThreadState = ( ThreadState_t * ) ( *( size_t * ) pxCurrentTCB ); + + /* pxThreadState->pvThread can be NULL if the task deleted + * itself - but a deleted task should never be resumed here. */ + configASSERT( pxThreadState->pvThread != NULL ); + ResumeThread( pxThreadState->pvThread ); } /* If the thread that is about to be resumed stopped running From a8376dbe816b230985db09d9b9203c33cdf4fc66 Mon Sep 17 00:00:00 2001 From: Gaurav-Aggarwal-AWS <33462878+aggarg@users.noreply.github.com> Date: Tue, 14 May 2024 16:24:49 +0530 Subject: [PATCH 3/3] Revert the change introduced in PR #1051 (#1056) As pointed out by Jeff Tenney, the comment introduced in the PR is not accurate. Signed-off-by: Gaurav Aggarwal --- list.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/list.c b/list.c index e6dbaaa3ab..eb9efbd2d4 100644 --- a/list.c +++ b/list.c @@ -184,14 +184,7 @@ void vListInsert( List_t * const pxList, * 4) Using a queue or semaphore before it has been initialised or * before the scheduler has been started (are interrupts firing * before vTaskStartScheduler() has been called?). - * 5) Attempting to 'take' binary semaphores created using - * `xSemaphoreCreateBinary()` or `xSemaphoreCreateBinaryStatic()` - * APIs, before 'giving' them. Binary semaphores created using - * `xSemaphoreCreateBinary()` or `xSemaphoreCreateBinaryStatic()`, - * are created in a state such that the semaphore must first be - * 'given' using xSemaphoreGive() API before it can be 'taken' using - * xSemaphoreTake() API. - * 6) If the FreeRTOS port supports interrupt nesting then ensure that + * 5) If the FreeRTOS port supports interrupt nesting then ensure that * the priority of the tick interrupt is at or below * configMAX_SYSCALL_INTERRUPT_PRIORITY. **********************************************************************/