Skip to content

Commit

Permalink
[BUG] Fix deadlocks on disconnected secondary half (qmk#17423)
Browse files Browse the repository at this point in the history
  • Loading branch information
KarlK90 authored and nolanseaton committed Jan 23, 2023
1 parent 05ffe92 commit f13f242
Show file tree
Hide file tree
Showing 3 changed files with 38 additions and 11 deletions.
11 changes: 4 additions & 7 deletions platforms/chibios/drivers/serial.c
Original file line number Diff line number Diff line change
Expand Up @@ -87,10 +87,7 @@ 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();
}
}

Expand Down Expand Up @@ -155,6 +152,7 @@ static void __attribute__((noinline)) serial_write_byte(uint8_t data) {

// interrupt handle to be used by the slave device
void interrupt_handler(void *arg) {
split_shared_memory_lock_autounlock();
chSysLockFromISR();

sync_send();
Expand Down Expand Up @@ -212,6 +210,8 @@ void interrupt_handler(void *arg) {
static inline bool initiate_transaction(uint8_t sstd_index) {
if (sstd_index > NUM_TOTAL_TRANSACTIONS) return false;

split_shared_memory_lock_autounlock();

split_transaction_desc_t *trans = &split_transaction_table[sstd_index];

// TODO: remove extra delay between transactions
Expand Down Expand Up @@ -292,8 +292,5 @@ static inline bool initiate_transaction(uint8_t 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;
return initiate_transaction((uint8_t)sstd_index);
}
8 changes: 4 additions & 4 deletions platforms/chibios/drivers/serial_protocol.c
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,11 @@ static THD_FUNCTION(SlaveThread, arg) {
chRegSetThreadName("split_protocol_tx_rx");

while (true) {
split_shared_memory_lock();
if (unlikely(!react_to_transaction())) {
/* Clear the receive queue, to start with a clean slate.
* Parts of failed transactions or spurious bytes could still be in it. */
serial_transport_driver_clear();
}
split_shared_memory_unlock();
}
}

Expand Down Expand Up @@ -64,6 +62,8 @@ static inline bool react_to_transaction(void) {
return false;
}

split_shared_memory_lock_autounlock();

split_transaction_desc_t* transaction = &split_transaction_table[transaction_id];

/* Send back the handshake which is XORed as a simple checksum,
Expand Down Expand Up @@ -102,9 +102,7 @@ static inline bool react_to_transaction(void) {
* @return bool Indicates success of transaction.
*/
bool soft_serial_transaction(int index) {
split_shared_memory_lock();
bool result = initiate_transaction((uint8_t)index);
split_shared_memory_unlock();

if (unlikely(!result)) {
/* Clear the receive queue, to start with a clean slate.
Expand All @@ -125,6 +123,8 @@ static inline bool initiate_transaction(uint8_t transaction_id) {
return false;
}

split_shared_memory_lock_autounlock();

split_transaction_desc_t* transaction = &split_transaction_table[transaction_id];

/* Send transaction table index to the slave, which doubles as basic handshake token. */
Expand Down
30 changes: 30 additions & 0 deletions platforms/synchronization_util.h
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,33 @@ void split_shared_memory_unlock(void);
inline void split_shared_memory_lock(void){};
inline void split_shared_memory_unlock(void){};
#endif

/* GCCs cleanup attribute expects a function with one parameter, which is a
* pointer to a type compatible with the variable. As we don't want to expose
* the platforms internal mutex type this workaround with auto generated adapter
* function is defined */
#define QMK_DECLARE_AUTOUNLOCK_HELPERS(prefix) \
inline unsigned prefix##_autounlock_lock_helper(void) { \
prefix##_lock(); \
return 0; \
} \
\
inline void prefix##_autounlock_unlock_helper(unsigned* unused_guard) { \
prefix##_unlock(); \
}

/* Convinience macro the automatically generate the correct RAII-style
* lock_autounlock function macro */
#define QMK_DECLARE_AUTOUNLOCK_CALL(prefix) unsigned prefix##_guard __attribute__((unused, cleanup(prefix##_autounlock_unlock_helper))) = prefix##_autounlock_lock_helper

QMK_DECLARE_AUTOUNLOCK_HELPERS(split_shared_memory)

/**
* @brief Acquire exclusive access to the split keyboard shared memory, by
* calling the platforms `split_shared_memory_lock()` function. The lock is
* automatically released by calling the platforms `split_shared_memory_unlock()`
* function. This happens when the block where
* `split_shared_memory_lock_autounlock()` is called in goes out of scope i.e.
* when the enclosing function returns.
*/
#define split_shared_memory_lock_autounlock QMK_DECLARE_AUTOUNLOCK_CALL(split_shared_memory)

0 comments on commit f13f242

Please sign in to comment.