[BUG] Fix deadlocks on disconnected secondary half (#17423)

qmk · Jun 20, 2022 · 2703ecc · 2703ecc
1 parent 62eaec5
commit 2703ecc
Show file tree

Hide file tree

Showing 3 changed files with 38 additions and 11 deletions.
diff --git a/platforms/chibios/drivers/serial.c b/platforms/chibios/drivers/serial.c
@@ -87,10 +87,7 @@ 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();
     }
 }
 
@@ -155,6 +152,7 @@ static void __attribute__((noinline)) serial_write_byte(uint8_t data) {
 
 // interrupt handle to be used by the slave device
 void interrupt_handler(void *arg) {
+    split_shared_memory_lock_autounlock();
     chSysLockFromISR();
 
     sync_send();
@@ -212,6 +210,8 @@ void interrupt_handler(void *arg) {
 static inline bool initiate_transaction(uint8_t sstd_index) {
     if (sstd_index > NUM_TOTAL_TRANSACTIONS) return false;
 
+    split_shared_memory_lock_autounlock();
+
     split_transaction_desc_t *trans = &split_transaction_table[sstd_index];
 
     // TODO: remove extra delay between transactions
@@ -292,8 +292,5 @@ static inline bool initiate_transaction(uint8_t 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;
+    return initiate_transaction((uint8_t)sstd_index);
 }
diff --git a/platforms/chibios/drivers/serial_protocol.c b/platforms/chibios/drivers/serial_protocol.c
@@ -22,13 +22,11 @@ static THD_FUNCTION(SlaveThread, arg) {
     chRegSetThreadName("split_protocol_tx_rx");
 
     while (true) {
-        split_shared_memory_lock();
         if (unlikely(!react_to_transaction())) {
             /* Clear the receive queue, to start with a clean slate.
              * Parts of failed transactions or spurious bytes could still be in it. */
             serial_transport_driver_clear();
         }
-        split_shared_memory_unlock();
     }
 }
 
@@ -64,6 +62,8 @@ static inline bool react_to_transaction(void) {
         return false;
     }
 
+    split_shared_memory_lock_autounlock();
+
     split_transaction_desc_t* transaction = &split_transaction_table[transaction_id];
 
     /* Send back the handshake which is XORed as a simple checksum,
@@ -102,9 +102,7 @@ static inline bool react_to_transaction(void) {
  * @return bool Indicates success of transaction.
  */
 bool soft_serial_transaction(int index) {
-    split_shared_memory_lock();
     bool result = initiate_transaction((uint8_t)index);
-    split_shared_memory_unlock();
 
     if (unlikely(!result)) {
         /* Clear the receive queue, to start with a clean slate.
@@ -125,6 +123,8 @@ static inline bool initiate_transaction(uint8_t transaction_id) {
         return false;
     }
 
+    split_shared_memory_lock_autounlock();
+
     split_transaction_desc_t* transaction = &split_transaction_table[transaction_id];
 
     /* Send transaction table index to the slave, which doubles as basic handshake token. */

diff --git a/platforms/synchronization_util.h b/platforms/synchronization_util.h
@@ -12,3 +12,33 @@ void split_shared_memory_unlock(void);
 inline void split_shared_memory_lock(void){};
 inline void split_shared_memory_unlock(void){};
 #endif
+
+/* GCCs cleanup attribute expects a function with one parameter, which is a
+ * pointer to a type compatible with the variable. As we don't want to expose
+ * the platforms internal mutex type this workaround with auto generated adapter
+ * function is defined */
+#define QMK_DECLARE_AUTOUNLOCK_HELPERS(prefix)                              \
+    inline unsigned prefix##_autounlock_lock_helper(void) {                 \
+        prefix##_lock();                                                    \
+        return 0;                                                           \
+    }                                                                       \
+                                                                            \
+    inline void prefix##_autounlock_unlock_helper(unsigned* unused_guard) { \
+        prefix##_unlock();                                                  \
+    }
+
+/* Convinience macro the automatically generate the correct RAII-style
+ * lock_autounlock function macro */
+#define QMK_DECLARE_AUTOUNLOCK_CALL(prefix) unsigned prefix##_guard __attribute__((unused, cleanup(prefix##_autounlock_unlock_helper))) = prefix##_autounlock_lock_helper
+
+QMK_DECLARE_AUTOUNLOCK_HELPERS(split_shared_memory)
+
+/**
+ * @brief Acquire exclusive access to the split keyboard shared memory, by
+ * calling the platforms `split_shared_memory_lock()` function. The lock is
+ * automatically released by calling the platforms `split_shared_memory_unlock()`
+ * function. This happens when the block where
+ * `split_shared_memory_lock_autounlock()` is called in goes out of scope i.e.
+ * when the enclosing function returns.
+ */
+#define split_shared_memory_lock_autounlock QMK_DECLARE_AUTOUNLOCK_CALL(split_shared_memory)