Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] Fix deadlocks on disconnected secondary half #17423

Merged
merged 1 commit into from
Jun 20, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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)