xiaoxiang781216 commented on code in PR #15075:
URL: https://github.com/apache/nuttx/pull/15075#discussion_r1876420989
##########
sched/semaphore/sem_trywait.c:
##########
@@ -38,6 +38,76 @@
#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;
+ int32_t 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_read(NXSEM_COUNT(sem));
Review Comment:
move before line 79
##########
sched/semaphore/sem_trywait.c:
##########
@@ -68,49 +138,28 @@
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());
- /* The following operations must be performed with interrupts disabled
- * because sem_post() may be called from an interrupt handler.
+ /* If this is a mutex, we can try to get the mutex in fast mode,
+ * else try to get it in slow mode.
*/
- flags = enter_critical_section();
-
- /* If the semaphore is available, give it to the requesting task */
-
- if (sem->semcount > 0)
+#if !defined(CONFIG_PRIORITY_INHERITANCE) && !defined(CONFIG_PRIORITY_PROTECT)
+ if (sem->flags & SEM_TYPE_MUTEX)
{
- /* It is, let the task take the semaphore */
-
- ret = nxsem_protect_wait(sem);
- if (ret < 0)
+ int32_t old = 1;
Review Comment:
All similar place change int32_t to atomic_t
##########
sched/semaphore/sem_destroy.c:
##########
@@ -60,6 +60,8 @@
int nxsem_destroy(FAR sem_t *sem)
{
+ int32_t old;
Review Comment:
atomic_t
##########
sched/semaphore/sem_reset.c:
##########
@@ -98,10 +99,15 @@ int nxsem_reset(FAR sem_t *sem, int16_t count)
* value of sem->semcount is already correct in this case.
*/
- if (sem->semcount >= 0)
+ do
{
- sem->semcount = count;
+ semcount = atomic_read(NXSEM_COUNT(sem));
Review Comment:
move before line 101
##########
sched/semaphore/sem_wait.c:
##########
@@ -224,6 +213,63 @@ int nxsem_wait(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)
+ {
+ int32_t old = 1;
Review Comment:
atomic_t
##########
sched/semaphore/sem_reset.c:
##########
@@ -60,6 +60,7 @@
int nxsem_reset(FAR sem_t *sem, int16_t count)
{
irqstate_t flags;
+ int32_t semcount;
Review Comment:
atomic_t
##########
sched/semaphore/sem_post.c:
##########
@@ -217,3 +214,58 @@ int nxsem_post(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)
+ {
+ int32_t old = 0;
Review Comment:
atomic_t
##########
sched/semaphore/sem_trywait.c:
##########
@@ -38,6 +38,76 @@
#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;
+ int32_t semcount;
Review Comment:
atomic_t
##########
sched/semaphore/sem_wait.c:
##########
@@ -102,7 +96,6 @@ int nxsem_wait(FAR sem_t *sem)
return ret;
}
- sem->semcount--;
Review Comment:
need restore count at line 95
##########
sched/semaphore/sem_post.c:
##########
@@ -78,21 +78,20 @@ int nxsem_post(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.
*/
flags = enter_critical_section();
- sem_count = sem->semcount;
+ sem_count = atomic_fetch_add(NXSEM_COUNT(sem), 1);
/* Check the maximum allowable value */
if (sem_count >= SEM_VALUE_MAX)
Review Comment:
SEM_VALUE_MAX could change to big value
##########
sched/semaphore/sem_destroy.c:
##########
@@ -72,10 +74,15 @@ int nxsem_destroy(FAR sem_t *sem)
* leave the count unchanged but still return OK.
*/
- if (sem->semcount >= 0)
+ do
{
- sem->semcount = 1;
+ old = atomic_read(NXSEM_COUNT(sem));
Review Comment:
move before line 75
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
