From 754739c7d51cf2ddce914fb321eb16e037aa9ae6 Mon Sep 17 00:00:00 2001 From: Austin Foxley Date: Mon, 9 Oct 2023 19:49:46 +0000 Subject: [PATCH] pw_thread_freertos: Fix extra wakeups when detaching threads vTaskResume() is known to cause premature return from blocking APIs, since it will indiscriminately call prvAddTaskToReadyList(). Instead, use vTaskSuspendAll() / xTaskResumeAll(), which will not wake up any tasks which weren't already running (and weren't signalled during scheduler suspension). This also fixes a latent bug where, in the existing fallback case of using vTaskSuspendAll(), the new task scheduler state will be taskSCHEDULER_SUSPENDED, causing the subsequent xTaskResumeAll() to be skipped. Bug: b/303885539 Change-Id: I4d09400e133b4d44353d601d07e13917e096a3eb Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/175310 Reviewed-by: Ewout van Bekkum Reviewed-by: Christoph Klee Commit-Queue: Auto-Submit Pigweed-Auto-Submit: Austin Foxley Reviewed-by: Jonathon Reinhart --- pw_thread_freertos/thread.cc | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/pw_thread_freertos/thread.cc b/pw_thread_freertos/thread.cc index a7e36814d..de9f0041d 100644 --- a/pw_thread_freertos/thread.cc +++ b/pw_thread_freertos/thread.cc @@ -193,22 +193,20 @@ Thread::Thread(const thread::Options& facade_options, void Thread::detach() { PW_CHECK(joinable()); - if (xTaskGetSchedulerState() == taskSCHEDULER_RUNNING) { -#if (INCLUDE_vTaskSuspend == 1) - // No need to suspend extra tasks. - vTaskSuspend(native_type_->task_handle()); -#else + // xTaskResumeAll() can only be used after the scheduler has been started. + const bool scheduler_initialized = + xTaskGetSchedulerState() != taskSCHEDULER_NOT_STARTED; + + if (scheduler_initialized) { + // We don't want to individually suspend and resume this task using + // vTaskResume() as that can cause tasks to prematurely wake up and return + // from blocking APIs (b/303885539). vTaskSuspendAll(); -#endif // INCLUDE_vTaskSuspend == 1 } native_type_->set_detached(); const bool thread_done = native_type_->thread_done(); - if (xTaskGetSchedulerState() == taskSCHEDULER_RUNNING) { -#if (INCLUDE_vTaskSuspend == 1) - vTaskResume(native_type_->task_handle()); -#else + if (scheduler_initialized) { xTaskResumeAll(); -#endif // INCLUDE_vTaskSuspend == 1 } if (thread_done) {