pkarashchenko commented on code in PR #7649:
URL: https://github.com/apache/incubator-nuttx/pull/7649#discussion_r1029773340


##########
libs/libc/machine/arch_atomic.c:
##########
@@ -733,3 +733,26 @@ SYNC_VAL_CMP_SWAP(4, uint32_t)
 SYNC_VAL_CMP_SWAP(8, uint64_t)
 
 #endif /* __clang__ */
+
+/****************************************************************************
+ * Name: up_testset
+ ****************************************************************************/
+
+#if defined(CONFIG_SPINLOCK) && !defined(CONFIG_ARCH_HAVE_TESTSET)
+spinlock_t up_testset(volatile FAR spinlock_t *lock)
+{
+  irqstate_t flags;
+  spinlock_t ret;
+
+  flags = spin_lock_irqsave(NULL);

Review Comment:
   Yeah, those are atomic values, but spinlock itself relies at up_testest, so 
using spinlock inside seems to be odd at least to me.
   I fully understand that spinlock can be implemented on top of atomic 
compare-and-swap, but this is not the case here. Also the macro for a single 
CPU system will end up into disabling of interrupts as it was in the code 
before this change.
   @masayuki2009 what is your opinion here?



-- 
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

Reply via email to