[Core] Use a mutex guard for split shared memory (#16647)

6 files changed, 82 insertions, 20 deletions

diff --git a/platforms/chibios/drivers/serial.c b/platforms/chibios/drivers/serial.c
index bb7b3c0554..0cff057d1d 100644
--- a/platforms/chibios/drivers/serial.c
+++ b/platforms/chibios/drivers/serial.c
@@ -5,6 +5,7 @@
 #include "quantum.h"
 #include "serial.h"
 #include "wait.h"
+#include "synchronization_util.h"
 
 #include <hal.h>
 
@@ -86,7 +87,10 @@ static THD_FUNCTION(Thread1, arg) {
     chRegSetThreadName("blinker");
     while (true) {
         palWaitLineTimeout(SOFT_SERIAL_PIN, TIME_INFINITE);
+
+        split_shared_memory_lock();
         interrupt_handler(NULL);
+        split_shared_memory_unlock();
     }
 }
 
@@ -205,14 +209,9 @@ void interrupt_handler(void *arg) {
     chSysUnlockFromISR();
 }
 
-/////////
-//  start transaction by initiator
-//
-// bool  soft_serial_transaction(int sstd_index)
-//
-// this code is very time dependent, so we need to disable interrupts
-bool soft_serial_transaction(int sstd_index) {
+static inline bool initiate_transaction(uint8_t sstd_index) {
     if (sstd_index > NUM_TOTAL_TRANSACTIONS) return false;
+
     split_transaction_desc_t *trans = &split_transaction_table[sstd_index];
 
     // TODO: remove extra delay between transactions
@@ -239,8 +238,7 @@ bool soft_serial_transaction(int sstd_index) {
         return false;
     }
 
-    // if the slave is present syncronize with it
-
+    // if the slave is present synchronize with it
     uint8_t checksum = 0;
     // send data to the slave
     serial_write_byte(sstd_index); // first chunk is transaction id
@@ -286,3 +284,16 @@ bool soft_serial_transaction(int sstd_index) {
     chSysUnlock();
     return true;
 }
+
+/////////
+//  start transaction by initiator
+//
+// bool  soft_serial_transaction(int sstd_index)
+//
+// this code is very time dependent, so we need to disable interrupts
+bool soft_serial_transaction(int sstd_index) {
+    split_shared_memory_lock();
+    bool result = initiate_transaction((uint8_t)sstd_index);
+    split_shared_memory_unlock();
+    return result;
+}
diff --git a/platforms/chibios/drivers/serial_usart.c b/platforms/chibios/drivers/serial_usart.c
index 85c64214d1..e9fa4af7a3 100644
--- a/platforms/chibios/drivers/serial_usart.c
+++ b/platforms/chibios/drivers/serial_usart.c
@@ -15,6 +15,7 @@
  */
 
 #include "serial_usart.h"
+#include "synchronization_util.h"
 
 #if defined(SERIAL_USART_CONFIG)
 static SerialConfig serial_config = SERIAL_USART_CONFIG;
@@ -173,6 +174,7 @@ static THD_FUNCTION(SlaveThread, arg) {
              * Parts of failed transactions or spurious bytes could still be in it. */
             usart_clear();
         }
+        split_shared_memory_unlock();
     }
 }
 
@@ -200,6 +202,7 @@ static inline bool react_to_transactions(void) {
         return false;
     }
 
+    split_shared_memory_lock();
     split_transaction_desc_t* trans = &split_transaction_table[sstd_index];
 
     /* Send back the handshake which is XORed as a simple checksum,
@@ -254,7 +257,12 @@ bool soft_serial_transaction(int index) {
     /* Clear the receive queue, to start with a clean slate.
      * Parts of failed transactions or spurious bytes could still be in it. */
     usart_clear();
-    return initiate_transaction((uint8_t)index);
+
+    split_shared_memory_lock();
+    bool result = initiate_transaction((uint8_t)index);
+    split_shared_memory_unlock();
+
+    return result;
 }
 
 /**
diff --git a/platforms/chibios/platform.mk b/platforms/chibios/platform.mk
index 91dd0479bc..21751f23fd 100644
--- a/platforms/chibios/platform.mk
+++ b/platforms/chibios/platform.mk
@@ -265,7 +265,8 @@ PLATFORM_SRC = \
         $(STREAMSSRC) \
         $(CHIBIOS)/os/various/syscalls.c \
         $(PLATFORM_COMMON_DIR)/syscall-fallbacks.c \
-        $(PLATFORM_COMMON_DIR)/wait.c
+        $(PLATFORM_COMMON_DIR)/wait.c \
+        $(PLATFORM_COMMON_DIR)/synchronization_util.c
 
 # Ensure the ASM files are not subjected to LTO -- it'll strip out interrupt handlers otherwise.
 QUANTUM_LIB_SRC += $(STARTUPASM) $(PORTASM) $(OSALASM) $(PLATFORMASM)
@@ -420,6 +421,9 @@ LDFLAGS  += $(SHARED_LDFLAGS) $(SHARED_LDSYMBOLS) $(TOOLCHAIN_LDFLAGS) $(TOOLCHA
 # Tell QMK that we are hosting it on ChibiOS.
 OPT_DEFS += -DPROTOCOL_CHIBIOS
 
+# ChibiOS supports synchronization primitives like a Mutex
+OPT_DEFS += -DPLATFORM_SUPPORTS_SYNCHRONIZATION
+
 # Workaround to stop ChibiOS from complaining about new GCC -- it's been fixed for 7/8/9 already
 OPT_DEFS += -DPORT_IGNORE_GCC_VERSION_CHECK=1
 
diff --git a/platforms/chibios/synchronization_util.c b/platforms/chibios/synchronization_util.c
new file mode 100644
index 0000000000..bc4a4e621f
--- /dev/null
+++ b/platforms/chibios/synchronization_util.c
@@ -0,0 +1,26 @@
+// Copyright 2022 Stefan Kerkmann
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#include "synchronization_util.h"
+#include "ch.h"
+
+#if defined(SPLIT_KEYBOARD)
+static MUTEX_DECL(SPLIT_SHARED_MEMORY_MUTEX);
+
+/**
+ * @brief Acquire exclusive access to the split keyboard shared memory, by
+ * locking the mutex guarding it. If the mutex is already held, the calling
+ * thread will be suspended until the mutex currently owning thread releases the
+ * mutex again.
+ */
+void split_shared_memory_lock(void) {
+    chMtxLock(&SPLIT_SHARED_MEMORY_MUTEX);
+}
+
+/**
+ * @brief Release the split shared memory mutex that has been acquired before.
+ */
+void split_shared_memory_unlock(void) {
+    chMtxUnlock(&SPLIT_SHARED_MEMORY_MUTEX);
+}
+#endif
diff --git a/platforms/synchronization_util.h b/platforms/synchronization_util.h
new file mode 100644
index 0000000000..3730f271db
--- /dev/null
+++ b/platforms/synchronization_util.h
@@ -0,0 +1,14 @@
+// Copyright 2022 Stefan Kerkmann
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#pragma once
+
+#if defined(PLATFORM_SUPPORTS_SYNCHRONIZATION)
+#    if defined(SPLIT_KEYBOARD)
+void split_shared_memory_lock(void);
+void split_shared_memory_unlock(void);
+#    endif
+#else
+inline void split_shared_memory_lock(void){};
+inline void split_shared_memory_unlock(void){};
+#endif
diff --git a/quantum/split_common/transactions.c b/quantum/split_common/transactions.c
index 105bf918cb..9e3df534e3 100644
--- a/quantum/split_common/transactions.c
+++ b/quantum/split_common/transactions.c
@@ -23,8 +23,9 @@
 #include "quantum.h"
 #include "transactions.h"
 #include "transport.h"
-#include "split_util.h"
 #include "transaction_id_define.h"
+#include "split_util.h"
+#include "synchronization_util.h"
 
 #define SYNC_TIMER_OFFSET 2
 
@@ -63,9 +64,7 @@ static bool transaction_handler_master(matrix_row_t master_matrix[], matrix_row_
             }
         }
         bool this_okay = true;
-        ATOMIC_BLOCK_FORCEON {
-            this_okay = handler(master_matrix, slave_matrix);
-        };
+        this_okay      = handler(master_matrix, slave_matrix);
         if (this_okay) return true;
     }
     dprintf("Failed to execute %s\n", prefix);
@@ -77,11 +76,11 @@ static bool transaction_handler_master(matrix_row_t master_matrix[], matrix_row_
         if (!transaction_handler_master(master_matrix, slave_matrix, #prefix, &prefix##_handlers_master)) return false; \
     } while (0)
 
-#define TRANSACTION_HANDLER_SLAVE(prefix)                         \
-    do {                                                          \
-        ATOMIC_BLOCK_FORCEON {                                    \
-            prefix##_handlers_slave(master_matrix, slave_matrix); \
-        };                                                        \
+#define TRANSACTION_HANDLER_SLAVE(prefix)                     \
+    do {                                                      \
+        split_shared_memory_lock();                           \
+        prefix##_handlers_slave(master_matrix, slave_matrix); \
+        split_shared_memory_unlock();                         \
     } while (0)
 
 inline static bool read_if_checksum_mismatch(int8_t trans_id_checksum, int8_t trans_id_retrieve, uint32_t *last_update, void *destination, const void *equiv_shmem, size_t length) {