From 56f7b34289e7f80ac540a7c9c01a5c3a3838cd7c Mon Sep 17 00:00:00 2001
From: Stefan Kerkmann <karlk90@pm.me>
Date: Tue, 4 Oct 2022 20:49:29 +0200
Subject: [PATCH] [Core] rewrite locking in split transaction handlers (#18417)
---
quantum/split_common/transactions.c | 121 ++++++++++++++++++++--------
1 file 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,7 +76,26 @@ 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); \
@@ -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)