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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
