Merge pull request #7423 from mprse/mutex_lock_assert

Fix issue #6872 - Mutex lock has possibility to fail at runtime (returning status flag)

Author: adbridge

TESTS/mbedmicro-rtos-mbed/mutex/main.cpp

@@ -48,9 +48,9 @@ volatile bool mutex_defect = false;
 bool manipulate_protected_zone(const int thread_delay)
 {
     bool result = true;
+    osStatus stat;
 
-    osStatus stat = stdio_mutex.lock();
-    TEST_ASSERT_EQUAL(osOK, stat);
+    stdio_mutex.lock();
 
     core_util_critical_section_enter();
     if (changing_counter == true) {
@@ -68,8 +68,8 @@ bool manipulate_protected_zone(const int thread_delay)
     changing_counter = false;
     core_util_critical_section_exit();
 
-    stat = stdio_mutex.unlock();
-    TEST_ASSERT_EQUAL(osOK, stat);
+    stdio_mutex.unlock();
+
     return result;
 }
 
@@ -118,20 +118,18 @@ void test_multiple_threads(void)
 
 void test_dual_thread_nolock_lock_thread(Mutex *mutex)
 {
-    osStatus stat = mutex->lock(osWaitForever);
-    TEST_ASSERT_EQUAL(osOK, stat);
+    osStatus stat;
+    mutex->lock();
 
-    stat = mutex->unlock();
-    TEST_ASSERT_EQUAL(osOK, stat);
+    mutex->unlock();
 }
 
 void test_dual_thread_nolock_trylock_thread(Mutex *mutex)
 {
     bool stat_b = mutex->trylock();
     TEST_ASSERT_EQUAL(true, stat_b);
 
-    osStatus stat = mutex->unlock();
-    TEST_ASSERT_EQUAL(osOK, stat);
+    mutex->unlock();
 }
 
 /** Test dual thread no-lock
@@ -140,13 +138,13 @@ void test_dual_thread_nolock_trylock_thread(Mutex *mutex)
     Given two threads A & B and a mutex
     When thread A creates a mutex and starts thread B
         and thread B calls @a lock and @a unlock
-    Then returned statuses are osOK
+    Then @a lock and @a unlock operations are successfully performed.
 
     Test dual thread second thread trylock
     Given two threads A & B and a mutex
     When thread A creates a mutex and starts thread B
         and thread B calls @a trylock and @a unlock
-    Then returned statuses are true and osOK
+    Then @a trylock and @a unlock operations are successfully performed.
 */
 template <void (*F)(Mutex *)>
 void test_dual_thread_nolock(void)
@@ -161,8 +159,7 @@ void test_dual_thread_nolock(void)
 
 void test_dual_thread_lock_unlock_thread(Mutex *mutex)
 {
-    osStatus stat = mutex->lock(osWaitForever);
-    TEST_ASSERT_EQUAL(osOK, stat);
+    mutex->lock();
 }
 
 /** Test dual thread lock unlock
@@ -180,13 +177,11 @@ void test_dual_thread_lock_unlock(void)
     osStatus stat;
     Thread thread(osPriorityNormal, TEST_STACK_SIZE);
 
-    stat = mutex.lock();
-    TEST_ASSERT_EQUAL(osOK, stat);
+    mutex.lock();
 
     thread.start(callback(test_dual_thread_lock_unlock_thread, &mutex));
 
-    stat = mutex.unlock();
-    TEST_ASSERT_EQUAL(osOK, stat);
+    mutex.unlock();
 
     wait_ms(TEST_DELAY);
 }
@@ -202,9 +197,9 @@ void test_dual_thread_lock_lock_thread(Mutex *mutex)
     Timer timer;
     timer.start();
 
-    osStatus stat = mutex->lock(TEST_DELAY);
-    TEST_ASSERT_EQUAL(osErrorTimeout, stat);
-    TEST_ASSERT_UINT32_WITHIN(5000, TEST_DELAY * 1000, timer.read_us());
+    bool stat = mutex->trylock_for(TEST_DELAY);
+    TEST_ASSERT_EQUAL(false, stat);
+    TEST_ASSERT_UINT32_WITHIN(5000, TEST_DELAY*1000, timer.read_us());
 }
 
 /** Test dual thread lock
@@ -228,46 +223,40 @@ void test_dual_thread_lock(void)
     osStatus stat;
     Thread thread(osPriorityNormal, TEST_STACK_SIZE);
 
-    stat = mutex.lock();
-    TEST_ASSERT_EQUAL(osOK, stat);
+    mutex.lock();
 
     thread.start(callback(F, &mutex));
 
     wait_ms(TEST_LONG_DELAY);
 
-    stat = mutex.unlock();
-    TEST_ASSERT_EQUAL(osOK, stat);
+    mutex.unlock();
 }
 
 /** Test single thread lock recursive
 
     Given a mutex and a single running thread
     When thread calls @a lock twice and @a unlock twice on the mutex
-    Then the returned statuses are osOK
+    Then @a lock and @a unlock operations are successfully performed.
 */
 void test_single_thread_lock_recursive(void)
 {
     Mutex mutex;
     osStatus stat;
 
-    stat = mutex.lock();
-    TEST_ASSERT_EQUAL(osOK, stat);
+    mutex.lock();
 
-    stat = mutex.lock();
-    TEST_ASSERT_EQUAL(osOK, stat);
+    mutex.lock();
 
-    stat = mutex.unlock();
-    TEST_ASSERT_EQUAL(osOK, stat);
+    mutex.unlock();
 
-    stat = mutex.unlock();
-    TEST_ASSERT_EQUAL(osOK, stat);
+    mutex.unlock();
 }
 
 /** Test single thread trylock
 
     Given a mutex and a single running thread
     When thread calls @a trylock and @a unlock on the mutex
-    Then the returned statuses are osOK
+    Then @a trylock and @a unlock operations are successfully performed.
 */
 void test_single_thread_trylock(void)
 {
@@ -276,26 +265,23 @@ void test_single_thread_trylock(void)
     bool stat_b = mutex.trylock();
     TEST_ASSERT_EQUAL(true, stat_b);
 
-    osStatus stat = mutex.unlock();
-    TEST_ASSERT_EQUAL(osOK, stat);
+    mutex.unlock();
 }
 
 /** Test single thread lock
 
     Given a mutex and a single running thread
     When thread calls @a lock and @a unlock on the mutex
-    Then the returned statuses are osOK
+    Then @a lock and @a unlock operations are successfully performed.
 */
 void test_single_thread_lock(void)
 {
     Mutex mutex;
     osStatus stat;
 
-    stat = mutex.lock();
-    TEST_ASSERT_EQUAL(osOK, stat);
+    mutex.lock();
 
-    stat = mutex.unlock();
-    TEST_ASSERT_EQUAL(osOK, stat);
+    mutex.unlock();
 }
 
 utest::v1::status_t test_setup(const size_t number_of_cases)

features/lorawan/lorastack/mac/LoRaMac.h

@@ -411,15 +411,11 @@ class LoRaMac {
 #if MBED_CONF_RTOS_PRESENT
     void lock(void)
     {
-        osStatus status = _mutex.lock();
-        MBED_ASSERT(status == osOK);
-        (void) status;
+        _mutex.lock();
     }
     void unlock(void)
     {
-        osStatus status = _mutex.unlock();
-        MBED_ASSERT(status == osOK);
-        (void) status;
+        _mutex.unlock();
     }
 #else
     void lock(void) { }

rtos/Mutex.cpp

@@ -41,7 +41,8 @@ Mutex::Mutex(const char *name)
 void Mutex::constructor(const char *name)
 {
     memset(&_obj_mem, 0, sizeof(_obj_mem));
-    osMutexAttr_t attr = { 0 };
+    osMutexAttr_t attr =
+    { 0 };
     attr.name = name ? name : "aplication_unnamed_mutex";
     attr.cb_mem = &_obj_mem;
     attr.cb_size = sizeof(_obj_mem);
@@ -50,30 +51,63 @@ void Mutex::constructor(const char *name)
     MBED_ASSERT(_id);
 }
 
-osStatus Mutex::lock(uint32_t millisec) {
+osStatus Mutex::lock(void)
+{
+    osStatus status = osMutexAcquire(_id, osWaitForever);
+    if (osOK == status) {
+        _count++;
+    }
+
+    if (status != osOK) {
+        MBED_ERROR1(MBED_MAKE_ERROR(MBED_MODULE_KERNEL, MBED_ERROR_CODE_MUTEX_LOCK_FAILED), "Mutex lock failed", status);
+    }
+
+    return osOK;
+}
+
+osStatus Mutex::lock(uint32_t millisec)
+{
     osStatus status = osMutexAcquire(_id, millisec);
     if (osOK == status) {
         _count++;
     }
+
+    bool success = (status == osOK ||
+                    (status == osErrorResource && millisec == 0) ||
+                    (status == osErrorTimeout && millisec != osWaitForever));
+
+    if (!success) {
+        MBED_ERROR1(MBED_MAKE_ERROR(MBED_MODULE_KERNEL, MBED_ERROR_CODE_MUTEX_LOCK_FAILED), "Mutex lock failed", status);
+    }
+
     return status;
 }
 
-bool Mutex::trylock() {
+bool Mutex::trylock()
+{
     return trylock_for(0);
 }
 
-bool Mutex::trylock_for(uint32_t millisec) {
-    osStatus status = lock(millisec);
+bool Mutex::trylock_for(uint32_t millisec)
+{
+    osStatus status = osMutexAcquire(_id, millisec);
     if (status == osOK) {
         return true;
     }
 
-    MBED_ASSERT(status == osErrorTimeout || status == osErrorResource);
+    bool success = (status == osOK ||
+                    (status == osErrorResource && millisec == 0) ||
+                    (status == osErrorTimeout && millisec != osWaitForever));
+
+    if (!success) {
+        MBED_ERROR1(MBED_MAKE_ERROR(MBED_MODULE_KERNEL, MBED_ERROR_CODE_MUTEX_LOCK_FAILED), "Mutex lock failed", status);
+    }
 
     return false;
 }
 
-bool Mutex::trylock_until(uint64_t millisec) {
+bool Mutex::trylock_until(uint64_t millisec)
+{
     uint64_t now = Kernel::get_ms_count();
 
     if (now >= millisec) {
@@ -86,16 +120,26 @@ bool Mutex::trylock_until(uint64_t millisec) {
     }
 }
 
-osStatus Mutex::unlock() {
+osStatus Mutex::unlock()
+{
     _count--;
-    return osMutexRelease(_id);
+
+    osStatus status = osMutexRelease(_id);
+
+    if (status != osOK) {
+        MBED_ERROR1(MBED_MAKE_ERROR(MBED_MODULE_KERNEL, MBED_ERROR_CODE_MUTEX_UNLOCK_FAILED), "Mutex unlock failed", status);
+    }
+
+    return osOK;
 }
 
-osThreadId Mutex::get_owner() {
+osThreadId Mutex::get_owner()
+{
     return osMutexGetOwner(_id);
 }
 
-Mutex::~Mutex() {
+Mutex::~Mutex()
+{
     osMutexDelete(_id);
 }
 

rtos/Mutex.h

@@ -29,6 +29,7 @@
 
 #include "platform/NonCopyable.h"
 #include "platform/ScopedLock.h"
+#include "platform/mbed_toolchain.h"
 
 namespace rtos {
 /** \addtogroup rtos */
@@ -73,23 +74,39 @@ class Mutex : private mbed::NonCopyable<Mutex> {
     /** Create and Initialize a Mutex object
 
      @param name name to be used for this mutex. It has to stay allocated for the lifetime of the thread.
-
      @note You cannot call this function from ISR context.
     */
     Mutex(const char *name);
 
-    /** Wait until a Mutex becomes available.
-      @param   millisec  timeout value or 0 in case of no time-out. (default: osWaitForever)
+    /**
+      Wait until a Mutex becomes available.
+
+      @return  status code that indicates the execution status of the function:
+               @a osOK the mutex has been obtained.
+
+      @note You cannot call this function from ISR context.
+      @note This function treats RTOS errors as fatal system errors, so can only return osOK.
+            Use of the return value is deprecated, as the return is expected to become void in the future.
+     */
+    osStatus lock(void);
+
+    /**
+      For backwards compatibility.
+      @deprecated Do not use this function. This function has been replaced with lock(), trylock() and trylock_for() functions.
+
+      Wait until a Mutex becomes available.
+      @param   millisec  timeout value or 0 in case of no time-out.
       @return  status code that indicates the execution status of the function:
                @a osOK the mutex has been obtained.
                @a osErrorTimeout the mutex could not be obtained in the given time.
-               @a osErrorParameter internal error.
                @a osErrorResource the mutex could not be obtained when no timeout was specified.
-               @a osErrorISR this function cannot be called from the interrupt service routine.
 
       @note You cannot call this function from ISR context.
+      @note This function treats RTOS errors as fatal system errors, so can only return osOK or
+            osErrorResource in case when millisec is 0 or osErrorTimeout if millisec is not osWaitForever.
      */
-    osStatus lock(uint32_t millisec=osWaitForever);
+    MBED_DEPRECATED_SINCE("mbed-os-5.10.0", "Replaced with lock(), trylock() and trylock_for() functions")
+    osStatus lock(uint32_t millisec);
 
     /** Try to lock the mutex, and return immediately
       @return true if the mutex was acquired, false otherwise.
@@ -123,14 +140,15 @@ class Mutex : private mbed::NonCopyable<Mutex> {
      */
     bool trylock_until(uint64_t millisec);
 
-    /** Unlock the mutex that has previously been locked by the same thread
+    /**
+      Unlock the mutex that has previously been locked by the same thread
+
       @return status code that indicates the execution status of the function:
               @a osOK the mutex has been released.
-              @a osErrorParameter internal error.
-              @a osErrorResource the mutex was not locked or the current thread wasn't the owner.
-              @a osErrorISR this function cannot be called from the interrupt service routine.
 
       @note You cannot call this function from ISR context.
+      @note This function treats RTOS errors as fatal system errors, so can only return osOK.
+            Use of the return value is deprecated, as the return is expected to become void in the future.
      */
     osStatus unlock();