[Core] rewrite locking in split transaction handlers (#18417)

1 files changed, 88 insertions, 33 deletions

diff --git a/quantum/split_common/transactions.c b/quantum/split_common/transactions.c
index 719068908f..13fd8a761e 100644
--- a/quantum/split_common/transactions.c
+++ b/quantum/split_common/transactions.c
@@ -76,8 +76,27 @@ 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)
 
+/**
+ * @brief Constructs a transaction handler that doesn't acquire a lock to the
+ * split shared memory. Therefore the locking and unlocking has to be done
+ * manually inside the handler. Use this macro only if the handler is
+ * non-deterministic in runtime and thus needs a manual lock unlock
+ * implementation to hold the lock for the shortest possible time.
+ */
 #define TRANSACTION_HANDLER_SLAVE(prefix)                     \
     do {                                                      \
+        prefix##_handlers_slave(master_matrix, slave_matrix); \
+    } while (0)
+
+/**
+ * @brief Constructs a transaction handler that automatically acquires a lock to
+ * safely access the split shared memory and releases the lock again after
+ * processing the handler. Use this macro if the handler is fast and
+ * deterministic in runtime and thus holds the lock only for a very short time.
+ * If not fallback to manually locking and unlocking inside the handler.
+ */
+#define TRANSACTION_HANDLER_SLAVE_AUTOLOCK(prefix)            \
+    do {                                                      \
         split_shared_memory_lock();                           \
         prefix##_handlers_slave(master_matrix, slave_matrix); \
         split_shared_memory_unlock();                         \
@@ -139,7 +158,7 @@ static void slave_matrix_handlers_slave(matrix_row_t master_matrix[], matrix_row
 
 // clang-format off
 #define TRANSACTIONS_SLAVE_MATRIX_MASTER() TRANSACTION_HANDLER_MASTER(slave_matrix)
-#define TRANSACTIONS_SLAVE_MATRIX_SLAVE() TRANSACTION_HANDLER_SLAVE(slave_matrix)
+#define TRANSACTIONS_SLAVE_MATRIX_SLAVE() TRANSACTION_HANDLER_SLAVE_AUTOLOCK(slave_matrix)
 #define TRANSACTIONS_SLAVE_MATRIX_REGISTRATIONS \
     [GET_SLAVE_MATRIX_CHECKSUM] = trans_target2initiator_initializer(smatrix.checksum), \
     [GET_SLAVE_MATRIX_DATA]     = trans_target2initiator_initializer(smatrix.matrix),
@@ -161,7 +180,7 @@ static void master_matrix_handlers_slave(matrix_row_t master_matrix[], matrix_ro
 }
 
 #    define TRANSACTIONS_MASTER_MATRIX_MASTER() TRANSACTION_HANDLER_MASTER(master_matrix)
-#    define TRANSACTIONS_MASTER_MATRIX_SLAVE() TRANSACTION_HANDLER_SLAVE(master_matrix)
+#    define TRANSACTIONS_MASTER_MATRIX_SLAVE() TRANSACTION_HANDLER_SLAVE_AUTOLOCK(master_matrix)
 #    define TRANSACTIONS_MASTER_MATRIX_REGISTRATIONS [PUT_MASTER_MATRIX] = trans_initiator2target_initializer(mmatrix.matrix),
 
 #else // SPLIT_TRANSPORT_MIRROR
@@ -197,7 +216,7 @@ static void encoder_handlers_slave(matrix_row_t master_matrix[], matrix_row_t sl
 
 // clang-format off
 #    define TRANSACTIONS_ENCODERS_MASTER() TRANSACTION_HANDLER_MASTER(encoder)
-#    define TRANSACTIONS_ENCODERS_SLAVE() TRANSACTION_HANDLER_SLAVE(encoder)
+#    define TRANSACTIONS_ENCODERS_SLAVE() TRANSACTION_HANDLER_SLAVE_AUTOLOCK(encoder)
 #    define TRANSACTIONS_ENCODERS_REGISTRATIONS \
     [GET_ENCODERS_CHECKSUM] = trans_target2initiator_initializer(encoders.checksum), \
     [GET_ENCODERS_DATA]     = trans_target2initiator_initializer(encoders.state),
@@ -239,7 +258,7 @@ static void sync_timer_handlers_slave(matrix_row_t master_matrix[], matrix_row_t
 }
 
 #    define TRANSACTIONS_SYNC_TIMER_MASTER() TRANSACTION_HANDLER_MASTER(sync_timer)
-#    define TRANSACTIONS_SYNC_TIMER_SLAVE() TRANSACTION_HANDLER_SLAVE(sync_timer)
+#    define TRANSACTIONS_SYNC_TIMER_SLAVE() TRANSACTION_HANDLER_SLAVE_AUTOLOCK(sync_timer)
 #    define TRANSACTIONS_SYNC_TIMER_REGISTRATIONS [PUT_SYNC_TIMER] = trans_initiator2target_initializer(sync_timer),
 
 #else // DISABLE_SYNC_TIMER
@@ -273,7 +292,7 @@ static void layer_state_handlers_slave(matrix_row_t master_matrix[], matrix_row_
 
 // clang-format off
 #    define TRANSACTIONS_LAYER_STATE_MASTER() TRANSACTION_HANDLER_MASTER(layer_state)
-#    define TRANSACTIONS_LAYER_STATE_SLAVE() TRANSACTION_HANDLER_SLAVE(layer_state)
+#    define TRANSACTIONS_LAYER_STATE_SLAVE() TRANSACTION_HANDLER_SLAVE_AUTOLOCK(layer_state)
 #    define TRANSACTIONS_LAYER_STATE_REGISTRATIONS \
     [PUT_LAYER_STATE]         = trans_initiator2target_initializer(layers.layer_state), \
     [PUT_DEFAULT_LAYER_STATE] = trans_initiator2target_initializer(layers.default_layer_state),
@@ -304,7 +323,7 @@ static void led_state_handlers_slave(matrix_row_t master_matrix[], matrix_row_t
 }
 
 #    define TRANSACTIONS_LED_STATE_MASTER() TRANSACTION_HANDLER_MASTER(led_state)
-#    define TRANSACTIONS_LED_STATE_SLAVE() TRANSACTION_HANDLER_SLAVE(led_state)
+#    define TRANSACTIONS_LED_STATE_SLAVE() TRANSACTION_HANDLER_SLAVE_AUTOLOCK(led_state)
 #    define TRANSACTIONS_LED_STATE_REGISTRATIONS [PUT_LED_STATE] = trans_initiator2target_initializer(led_state),
 
 #else // SPLIT_LED_STATE_ENABLE
@@ -353,10 +372,15 @@ static bool mods_handlers_master(matrix_row_t master_matrix[], matrix_row_t slav
 }
 
 static void mods_handlers_slave(matrix_row_t master_matrix[], matrix_row_t slave_matrix[]) {
-    set_mods(split_shmem->mods.real_mods);
-    set_weak_mods(split_shmem->mods.weak_mods);
+    split_shared_memory_lock();
+    split_mods_sync_t mods;
+    memcpy(&mods, &split_shmem->mods, sizeof(split_mods_sync_t));
+    split_shared_memory_unlock();
+
+    set_mods(mods.real_mods);
+    set_weak_mods(mods.weak_mods);
 #    ifndef NO_ACTION_ONESHOT
-    set_oneshot_mods(split_shmem->mods.oneshot_mods);
+    set_oneshot_mods(mods.oneshot_mods);
 #    endif
 }
 
@@ -384,7 +408,11 @@ static bool backlight_handlers_master(matrix_row_t master_matrix[], matrix_row_t
 }
 
 static void backlight_handlers_slave(matrix_row_t master_matrix[], matrix_row_t slave_matrix[]) {
-    backlight_set(split_shmem->backlight_level);
+    split_shared_memory_lock();
+    uint8_t backlight_level = split_shmem->backlight_level;
+    split_shared_memory_unlock();
+
+    backlight_set(backlight_level);
 }
 
 #    define TRANSACTIONS_BACKLIGHT_MASTER() TRANSACTION_HANDLER_MASTER(backlight)
@@ -417,10 +445,15 @@ static bool rgblight_handlers_master(matrix_row_t master_matrix[], matrix_row_t
 }
 
 static void rgblight_handlers_slave(matrix_row_t master_matrix[], matrix_row_t slave_matrix[]) {
+    split_shared_memory_lock();
     // Update the RGB with the new data
-    if (split_shmem->rgblight_sync.status.change_flags != 0) {
-        rgblight_update_sync(&split_shmem->rgblight_sync, false);
-        split_shmem->rgblight_sync.status.change_flags = 0;
+    rgblight_syncinfo_t rgblight_sync;
+    memcpy(&rgblight_sync, &split_shmem->rgblight_sync, sizeof(rgblight_syncinfo_t));
+    split_shmem->rgblight_sync.status.change_flags = 0;
+    split_shared_memory_unlock();
+
+    if (rgblight_sync.status.change_flags != 0) {
+        rgblight_update_sync(&rgblight_sync, false);
     }
 }
 
@@ -450,8 +483,12 @@ static bool led_matrix_handlers_master(matrix_row_t master_matrix[], matrix_row_
 }
 
 static void led_matrix_handlers_slave(matrix_row_t master_matrix[], matrix_row_t slave_matrix[]) {
+    split_shared_memory_lock();
     memcpy(&led_matrix_eeconfig, &split_shmem->led_matrix_sync.led_matrix, sizeof(led_eeconfig_t));
-    led_matrix_set_suspend_state(split_shmem->led_matrix_sync.led_suspend_state);
+    bool led_suspend_state = split_shmem->led_matrix_sync.led_suspend_state;
+    split_shared_memory_unlock();
+
+    led_matrix_set_suspend_state(led_suspend_state);
 }
 
 #    define TRANSACTIONS_LED_MATRIX_MASTER() TRANSACTION_HANDLER_MASTER(led_matrix)
@@ -480,8 +517,12 @@ static bool rgb_matrix_handlers_master(matrix_row_t master_matrix[], matrix_row_
 }
 
 static void rgb_matrix_handlers_slave(matrix_row_t master_matrix[], matrix_row_t slave_matrix[]) {
+    split_shared_memory_lock();
     memcpy(&rgb_matrix_config, &split_shmem->rgb_matrix_sync.rgb_matrix, sizeof(rgb_config_t));
-    rgb_matrix_set_suspend_state(split_shmem->rgb_matrix_sync.rgb_suspend_state);
+    bool rgb_suspend_state = split_shmem->rgb_matrix_sync.rgb_suspend_state;
+    split_shared_memory_unlock();
+
+    rgb_matrix_set_suspend_state(rgb_suspend_state);
 }
 
 #    define TRANSACTIONS_RGB_MATRIX_MASTER() TRANSACTION_HANDLER_MASTER(rgb_matrix)
@@ -512,7 +553,7 @@ static void wpm_handlers_slave(matrix_row_t master_matrix[], matrix_row_t slave_
 }
 
 #    define TRANSACTIONS_WPM_MASTER() TRANSACTION_HANDLER_MASTER(wpm)
-#    define TRANSACTIONS_WPM_SLAVE() TRANSACTION_HANDLER_SLAVE(wpm)
+#    define TRANSACTIONS_WPM_SLAVE() TRANSACTION_HANDLER_SLAVE_AUTOLOCK(wpm)
 #    define TRANSACTIONS_WPM_REGISTRATIONS [PUT_WPM] = trans_initiator2target_initializer(current_wpm),
 
 #else // defined(WPM_ENABLE) && defined(SPLIT_WPM_ENABLE)
@@ -535,7 +576,11 @@ static bool oled_handlers_master(matrix_row_t master_matrix[], matrix_row_t slav
 }
 
 static void oled_handlers_slave(matrix_row_t master_matrix[], matrix_row_t slave_matrix[]) {
-    if (split_shmem->current_oled_state) {
+    split_shared_memory_lock();
+    uint8_t current_oled_state = split_shmem->current_oled_state;
+    split_shared_memory_unlock();
+
+    if (current_oled_state) {
         oled_on();
     } else {
         oled_off();
@@ -566,7 +611,11 @@ static bool st7565_handlers_master(matrix_row_t master_matrix[], matrix_row_t sl
 }
 
 static void st7565_handlers_slave(matrix_row_t master_matrix[], matrix_row_t slave_matrix[]) {
-    if (split_shmem->current_st7565_state) {
+    split_shared_memory_lock();
+    uint8_t current_st7565_state = split_shmem->current_st7565_state;
+    split_shared_memory_unlock();
+
+    if (current_st7565_state) {
         st7565_on();
     } else {
         st7565_off();
@@ -574,7 +623,7 @@ static void st7565_handlers_slave(matrix_row_t master_matrix[], matrix_row_t sla
 }
 
 #    define TRANSACTIONS_ST7565_MASTER() TRANSACTION_HANDLER_MASTER(st7565)
-#    define TRANSACTIONS_ST7565_SLAVE() TRANSACTION_HANDLER_SLAVE(st7565)
+#    define TRANSACTIONS_ST7565_SLAVE() TRANSACTION_HANDLER_SLAVE_AUTOLOCK(st7565)
 #    define TRANSACTIONS_ST7565_REGISTRATIONS [PUT_ST7565] = trans_initiator2target_initializer(current_st7565_state),
 
 #else // defined(ST7565_ENABLE) && defined(SPLIT_ST7565_ENABLE)
@@ -607,9 +656,9 @@ static bool pointing_handlers_master(matrix_row_t master_matrix[], matrix_row_t
     bool            okay = read_if_checksum_mismatch(GET_POINTING_CHECKSUM, GET_POINTING_DATA, &last_update, &temp_state, &split_shmem->pointing.report, sizeof(temp_state));
     if (okay) pointing_device_set_shared_report(temp_state);
     temp_cpi = pointing_device_get_shared_cpi();
-    if (temp_cpi && memcmp(&last_cpi, &temp_cpi, sizeof(temp_cpi)) != 0) {
-        memcpy(&split_shmem->pointing.cpi, &temp_cpi, sizeof(temp_cpi));
-        okay = transport_write(PUT_POINTING_CPI, &split_shmem->pointing.cpi, sizeof(split_shmem->pointing.cpi));
+    if (temp_cpi && last_cpi != temp_cpi) {
+        split_shmem->pointing.cpi = temp_cpi;
+        okay                      = transport_write(PUT_POINTING_CPI, &split_shmem->pointing.cpi, sizeof(split_shmem->pointing.cpi));
         if (okay) {
             last_cpi = temp_cpi;
         }
@@ -629,8 +678,6 @@ static void pointing_handlers_slave(matrix_row_t master_matrix[], matrix_row_t s
         return;
     }
 #    endif
-    report_mouse_t temp_report;
-    uint16_t       temp_cpi;
 #    if (POINTING_DEVICE_TASK_THROTTLE_MS > 0)
     static uint32_t last_exec = 0;
     if (timer_elapsed32(last_exec) < POINTING_DEVICE_TASK_THROTTLE_MS) {
@@ -638,17 +685,25 @@ static void pointing_handlers_slave(matrix_row_t master_matrix[], matrix_row_t s
     }
     last_exec = timer_read32();
 #    endif
-    temp_cpi = !pointing_device_driver.get_cpi ? 0 : pointing_device_driver.get_cpi(); // check for NULL
-    if (split_shmem->pointing.cpi && memcmp(&split_shmem->pointing.cpi, &temp_cpi, sizeof(temp_cpi)) != 0) {
-        if (pointing_device_driver.set_cpi) {
-            pointing_device_driver.set_cpi(split_shmem->pointing.cpi);
-        }
+
+    uint16_t temp_cpi = !pointing_device_driver.get_cpi ? 0 : pointing_device_driver.get_cpi(); // check for NULL
+
+    split_shared_memory_lock();
+    split_slave_pointing_sync_t pointing;
+    memcpy(&pointing, &split_shmem->pointing, sizeof(split_slave_pointing_sync_t));
+    split_shared_memory_unlock();
+
+    if (pointing.cpi && pointing.cpi != temp_cpi && pointing_device_driver.set_cpi) {
+        pointing_device_driver.set_cpi(pointing.cpi);
     }
-    memset(&temp_report, 0, sizeof(temp_report));
-    temp_report = pointing_device_driver.get_report(temp_report);
-    memcpy(&split_shmem->pointing.report, &temp_report, sizeof(temp_report));
+
+    pointing.report = pointing_device_driver.get_report((report_mouse_t){0});
     // Now update the checksum given that the pointing has been written to
-    split_shmem->pointing.checksum = crc8(&temp_report, sizeof(temp_report));
+    pointing.checksum = crc8(&pointing.report, sizeof(report_mouse_t));
+
+    split_shared_memory_lock();
+    memcpy(&split_shmem->pointing, &pointing, sizeof(split_slave_pointing_sync_t));
+    split_shared_memory_unlock();
 }
 
 #    define TRANSACTIONS_POINTING_MASTER() TRANSACTION_HANDLER_MASTER(pointing)