casaroli commented on code in PR #21342:
URL: https://github.com/apache/nuttx/pull/21342#discussion_r4448848539
##########
include/nuttx/spinlock.h:
##########
@@ -849,16 +915,22 @@ void spin_unlock_irqrestore_nopreempt(FAR volatile
spinlock_t *lock,
static inline_function
bool rspin_unlock(FAR rspinlock_t *lock)
{
- DEBUGASSERT(lock->owner == this_cpu() + 1);
- DEBUGASSERT(lock->count >= 1);
+ bool ret = false;
+ uint32_t val = lock->local_val;
Review Comment:
This commit removes the two DEBUGASSERT()s of `rspin_unlock()` (owner is
this CPU, count >= 1). Without them an unlock of a lock that is not held sets
`local_val` to 0xffffffff: the lock then looks owned by CPU 65535 and every
later `rspin_lock()` spins for ever, with no assert in a debug build.
Fix: keep `DEBUGASSERT(RSPINLOCK_OWNER(val) == (uint32_t)this_cpu() + 1u);`
and `DEBUGASSERT(RSPINLOCK_COUNT(val) >= 1u);` before the `if`.
##########
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:
`UP_SEV()` is empty on RISC-V, x86_64, Xtensa and sim, but `UP_DSB()` is
`fence iorw,iorw` on RISC-V and `mfence` on x86_64. So every unlock on these
arches gets a full barrier that only the ARM WFE waiters need.
Fix: do `UP_DSB(); UP_SEV();` only where `UP_WFE()` is real (arm64 and
CONFIG_ARM_HAVE_WFE_SEV), for example in one helper macro used at lines 341,
924, 1139 and 1249.
##########
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()` and `irq_restore()` do not exist: the commit that added them is
no longer in this PR, and `git grep -w irq_save` finds only these lines. So
`rspin_trylock_irqsave()` without CONFIG_SPINLOCK now fails with "implicit
declaration of function 'irq_save'" (master compiles it), and lines 1282, 1320,
1355 and 1394 fail in the same way with CONFIG_RW_SPINLOCK.
Fix: use `up_irq_save()` and `up_irq_restore()` at these five lines, as
master does.
--
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]
