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

Conversation

KarlK90
Copy link
Member

@KarlK90 KarlK90 commented Jun 19, 2022

Description

This fixes a deadlock bug that sneaked in with #16669. It can only be triggered on a secondary half that is not connected to the main usb connected half, thus not receiving any handshake packets. Which should be a edge-case e.g. when updating one half but still...

The problematic code that could trigger the deadlock

static THD_FUNCTION(SlaveThread, arg) {
    (void)arg;
    chRegSetThreadName("split_protocol_tx_rx");

    while (true) {
        split_shared_memory_lock(); // <-- (1.) Acquire Mutex in rx/tx thread which runs concurrent with main thread
        if (unlikely(!react_to_transaction())) {
            serial_transport_driver_clear();
        }
        split_shared_memory_unlock();
    }
}

static inline bool react_to_transaction(void) {
    uint8_t transaction_id = 0;
    if (unlikely(!serial_transport_receive_blocking(&transaction_id, sizeof(transaction_id)))) { // <-- (2.) Potentially wait forever if disconnected and no handshake packet is received.
        return false;
    }
    // SNIP
}

And now in transactions_slave() the Mutex is acquired again.

#define TRANSACTION_HANDLER_SLAVE(prefix)                     \
    do {                                                      \
        split_shared_memory_lock(); // <-- (3.) Deadlock in main thread because Mutex is already held by tx/rx thread and never released.                          \
        prefix##_handlers_slave(master_matrix, slave_matrix); \
        split_shared_memory_unlock();                         \
    } while (0)

Because manually locking and unlocking can be quite error prone, I added support for RAII-style mutex guards split_shared_memory_lock_autounlock that can be used in addition to the already existing lock / unlock functions. Unfortunately GCC cleanup attribute always wants a cleanup function with a pointer variable, so some unsexy function wrapping was necessary.

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

Issues Fixed or Closed by This PR

  • None ATM

Checklist

  • My code follows the code style of this project: C, Python
  • I have read the PR Checklist document and have made the appropriate changes.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@KarlK90 KarlK90 requested a review from a team June 19, 2022 17:49
@github-actions github-actions bot added the core label Jun 19, 2022
@KarlK90 KarlK90 requested a review from tzarc June 19, 2022 17:49
@KarlK90
Copy link
Member Author

KarlK90 commented Jun 19, 2022

CC: @preisi

@drashna drashna requested a review from a team June 20, 2022 19:54
@tzarc tzarc merged commit 2703ecc into qmk:develop Jun 20, 2022
0xcharly pushed a commit to Bastardkb/bastardkb-qmk that referenced this pull request Jul 4, 2022
0xcharly pushed a commit to Bastardkb/bastardkb-qmk that referenced this pull request Jul 4, 2022
nolanseaton pushed a commit to nolanseaton/qmk_firmware that referenced this pull request Jan 23, 2023
nolanseaton pushed a commit to nolanseaton/qmk_firmware that referenced this pull request Jan 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants