From 7712a286dccea029785976311433cf8673594f6f Mon Sep 17 00:00:00 2001 From: Stefan Kerkmann Date: Tue, 19 Apr 2022 12:56:16 +0200 Subject: [PATCH] [Core] Use a mutex guard for split shared memory (#16647) --- 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 bb7b3c0554..0cff057d1d 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(); } } @@ -205,14 +209,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 @@ -239,8 +238,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 @@ -286,3 +284,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 85c64214d1..e9fa4af7a3 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 91dd0479bc..21751f23fd 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 0000000000..bc4a4e621f --- /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 0000000000..3730f271db --- /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 105bf918cb..9e3df534e3 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) {