From 56f7b34289e7f80ac540a7c9c01a5c3a3838cd7c Mon Sep 17 00:00:00 2001 From: Stefan Kerkmann 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 719068908f77..13fd8a761ee0 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)