Merge pull request #21895 from maribu/core/thread_flags_group/fix

core/thread_flags_group: use portable implementation

core/include/thread_flags.h

@@ -182,19 +182,26 @@ thread_flags_t thread_flags_wait_all(thread_flags_t mask);
 thread_flags_t thread_flags_wait_one(thread_flags_t mask);
 
 /**
- * @brief Possibly Wake up thread waiting for flags
- *
- * Wakes up a thread if it is thread flag blocked and its condition is met.
- * Has to be called with interrupts disabled.
- * Does not trigger yield.
+ * @brief   Set the flags of the given thread and update its state, but do not
+ *          yield
  *
  * @internal
  *
- * @param[in]   thread  thread to possibly wake up
- * @return      1       if @p thread has been woken up
- *              0       otherwise
+ * @warning This is not a stable API intended for external use.
+ *
+ * @param[in,out]   thread  Thread to modify
+ * @param[in]       mask    Bitmask containing the flags to set
+ *
+ * @retval  true    The thread in @p thread has changed its state to pending,
+ *                  the caller needs to yield
+ * @retval  false   The thread in @p has not changed its state. It may already
+ *                  be pending or is not waiting on the flags in @p mask
+ *
+ * @pre     The caller has IRQ disabled.
+ * @pre     @p thread is not `NULL`
+ * @pre     The caller is prepared to yield if this function returns `true`
  */
-int thread_flags_wake(thread_t *thread);
+bool thread_flags_set_internal(thread_t *thread, thread_flags_t mask);
 
 #ifdef __cplusplus
 }

core/msg.c

@@ -58,8 +58,7 @@ static int queue_msg(thread_t *target, const msg_t *m)
 
     *dest = *m;
 #if MODULE_CORE_THREAD_FLAGS
-    target->flags |= THREAD_FLAG_MSG_WAITING;
-    thread_flags_wake(target);
+    thread_flags_set_internal(target, THREAD_FLAG_MSG_WAITING);
 #endif
     return 1;
 }
@@ -157,8 +156,7 @@ static int _msg_send(msg_t *m, kernel_pid_t target_pid, bool block,
         thread_add_to_list(&(target->msg_waiters), me);
 
 #if MODULE_CORE_THREAD_FLAGS
-        target->flags |= THREAD_FLAG_MSG_WAITING;
-        thread_flags_wake(target);
+        thread_flags_set_internal(target, THREAD_FLAG_MSG_WAITING);
 #endif
 
         irq_restore(state);

core/thread_flags.c

@@ -53,11 +53,6 @@ static inline int __attribute__((always_inline)) _thread_flags_wake(
     return wakeup;
 }
 
-int thread_flags_wake(thread_t *thread)
-{
-    return _thread_flags_wake(thread);
-}
-
 static thread_flags_t _thread_flags_clear_atomic(thread_t *thread,
                                                  thread_flags_t mask)
 {
@@ -142,6 +137,12 @@ thread_flags_t thread_flags_wait_all(thread_flags_t mask)
     return _thread_flags_clear_atomic(me, mask);
 }
 
+bool thread_flags_set_internal(thread_t *thread, thread_flags_t mask)
+{
+    thread->flags |= mask;
+    return _thread_flags_wake(thread);
+}
+
 void thread_flags_set(thread_t *thread, thread_flags_t mask)
 {
     DEBUG("thread_flags_set(): setting 0x%08x for pid %" PRIkernel_pid "\n",

core/thread_flags_group.c

@@ -18,17 +18,26 @@
  * @}
  */
 
+ #include <stdbool.h>
+
 #include "bitarithm.h"
 #include "irq.h"
 #include "thread.h"
 #include "thread_flags_group.h"
 
+#define ENABLE_DEBUG 0
+#include "debug.h"
+
 void thread_flags_group_set(thread_flags_group_t *group, thread_flags_t mask)
 {
     /* Interrupts must be disabled because the threads are not ordered by
      * priority. */
     unsigned irq_state = irq_disable();
 
+    DEBUG("thread_flags_group_set(%p, %x):\n", (void *)group, (unsigned)mask);
+    DEBUG("| TID | Flags | Status (old) | Status (new) |\n");
+
+    bool yield = false;
     for (kernel_pid_t i = 0; i < (kernel_pid_t)ARRAY_SIZE(group->members); i++) {
         unsigned pid_block = group->members[i];
         kernel_pid_t const pid_base = i * UINT_WIDTH;
@@ -36,9 +45,31 @@ void thread_flags_group_set(thread_flags_group_t *group, thread_flags_t mask)
 
         while (pid_block) {
             pid_block = bitarithm_test_and_clear(pid_block, &pid_offs);
-            thread_flags_set(thread_get(pid_base + pid_offs), mask);
+            kernel_pid_t target_pid = pid_base + pid_offs;
+            thread_t *target = thread_get(target_pid);
+            if (!target) {
+                DEBUG("|  %02u |   n/a |   n/a (dead) |   n/a (dead) |\n",
+                      target_pid);
+                continue;
+            }
+            thread_status_t old_status = target->status;
+            bool awoken = thread_flags_set_internal(target, mask);
+            thread_status_t new_status = target->status;
+            /* NOTE: wait_data is shared by thread_flags with other
+             *       mechanisms and may e.g. be a pointer to an `msg_t`.
+             *       Some interpretation of the output is needed by the
+             *       user of the debug output. */
+            thread_flags_t wait_data = (uint16_t)(uintptr_t)target->wait_data;
+            DEBUG("|  %02u |  %04x | %12s | %12s |\n",
+                  (unsigned)target_pid, (unsigned)wait_data,
+                  thread_state_to_string(old_status),
+                  thread_state_to_string(new_status));
+            yield = yield || awoken;
         }
     }
 
     irq_restore(irq_state);
+    if (yield) {
+        thread_yield_higher();
+    }
 }