pw_sync_embos: Disable task switching in the ISL
Updates the pw::sync::InterruptSpinLock implementation for embOS to
disable task switching if not in an interrupt. This way there's no
risk that the current task gets switched out to another which
recursively attempts to grab the same lock.
This also changes the internal state to use a plain bool instead
of an atomic bool as an atomic is not necessary.
Change-Id: Ic30bdbee2b298398056d0e5dcdae878ac681f95d
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/50462
Reviewed-by: Wyatt Hepler <hepler@google.com>
Reviewed-by: Keir Mierle <keir@google.com>
Commit-Queue: Auto-Submit <auto-submit@pigweed.google.com.iam.gserviceaccount.com>
Pigweed-Auto-Submit: Ewout van Bekkum <ewout@google.com>
diff --git a/pw_sync_embos/interrupt_spin_lock.cc b/pw_sync_embos/interrupt_spin_lock.cc
index 2bb029c..ee2c9ed 100644
--- a/pw_sync_embos/interrupt_spin_lock.cc
+++ b/pw_sync_embos/interrupt_spin_lock.cc
@@ -20,26 +20,42 @@
namespace pw::sync {
void InterruptSpinLock::lock() {
+ // Mask interrupts.
OS_IncDI();
+
+ // Disable task switching to ensure kernel APIs cannot switch to other tasks
+ // which could then end up deadlocking recursively on this same lock.
+ OS_SuspendAllTasks();
+
// We can't deadlock here so crash instead.
- PW_CHECK(!native_type_.locked.load(std::memory_order_relaxed),
+ PW_CHECK(!native_type_.locked,
"Recursive InterruptSpinLock::lock() detected");
- native_type_.locked.store(true, std::memory_order_relaxed);
+ native_type_.locked = true;
}
bool InterruptSpinLock::try_lock() {
+ // Mask interrupts.
OS_IncDI();
- if (native_type_.locked.load(std::memory_order_relaxed)) {
- OS_DecRI(); // Already locked, restore interrupts and bail out.
+
+ // Disable task switching to ensure kernel APIs cannot switch to other tasks
+ // which could then end up deadlocking recursively on this same lock.
+ OS_SuspendAllTasks();
+
+ if (native_type_.locked) {
+ // Already locked, bail out.
+ OS_ResumeAllSuspendedTasks(); // Restore task switching.
+ OS_DecRI(); // Restore interrupts.
return false;
}
- native_type_.locked.store(true, std::memory_order_relaxed);
+
+ native_type_.locked = true;
return true;
}
void InterruptSpinLock::unlock() {
- native_type_.locked.store(false, std::memory_order_relaxed);
- OS_DecRI();
+ native_type_.locked = false;
+ OS_ResumeAllSuspendedTasks(); // Restore task switching.
+ OS_DecRI(); // Restore interrupts.
}
} // namespace pw::sync
diff --git a/pw_sync_embos/public/pw_sync_embos/interrupt_spin_lock_inline.h b/pw_sync_embos/public/pw_sync_embos/interrupt_spin_lock_inline.h
index 0484b3c..03790cc 100644
--- a/pw_sync_embos/public/pw_sync_embos/interrupt_spin_lock_inline.h
+++ b/pw_sync_embos/public/pw_sync_embos/interrupt_spin_lock_inline.h
@@ -18,7 +18,7 @@
namespace pw::sync {
constexpr InterruptSpinLock::InterruptSpinLock()
- : native_type_{.locked{false}} {}
+ : native_type_{.locked = false} {}
inline InterruptSpinLock::native_handle_type
InterruptSpinLock::native_handle() {
diff --git a/pw_sync_embos/public/pw_sync_embos/interrupt_spin_lock_native.h b/pw_sync_embos/public/pw_sync_embos/interrupt_spin_lock_native.h
index 3c91f64..548b67a 100644
--- a/pw_sync_embos/public/pw_sync_embos/interrupt_spin_lock_native.h
+++ b/pw_sync_embos/public/pw_sync_embos/interrupt_spin_lock_native.h
@@ -13,14 +13,12 @@
// the License.
#pragma once
-#include <atomic>
-
#include "RTOS.h"
namespace pw::sync::backend {
struct NativeInterruptSpinLock {
- std::atomic<bool> locked; // Used to detect recursion.
+ bool locked; // Used to detect recursion.
};
using NativeInterruptSpinLockHandle = NativeInterruptSpinLock&;
