portability: cmsis: Bugfix osThreadFlagsWait behavior

The osFlagsWaitAny option is not handled correctly.  It returns once any
flag at all is set, whereas it is supposed to only return once one of
the flags specified in flags is set.

Signed-off-by: Utsav Munendra <[email protected]>

Author: utsavm9

subsys/portability/cmsis_rtos_v2/thread_flags.c

@@ -109,6 +109,24 @@ uint32_t osThreadFlagsWait(uint32_t flags, uint32_t options, uint32_t timeout)
 
 		time_stamp_start = (uint64_t)k_cycle_get_32();
 
+		sig = tid->signal_results & flags;
+
+		if (options & osFlagsWaitAll) {
+			/* Check if all events we are waiting on have
+			 * been signalled
+			 */
+			if (sig == flags) {
+				break;
+			}
+		} else {
+			/* Check if any events we are waiting on have
+			 * been signalled
+			 */
+			if (sig != 0) {
+				break;
+			}
+		}
+
 		switch (timeout) {
 		case 0:
 			retval = k_poll(&tid->poll_event, 1, K_NO_WAIT);
@@ -138,40 +156,27 @@ uint32_t osThreadFlagsWait(uint32_t flags, uint32_t options, uint32_t timeout)
 		tid->poll_event.signal->signaled = 0U;
 		tid->poll_event.state = K_POLL_STATE_NOT_READY;
 
-		if (options & osFlagsWaitAll) {
-			/* Check if all events we are waiting on have
-			 * been signalled
-			 */
-			if ((tid->signal_results & flags) == flags) {
-				break;
-			}
+		/* If we need to wait on more signals, we need to
+		 * adjust the timeout value accordingly based on
+		 * the time that has already elapsed.
+		 */
+		hwclk_cycles_delta = (uint64_t)k_cycle_get_32() - time_stamp_start;
 
-			/* If we need to wait on more signals, we need to
-			 * adjust the timeout value accordingly based on
-			 * the time that has already elapsed.
-			 */
-			hwclk_cycles_delta = (uint64_t)k_cycle_get_32() - time_stamp_start;
-
-			time_delta_ns = (uint32_t)k_cyc_to_ns_floor64(hwclk_cycles_delta);
+		time_delta_ns = (uint32_t)k_cyc_to_ns_floor64(hwclk_cycles_delta);
 
-			time_delta_ms = (uint32_t)time_delta_ns / NSEC_PER_MSEC;
+		time_delta_ms = (uint32_t)time_delta_ns / NSEC_PER_MSEC;
 
-			if (timeout_ms > time_delta_ms) {
-				timeout_ms -= time_delta_ms;
-			} else {
-				timeout_ms = 0U;
-			}
+		if (timeout_ms > time_delta_ms) {
+			timeout_ms -= time_delta_ms;
 		} else {
-			break;
+			timeout_ms = 0U;
 		}
 	}
 
-	sig = tid->signal_results;
 	if (!(options & osFlagsNoClear)) {
-
 		/* Clear signal flags as the thread is ready now */
 		key = irq_lock();
-		tid->signal_results &= ~(flags);
+		tid->signal_results &= ~(sig);
 		irq_unlock(key);
 	}
 

tests/subsys/portability/cmsis_rtos_v2/src/thread_flags.c

@@ -6,14 +6,16 @@
 
 #include <zephyr/ztest.h>
 #include <zephyr/kernel.h>
-#include <cmsis_os2.h>
+#include <zephyr/portability/cmsis_os2.h>
+#include <zephyr/portability/cmsis_types.h>
 
 #include <zephyr/irq_offload.h>
 #include <zephyr/kernel_structs.h>
 
-#define TIMEOUT_TICKS (10)
+#define TIMEOUT_TICKS (1000)
 #define FLAG1         (0x00000020)
 #define FLAG2         (0x00000004)
+#define FLAG3         (0x00000100)
 #define FLAG          (FLAG1 | FLAG2)
 #define ISR_FLAG      (0x50)
 #define STACKSZ       CONFIG_CMSIS_V2_THREAD_MAX_STACK_SIZE
@@ -34,6 +36,10 @@ static void thread1(void *arg)
 	flags = osThreadFlagsGet();
 	zassert_equal(flags & FLAG1, FLAG1, "");
 
+	/* We should be able to get the exact same flags again as they were not cleared */
+	flags = osThreadFlagsWait(FLAG1, osFlagsWaitAny | osFlagsNoClear, 0);
+	zassert_equal(flags & FLAG1, FLAG1, "");
+
 	/* Clear the Flag explicitly */
 	flags = osThreadFlagsClear(FLAG1);
 	zassert_not_equal(flags, osFlagsErrorParameter, "ThreadFlagsClear failed");
@@ -161,4 +167,64 @@ ZTEST(cmsis_thread_flags, test_thread_flags_isr)
 
 	osDelay(TIMEOUT_TICKS);
 }
+
+static K_THREAD_STACK_DEFINE(test_stack4, STACKSZ);
+static struct cmsis_rtos_thread_cb test_cb4;
+static osThreadAttr_t thread4_attr = {
+	.name = "Thread4",
+	.cb_mem = &test_cb4,
+	.cb_size = sizeof(test_cb4),
+	.stack_mem = &test_stack4,
+	.stack_size = STACKSZ,
+	.priority = osPriorityHigh,
+};
+static bool m_thread_4_is_blocked;
+
+static void thread4(void *arg)
+{
+	uint32_t flags;
+
+	/* Nothing will trigger FLAG1 to this thread, so the following should timeout */
+	flags = osThreadFlagsWait(FLAG1, osFlagsWaitAny, 0);
+	zassert_equal(flags, osFlagsErrorTimeout,
+		      "ThreadFlagsWait unexpected found 0x%x flags were set");
+
+	flags = osThreadFlagsWait(FLAG1, osFlagsWaitAll, TIMEOUT_TICKS / 10);
+	zassert_equal(flags, osFlagsErrorTimeout,
+		      "ThreadFlagsWait unexpected found 0x%x flags were set");
+
+	flags = osThreadFlagsWait(FLAG1, osFlagsWaitAny | osFlagsNoClear, 0);
+	zassert_equal(flags, osFlagsErrorTimeout,
+		      "ThreadFlagsWait unexpected found 0x%x flags were set");
+
+	/* Nothing will trigger FLAG1 to this thread, so it should remain blocked here */
+	m_thread_4_is_blocked = true;
+	flags = osThreadFlagsWait(FLAG1, osFlagsWaitAny, osWaitForever);
+	zassert_unreachable();
+}
+
+ZTEST(cmsis_thread_flags, test_thread_flags_set_flags_not_waited_upon)
+{
+	osThreadId_t id;
+	uint32_t flags;
+
+	m_thread_4_is_blocked = false;
+
+	id = osThreadNew(thread4, NULL, &thread4_attr);
+	zassert_true(id != NULL, "Failed creating thread3");
+
+	/* The thread will wait on any of FLAG1. Signal something it is not waiting for. */
+	flags = osThreadFlagsSet(id, FLAG3);
+	zassert_equal(flags & FLAG3, FLAG3, "");
+	osThreadYield();
+
+	/* Wait a bit, but thread4 should remain blocked */
+	osDelay(TIMEOUT_TICKS);
+
+	zassert_true(m_thread_4_is_blocked, "Thread 4 did run till expected point");
+
+	/* Kill the thread */
+	osThreadTerminate(id);
+}
+
 ZTEST_SUITE(cmsis_thread_flags, NULL, NULL, NULL, NULL, NULL);