Revert "sem: change sem wait to atomic operation"

This reverts commit befe29801f.

Because a few regressions have been reported and
it likely will take some time to fix them:

* for some configurations, semaphore can be used on the special
  memory region, where atomic access is not available.
  cf. https://github.com/apache/nuttx/pull/14625

* include/nuttx/lib/stdatomic.h is not compatible with
  the C11 semantics, which the change in question relies on.
  cf. https://github.com/apache/nuttx/pull/14755

sched/semaphore/sem_destroy.c

@@ -60,8 +60,6 @@
 
 int nxsem_destroy(FAR sem_t *sem)
 {
-  short old;
-
   DEBUGASSERT(sem != NULL);
 
   /* There is really no particular action that we need
@@ -74,18 +72,10 @@ int nxsem_destroy(FAR sem_t *sem)
    * leave the count unchanged but still return OK.
    */
 
-  do
+  if (sem->semcount >= 0)
     {
-      old = atomic_load(NXSEM_COUNT(sem));
+      sem->semcount = 1;
-      if (old < 0)
-        {
-          break;
-        }
     }
-  while (!atomic_compare_exchange_weak_explicit(NXSEM_COUNT(sem),
-                                                &old, 1,
-                                                memory_order_release,
-                                                memory_order_relaxed));
 
   /* Release holders of the semaphore */
 

sched/semaphore/sem_holder.c

@@ -880,7 +880,7 @@ void nxsem_canceled(FAR struct tcb_s *stcb, FAR sem_t *sem)
 {
   /* Check our assumptions */
 
-  DEBUGASSERT(atomic_load(NXSEM_COUNT(sem)) <= 0);
+  DEBUGASSERT(sem->semcount <= 0);
 
   /* Adjust the priority of every holder as necessary */
 
@@ -978,7 +978,7 @@ void nxsem_release_all(FAR struct tcb_s *htcb)
        * that was taken by sem_wait() or sem_post().
        */
 
-      atomic_fetch_add(NXSEM_COUNT(sem), 1);
+      sem->semcount++;
     }
 }
 

sched/semaphore/sem_post.c

@@ -37,11 +37,11 @@
 #include "semaphore/semaphore.h"
 
 /****************************************************************************
- * Private Functions
+ * Public Functions
  ****************************************************************************/
 
 /****************************************************************************
- * Name: nxsem_post_slow
+ * Name: nxsem_post
  *
  * Description:
  *   When a kernel thread has finished with a semaphore, it will call
@@ -69,7 +69,7 @@
  *
  ****************************************************************************/
 
-static int nxsem_post_slow(FAR sem_t *sem)
+int nxsem_post(FAR sem_t *sem)
 {
   FAR struct tcb_s *stcb = NULL;
   irqstate_t flags;
@@ -78,6 +78,8 @@ static int nxsem_post_slow(FAR sem_t *sem)
   uint8_t proto;
 #endif
 
+  DEBUGASSERT(sem != NULL);
+
   /* The following operations must be performed with interrupts
    * disabled because sem_post() may be called from an interrupt
    * handler.
@@ -85,13 +87,12 @@ static int nxsem_post_slow(FAR sem_t *sem)
 
   flags = enter_critical_section();
 
-  sem_count = atomic_fetch_add(NXSEM_COUNT(sem), 1);
+  sem_count = sem->semcount;
 
   /* Check the maximum allowable value */
 
   if (sem_count >= SEM_VALUE_MAX)
     {
-      atomic_fetch_sub(NXSEM_COUNT(sem), 1);
       leave_critical_section(flags);
       return -EOVERFLOW;
     }
@@ -114,6 +115,8 @@ static int nxsem_post_slow(FAR sem_t *sem)
    */
 
   nxsem_release_holder(sem);
+  sem_count++;
+  sem->semcount = sem_count;
 
 #if defined(CONFIG_PRIORITY_INHERITANCE) || defined(CONFIG_PRIORITY_PROTECT)
   /* Don't let any unblocked tasks run until we complete any priority
@@ -135,7 +138,7 @@ static int nxsem_post_slow(FAR sem_t *sem)
    * there must be some task waiting for the semaphore.
    */
 
-  if (sem_count < 0)
+  if (sem_count <= 0)
     {
       /* Check if there are any tasks in the waiting for semaphore
        * task list that are waiting for this semaphore.  This is a
@@ -214,60 +217,3 @@ static int nxsem_post_slow(FAR sem_t *sem)
 
   return OK;
 }
-
-/****************************************************************************
- * Public Functions
- ****************************************************************************/
-
-/****************************************************************************
- * Name: nxsem_post
- *
- * Description:
- *   When a kernel thread has finished with a semaphore, it will call
- *   nxsem_post().  This function unlocks the semaphore referenced by sem
- *   by performing the semaphore unlock operation on that semaphore.
- *
- *   If the semaphore value resulting from this operation is positive, then
- *   no tasks were blocked waiting for the semaphore to become unlocked; the
- *   semaphore is simply incremented.
- *
- *   If the value of the semaphore resulting from this operation is zero,
- *   then one of the tasks blocked waiting for the semaphore shall be
- *   allowed to return successfully from its call to nxsem_wait().
- *
- * Input Parameters:
- *   sem - Semaphore descriptor
- *
- * Returned Value:
- *   This is an internal OS interface and should not be used by applications.
- *   It follows the NuttX internal error return policy:  Zero (OK) is
- *   returned on success.  A negated errno value is returned on failure.
- *
- * Assumptions:
- *   This function may be called from an interrupt handler.
- *
- ****************************************************************************/
-
-int nxsem_post(FAR sem_t *sem)
-{
-  DEBUGASSERT(sem != NULL);
-
-  /* If this is a mutex, we can try to unlock the mutex in fast mode,
-   * else try to get it in slow mode.
-   */
-
-#if !defined(CONFIG_PRIORITY_INHERITANCE) && !defined(CONFIG_PRIORITY_PROTECT)
-  if (sem->flags & SEM_TYPE_MUTEX)
-    {
-      short old = 0;
-      if (atomic_compare_exchange_weak_explicit(NXSEM_COUNT(sem), &old, 1,
-                                                memory_order_release,
-                                                memory_order_relaxed))
-        {
-          return OK;
-        }
-    }
-#endif
-
-  return nxsem_post_slow(sem);
-}

sched/semaphore/sem_recover.c

@@ -85,7 +85,7 @@ void nxsem_recover(FAR struct tcb_s *tcb)
   if (tcb->task_state == TSTATE_WAIT_SEM)
     {
       FAR sem_t *sem = tcb->waitobj;
-      DEBUGASSERT(sem != NULL && atomic_load(NXSEM_COUNT(sem)) < 0);
+      DEBUGASSERT(sem != NULL && sem->semcount < 0);
 
       /* Restore the correct priority of all threads that hold references
        * to this semaphore.
@@ -99,7 +99,7 @@ void nxsem_recover(FAR struct tcb_s *tcb)
        * place.
        */
 
-      atomic_fetch_add(NXSEM_COUNT(sem), 1);
+      sem->semcount++;
     }
 
   /* Release all semphore holders for the task */

sched/semaphore/sem_reset.c

@@ -60,7 +60,6 @@
 int nxsem_reset(FAR sem_t *sem, int16_t count)
 {
   irqstate_t flags;
-  short semcount;
 
   DEBUGASSERT(sem != NULL && count >= 0);
 
@@ -81,7 +80,7 @@ int nxsem_reset(FAR sem_t *sem, int16_t count)
    * out counts to any waiting threads.
    */
 
-  while (atomic_load(NXSEM_COUNT(sem)) < 0 && count > 0)
+  while (sem->semcount < 0 && count > 0)
     {
       /* Give out one counting, waking up one of the waiting threads
        * and, perhaps, kicking off a lot of priority inheritance
@@ -99,18 +98,10 @@ int nxsem_reset(FAR sem_t *sem, int16_t count)
    * value of sem->semcount is already correct in this case.
    */
 
-  do
+  if (sem->semcount >= 0)
     {
-      semcount = atomic_load(NXSEM_COUNT(sem));
+      sem->semcount = count;
-      if (semcount < 0)
-        {
-          break;
-        }
     }
-  while (!atomic_compare_exchange_weak_explicit(NXSEM_COUNT(sem),
-                                                &semcount, count,
-                                                memory_order_release,
-                                                memory_order_relaxed));
 
   /* Allow any pending context switches to occur now */
 

sched/semaphore/sem_trywait.c

@@ -38,78 +38,6 @@
 #include "sched/sched.h"
 #include "semaphore/semaphore.h"
 
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
-
-/****************************************************************************
- * Name: nxsem_trywait_slow
- *
- * Description:
- *   This function locks the specified semaphore in slow mode.
- *
- * Input Parameters:
- *   sem - the semaphore descriptor
- *
- * Returned Value:
- *
- *     EINVAL - Invalid attempt to get the semaphore
- *     EAGAIN - The semaphore is not available.
- *
- * Assumptions:
- *
- ****************************************************************************/
-
-static int nxsem_trywait_slow(FAR sem_t *sem)
-{
-  FAR struct tcb_s *rtcb;
-  irqstate_t flags;
-  short semcount;
-  int ret;
-
-  /* The following operations must be performed with interrupts disabled
-   * because sem_post() may be called from an interrupt handler.
-   */
-
-  flags = enter_critical_section();
-  rtcb = this_task();
-
-  /* If the semaphore is available, give it to the requesting task */
-
-  do
-    {
-      semcount = atomic_load(NXSEM_COUNT(sem));
-      if (semcount <= 0)
-        {
-          leave_critical_section(flags);
-          return -EAGAIN;
-        }
-    }
-  while (!atomic_compare_exchange_weak_explicit(NXSEM_COUNT(sem),
-                                                &semcount, semcount - 1,
-                                                memory_order_acquire,
-                                                memory_order_relaxed));
-
-  /* It is, let the task take the semaphore */
-
-  ret = nxsem_protect_wait(sem);
-  if (ret < 0)
-    {
-      atomic_fetch_add(NXSEM_COUNT(sem), 1);
-      leave_critical_section(flags);
-      return ret;
-    }
-
-  nxsem_add_holder(sem);
-  rtcb->waitobj = NULL;
-  ret = OK;
-
-  /* Interrupts may now be enabled. */
-
-  leave_critical_section(flags);
-  return ret;
-}
-
 /****************************************************************************
  * Public Functions
  ****************************************************************************/
@@ -140,30 +68,49 @@ static int nxsem_trywait_slow(FAR sem_t *sem)
 
 int nxsem_trywait(FAR sem_t *sem)
 {
+  FAR struct tcb_s *rtcb = this_task();
+  irqstate_t flags;
+  int ret;
+
   /* This API should not be called from the idleloop */
 
   DEBUGASSERT(sem != NULL);
   DEBUGASSERT(!OSINIT_IDLELOOP() || !sched_idletask() ||
               up_interrupt_context());
 
-  /* If this is a mutex, we can try to get the mutex in fast mode,
+  /* The following operations must be performed with interrupts disabled
-   * else try to get it in slow mode.
+   * because sem_post() may be called from an interrupt handler.
    */
 
-#if !defined(CONFIG_PRIORITY_INHERITANCE) && !defined(CONFIG_PRIORITY_PROTECT)
+  flags = enter_critical_section();
-  if (sem->flags & SEM_TYPE_MUTEX)
+
+  /* If the semaphore is available, give it to the requesting task */
+
+  if (sem->semcount > 0)
     {
-      short old = 1;
+      /* It is, let the task take the semaphore */
-      if (atomic_compare_exchange_weak_explicit(NXSEM_COUNT(sem), &old, 0,
+
-                                                memory_order_acquire,
+      ret = nxsem_protect_wait(sem);
-                                                memory_order_relaxed))
+      if (ret < 0)
         {
-          return OK;
+          leave_critical_section(flags);
+          return ret;
         }
 
-      return -EAGAIN;
+      sem->semcount--;
+      nxsem_add_holder(sem);
+      rtcb->waitobj = NULL;
+      ret = OK;
     }
-#endif
+  else
+    {
+      /* Semaphore is not available */
 
-  return nxsem_trywait_slow(sem);
+      ret = -EAGAIN;
+    }
+
+  /* Interrupts may now be enabled. */
+
+  leave_critical_section(flags);
+  return ret;
 }

sched/semaphore/sem_wait.c

@@ -38,15 +38,16 @@
 #include "semaphore/semaphore.h"
 
 /****************************************************************************
- * Private Functions
+ * Public Functions
  ****************************************************************************/
 
 /****************************************************************************
- * Name: nxsem_wait_slow
+ * Name: nxsem_wait
  *
  * Description:
- *   This function attempts to lock the semaphore referenced by 'sem' in
+ *   This function attempts to lock the semaphore referenced by 'sem'.  If
- *   slow mode.
+ *   the semaphore value is (<=) zero, then the calling task will not return
+ *   until it successfully acquires the lock.
  *
  *   This is an internal OS interface.  It is functionally equivalent to
  *   sem_wait except that:
@@ -68,12 +69,17 @@
  *
  ****************************************************************************/
 
-static int nxsem_wait_slow(FAR sem_t *sem)
+int nxsem_wait(FAR sem_t *sem)
 {
   FAR struct tcb_s *rtcb = this_task();
   irqstate_t flags;
   int ret;
 
+  /* This API should not be called from interrupt handlers & idleloop */
+
+  DEBUGASSERT(sem != NULL && up_interrupt_context() == false);
+  DEBUGASSERT(!OSINIT_IDLELOOP() || !sched_idletask());
+
   /* The following operations must be performed with interrupts
    * disabled because nxsem_post() may be called from an interrupt
    * handler.
@@ -85,7 +91,7 @@ static int nxsem_wait_slow(FAR sem_t *sem)
 
   /* Check if the lock is available */
 
-  if (atomic_fetch_sub(NXSEM_COUNT(sem), 1) > 0)
+  if (sem->semcount > 0)
     {
       /* It is, let the task take the semaphore. */
 
@@ -96,6 +102,7 @@ static int nxsem_wait_slow(FAR sem_t *sem)
           return ret;
         }
 
+      sem->semcount--;
       nxsem_add_holder(sem);
       rtcb->waitobj = NULL;
       ret = OK;
@@ -117,6 +124,10 @@ static int nxsem_wait_slow(FAR sem_t *sem)
 
       DEBUGASSERT(rtcb->waitobj == NULL);
 
+      /* Handle the POSIX semaphore (but don't set the owner yet) */
+
+      sem->semcount--;
+
       /* Save the waited on semaphore in the TCB */
 
       rtcb->waitobj = sem;
@@ -213,65 +224,6 @@ static int nxsem_wait_slow(FAR sem_t *sem)
   return ret;
 }
 
-/****************************************************************************
- * Public Functions
- ****************************************************************************/
-
-/****************************************************************************
- * Name: nxsem_wait
- *
- * Description:
- *   This function attempts to lock the semaphore referenced by 'sem'.  If
- *   the semaphore value is (<=) zero, then the calling task will not return
- *   until it successfully acquires the lock.
- *
- *   This is an internal OS interface.  It is functionally equivalent to
- *   sem_wait except that:
- *
- *   - It is not a cancellation point, and
- *   - It does not modify the errno value.
- *
- * Input Parameters:
- *   sem - Semaphore descriptor.
- *
- * Returned Value:
- *   This is an internal OS interface and should not be used by applications.
- *   It follows the NuttX internal error return policy:  Zero (OK) is
- *   returned on success.  A negated errno value is returned on failure.
- *   Possible returned errors:
- *
- *   - EINVAL:  Invalid attempt to get the semaphore
- *   - EINTR:   The wait was interrupted by the receipt of a signal.
- *
- ****************************************************************************/
-
-int nxsem_wait(FAR sem_t *sem)
-{
-  /* This API should not be called from interrupt handlers & idleloop */
-
-  DEBUGASSERT(sem != NULL && up_interrupt_context() == false);
-  DEBUGASSERT(!OSINIT_IDLELOOP() || !sched_idletask());
-
-  /* If this is a mutex, we can try to get the mutex in fast mode,
-   * else try to get it in slow mode.
-   */
-
-#if !defined(CONFIG_PRIORITY_INHERITANCE) && !defined(CONFIG_PRIORITY_PROTECT)
-  if (sem->flags & SEM_TYPE_MUTEX)
-    {
-      short old = 1;
-      if (atomic_compare_exchange_weak_explicit(NXSEM_COUNT(sem), &old, 0,
-                                                memory_order_acquire,
-                                                memory_order_relaxed))
-        {
-          return OK;
-        }
-    }
-#endif
-
-  return nxsem_wait_slow(sem);
-}
-
 /****************************************************************************
  * Name: nxsem_wait_uninterruptible
  *

sched/semaphore/sem_waitirq.c

@@ -87,7 +87,7 @@ void nxsem_wait_irq(FAR struct tcb_s *wtcb, int errcode)
    * and already changed the task's state.
    */
 
-  DEBUGASSERT(sem != NULL && atomic_load(NXSEM_COUNT(sem)) < 0);
+  DEBUGASSERT(sem != NULL && sem->semcount < 0);
 
   /* Restore the correct priority of all threads that hold references
    * to this semaphore.
@@ -101,7 +101,7 @@ void nxsem_wait_irq(FAR struct tcb_s *wtcb, int errcode)
    * place.
    */
 
-  atomic_fetch_add(NXSEM_COUNT(sem), 1);
+  sem->semcount++;
 
   /* Remove task from waiting list */
 

sched/semaphore/semaphore.h

@@ -31,17 +31,10 @@
 #include <nuttx/compiler.h>
 #include <nuttx/semaphore.h>
 #include <nuttx/sched.h>
-#include <nuttx/atomic.h>
 
 #include <stdint.h>
 #include <stdbool.h>
 
-/****************************************************************************
- * Pre-processor Definitions
- ****************************************************************************/
-
-#define NXSEM_COUNT(s) ((FAR atomic_short *)&(s)->semcount)
-
 /****************************************************************************
  * Public Function Prototypes
  ****************************************************************************/