From c6ee6bf89236d31e6bde9827799c1c59df9139e4 Mon Sep 17 00:00:00 2001 From: Stefan Kerkmann Date: Mon, 14 Mar 2022 19:38:50 +0100 Subject: [PATCH] Use a mutex for split shared memory synchronization ...on platforms that support it, which is only ChibiOS at this point. The problem: The shared memory on the ChibiOS platform can be accessed concurrently on the secondary device. This happens when the serial transaction thread, reacts to transactions requests by the main side of the keyboard. This can lead to race conditions and corrupted transactions. The solution: The access to the split shared memory is made exclusive at any given time by guarding it with a mutex. The previous solution using a critical section didn't really work out because serial functions like send and receive would disable and enable interrupts on their own, thus leaving the system with enabled interrupts after the first transaction. The same was true for slave transaction handlers for e.g. sync timers, those functions effectively broke the critical section as well. --- platforms/chibios/drivers/serial.c | 29 ++++++++++++++++-------- platforms/chibios/drivers/serial_usart.c | 10 +++++++- platforms/chibios/platform.mk | 6 ++++- platforms/chibios/synchronization_util.c | 26 +++++++++++++++++++++ platforms/synchronization_util.h | 14 ++++++++++++ quantum/split_common/transactions.c | 17 +++++++------- 6 files changed, 82 insertions(+), 20 deletions(-) create mode 100644 platforms/chibios/synchronization_util.c create mode 100644 platforms/synchronization_util.h diff --git a/platforms/chibios/drivers/serial.c b/platforms/chibios/drivers/serial.c index 6db5d8525067..f1a338882479 100644 --- a/platforms/chibios/drivers/serial.c +++ b/platforms/chibios/drivers/serial.c @@ -5,6 +5,7 @@ #include "quantum.h" #include "serial.h" #include "wait.h" +#include "synchronization_util.h" #include @@ -86,7 +87,10 @@ 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(); } } @@ -204,14 +208,9 @@ void interrupt_handler(void *arg) { chSysUnlockFromISR(); } -///////// -// start transaction by initiator -// -// bool soft_serial_transaction(int sstd_index) -// -// this code is very time dependent, so we need to disable interrupts -bool soft_serial_transaction(int sstd_index) { +static inline bool initiate_transaction(uint8_t sstd_index) { if (sstd_index > NUM_TOTAL_TRANSACTIONS) return false; + split_transaction_desc_t *trans = &split_transaction_table[sstd_index]; // TODO: remove extra delay between transactions @@ -238,8 +237,7 @@ bool soft_serial_transaction(int sstd_index) { return false; } - // if the slave is present syncronize with it - + // if the slave is present synchronize with it uint8_t checksum = 0; // send data to the slave serial_write_byte(sstd_index); // first chunk is transaction id @@ -285,3 +283,16 @@ bool soft_serial_transaction(int sstd_index) { chSysUnlock(); return true; } + +///////// +// start transaction by initiator +// +// bool soft_serial_transaction(int 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; +} diff --git a/platforms/chibios/drivers/serial_usart.c b/platforms/chibios/drivers/serial_usart.c index 85c64214d161..e9fa4af7a3d4 100644 --- a/platforms/chibios/drivers/serial_usart.c +++ b/platforms/chibios/drivers/serial_usart.c @@ -15,6 +15,7 @@ */ #include "serial_usart.h" +#include "synchronization_util.h" #if defined(SERIAL_USART_CONFIG) static SerialConfig serial_config = SERIAL_USART_CONFIG; @@ -173,6 +174,7 @@ static THD_FUNCTION(SlaveThread, arg) { * Parts of failed transactions or spurious bytes could still be in it. */ usart_clear(); } + split_shared_memory_unlock(); } } @@ -200,6 +202,7 @@ static inline bool react_to_transactions(void) { return false; } + split_shared_memory_lock(); split_transaction_desc_t* trans = &split_transaction_table[sstd_index]; /* Send back the handshake which is XORed as a simple checksum, @@ -254,7 +257,12 @@ bool soft_serial_transaction(int index) { /* Clear the receive queue, to start with a clean slate. * Parts of failed transactions or spurious bytes could still be in it. */ usart_clear(); - return initiate_transaction((uint8_t)index); + + split_shared_memory_lock(); + bool result = initiate_transaction((uint8_t)index); + split_shared_memory_unlock(); + + return result; } /** diff --git a/platforms/chibios/platform.mk b/platforms/chibios/platform.mk index 91dd0479bc0b..21751f23fd15 100644 --- a/platforms/chibios/platform.mk +++ b/platforms/chibios/platform.mk @@ -265,7 +265,8 @@ PLATFORM_SRC = \ $(STREAMSSRC) \ $(CHIBIOS)/os/various/syscalls.c \ $(PLATFORM_COMMON_DIR)/syscall-fallbacks.c \ - $(PLATFORM_COMMON_DIR)/wait.c + $(PLATFORM_COMMON_DIR)/wait.c \ + $(PLATFORM_COMMON_DIR)/synchronization_util.c # Ensure the ASM files are not subjected to LTO -- it'll strip out interrupt handlers otherwise. QUANTUM_LIB_SRC += $(STARTUPASM) $(PORTASM) $(OSALASM) $(PLATFORMASM) @@ -420,6 +421,9 @@ LDFLAGS += $(SHARED_LDFLAGS) $(SHARED_LDSYMBOLS) $(TOOLCHAIN_LDFLAGS) $(TOOLCHA # Tell QMK that we are hosting it on ChibiOS. OPT_DEFS += -DPROTOCOL_CHIBIOS +# ChibiOS supports synchronization primitives like a Mutex +OPT_DEFS += -DPLATFORM_SUPPORTS_SYNCHRONIZATION + # Workaround to stop ChibiOS from complaining about new GCC -- it's been fixed for 7/8/9 already OPT_DEFS += -DPORT_IGNORE_GCC_VERSION_CHECK=1 diff --git a/platforms/chibios/synchronization_util.c b/platforms/chibios/synchronization_util.c new file mode 100644 index 000000000000..bc4a4e621fb8 --- /dev/null +++ b/platforms/chibios/synchronization_util.c @@ -0,0 +1,26 @@ +// Copyright 2022 Stefan Kerkmann +// SPDX-License-Identifier: GPL-2.0-or-later + +#include "synchronization_util.h" +#include "ch.h" + +#if defined(SPLIT_KEYBOARD) +static MUTEX_DECL(SPLIT_SHARED_MEMORY_MUTEX); + +/** + * @brief Acquire exclusive access to the split keyboard shared memory, by + * locking the mutex guarding it. If the mutex is already held, the calling + * thread will be suspended until the mutex currently owning thread releases the + * mutex again. + */ +void split_shared_memory_lock(void) { + chMtxLock(&SPLIT_SHARED_MEMORY_MUTEX); +} + +/** + * @brief Release the split shared memory mutex that has been acquired before. + */ +void split_shared_memory_unlock(void) { + chMtxUnlock(&SPLIT_SHARED_MEMORY_MUTEX); +} +#endif diff --git a/platforms/synchronization_util.h b/platforms/synchronization_util.h new file mode 100644 index 000000000000..3730f271db97 --- /dev/null +++ b/platforms/synchronization_util.h @@ -0,0 +1,14 @@ +// Copyright 2022 Stefan Kerkmann +// SPDX-License-Identifier: GPL-2.0-or-later + +#pragma once + +#if defined(PLATFORM_SUPPORTS_SYNCHRONIZATION) +# if defined(SPLIT_KEYBOARD) +void split_shared_memory_lock(void); +void split_shared_memory_unlock(void); +# endif +#else +inline void split_shared_memory_lock(void){}; +inline void split_shared_memory_unlock(void){}; +#endif diff --git a/quantum/split_common/transactions.c b/quantum/split_common/transactions.c index 105bf918cb5e..9e3df534e3bf 100644 --- a/quantum/split_common/transactions.c +++ b/quantum/split_common/transactions.c @@ -23,8 +23,9 @@ #include "quantum.h" #include "transactions.h" #include "transport.h" -#include "split_util.h" #include "transaction_id_define.h" +#include "split_util.h" +#include "synchronization_util.h" #define SYNC_TIMER_OFFSET 2 @@ -63,9 +64,7 @@ static bool transaction_handler_master(matrix_row_t master_matrix[], matrix_row_ } } bool this_okay = true; - ATOMIC_BLOCK_FORCEON { - this_okay = handler(master_matrix, slave_matrix); - }; + this_okay = handler(master_matrix, slave_matrix); if (this_okay) return true; } dprintf("Failed to execute %s\n", prefix); @@ -77,11 +76,11 @@ 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) -#define TRANSACTION_HANDLER_SLAVE(prefix) \ - do { \ - ATOMIC_BLOCK_FORCEON { \ - prefix##_handlers_slave(master_matrix, slave_matrix); \ - }; \ +#define TRANSACTION_HANDLER_SLAVE(prefix) \ + do { \ + split_shared_memory_lock(); \ + prefix##_handlers_slave(master_matrix, slave_matrix); \ + split_shared_memory_unlock(); \ } while (0) inline static bool read_if_checksum_mismatch(int8_t trans_id_checksum, int8_t trans_id_retrieve, uint32_t *last_update, void *destination, const void *equiv_shmem, size_t length) {