From ea4b76d5303876d6f34e4de0eaa0245b9c5d9109 Mon Sep 17 00:00:00 2001 From: Stefan Kerkmann Date: Mon, 19 Sep 2022 14:14:59 +0200 Subject: [PATCH] Rewrite locking in split transaction handlers The access to the split shared memory shall be as short as possible in order to not block any pending transaction initiated by the main half of the keyboard. Most transaction handlers are deterministic and short in runtime and therefore can simply take the lock for the whole duration of the handler. Other handlers execute code that might end up calling user code or invoke interactions with external peripherals. Those handlers need a specialized implementation that takes a lock for the shortest possible duration. This is done in the following commit. --- 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)