casaroli commented on code in PR #21342:
URL: https://github.com/apache/nuttx/pull/21342#discussion_r4453531257
##########
include/nuttx/spinlock.h:
##########
@@ -641,7 +707,7 @@ static inline_function bool rspin_trylock(FAR rspinlock_t
*lock)
# define rspin_trylock_irqsave(l, f) \
({ \
(void)(l); \
- (f) = up_irq_save(); \
+ (f) = irq_save(); \
Review Comment:
`irq_save()` is still at line 709: only the 4 RW sites changed.
`rspin_trylock_irqsave()` without CONFIG_SPINLOCK still fails with "implicit
declaration of function 'irq_save'" (qemu-armv8a:nsh with SPINLOCK off; master
compiles).
Fix: `(f) = up_irq_save(); \` at line 709.
##########
arch/arm/include/armv7-a/barriers.h:
##########
@@ -47,6 +47,21 @@
#define UP_ISB() arm_isb()
#define UP_NOP() arm_nop()
-#define UP_SEV() arm_sev()
+
+/* SEV is not a memory access and is not subject to memory ordering,
+ * so a waiter may observe the event before the lock release store
+ * becomes visible. The waiter then re-reads the stale lock and sleeps
+ * again with the event already consumed (missed wakeup). DSB makes
+ * the store visible before SEV. From ARM Reference Manual DDI 0406C,
+ * "Use of WFE and SEV with Locks".
+ */
+
+#define UP_SEV() \
Review Comment:
arch/arm/include/arch.h:160 defines `UP_SEV()` again as a bare `sev` for
CONFIG_ARM_HAVE_WFE_SEV (all ARMv7-A, RP2040, RP23XX) after it includes this
file, so this DSB is never used (include/ is -isystem, so GCC does not warn).
In qemu-armv7a:smp built from this PR all 77 `sev` directly follow the unlock
`str`, with no `dsb`.
Fix: put the DSB into the `UP_SEV()` at arch/arm/include/arch.h:160, which
is the definition the build uses.
##########
include/nuttx/spinlock.h:
##########
@@ -314,10 +334,12 @@ static inline_function void
spin_unlock_notrace(FAR volatile spinlock_t *lock)
{
#ifdef CONFIG_TICKET_SPINLOCK
- atomic_add_release(&lock->owner, 1);
+ atomic_set_release(&lock->owner, lock->owner + 1);
#else
atomic_set_release((FAR atomic_t *)lock, SP_UNLOCKED);
#endif
+ UP_DSB();
Review Comment:
On arm64 the SEV now comes without a DSB. These waiters arm no exclusive
monitor (`swpa`, or `ldaxr`+`stxr` at line 155, `ldar` at line 150), so only
the SEV wakes them, and without a DSB the SEV can come before the `stlr` is
visible to the other CPU. In qemu-armv8a:nsh_smp built from this PR all 79
`sev` directly follow `stlr`.
Fix: define the arm64 `UP_SEV()` (arch/arm64/include/arch.h:58) as `dsb` +
`sev`, as for ARMv7-A.
--
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]
