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.

Sign up for GitHub

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

[Core] rewrite locking in split transaction handlers #18417

Merged
merged 1 commit into from
Oct 4, 2022
Merged

[Core] rewrite locking in split transaction handlers #18417

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
121 changes: 88 additions & 33 deletions quantum/split_common/transactions.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,26 @@ 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)

/**
* @brief Constructs a transaction handler that doesn't acquire a lock to the
* split shared memory. Therefore the locking and unlocking has to be done
* manually inside the handler. Use this macro only if the handler is
* non-deterministic in runtime and thus needs a manual lock unlock
* implementation to hold the lock for the shortest possible time.
*/
#define TRANSACTION_HANDLER_SLAVE(prefix) \
do { \
prefix##_handlers_slave(master_matrix, slave_matrix); \
} while (0)

/**
* @brief Constructs a transaction handler that automatically acquires a lock to
* safely access the split shared memory and releases the lock again after
* processing the handler. Use this macro if the handler is fast and
* deterministic in runtime and thus holds the lock only for a very short time.
* If not fallback to manually locking and unlocking inside the handler.
*/
#define TRANSACTION_HANDLER_SLAVE_AUTOLOCK(prefix) \
do { \
split_shared_memory_lock(); \
prefix##_handlers_slave(master_matrix, slave_matrix); \
Expand Down Expand Up @@ -139,7 +158,7 @@ static void slave_matrix_handlers_slave(matrix_row_t master_matrix[], matrix_row

// clang-format off
#define TRANSACTIONS_SLAVE_MATRIX_MASTER() TRANSACTION_HANDLER_MASTER(slave_matrix)
#define TRANSACTIONS_SLAVE_MATRIX_SLAVE() TRANSACTION_HANDLER_SLAVE(slave_matrix)
#define TRANSACTIONS_SLAVE_MATRIX_SLAVE() TRANSACTION_HANDLER_SLAVE_AUTOLOCK(slave_matrix)
#define TRANSACTIONS_SLAVE_MATRIX_REGISTRATIONS \
[GET_SLAVE_MATRIX_CHECKSUM] = trans_target2initiator_initializer(smatrix.checksum), \
[GET_SLAVE_MATRIX_DATA] = trans_target2initiator_initializer(smatrix.matrix),
Expand All @@ -161,7 +180,7 @@ static void master_matrix_handlers_slave(matrix_row_t master_matrix[], matrix_ro
}

# define TRANSACTIONS_MASTER_MATRIX_MASTER() TRANSACTION_HANDLER_MASTER(master_matrix)
# define TRANSACTIONS_MASTER_MATRIX_SLAVE() TRANSACTION_HANDLER_SLAVE(master_matrix)
# define TRANSACTIONS_MASTER_MATRIX_SLAVE() TRANSACTION_HANDLER_SLAVE_AUTOLOCK(master_matrix)
# define TRANSACTIONS_MASTER_MATRIX_REGISTRATIONS [PUT_MASTER_MATRIX] = trans_initiator2target_initializer(mmatrix.matrix),

#else // SPLIT_TRANSPORT_MIRROR
Expand Down Expand Up @@ -197,7 +216,7 @@ static void encoder_handlers_slave(matrix_row_t master_matrix[], matrix_row_t sl

// clang-format off
# define TRANSACTIONS_ENCODERS_MASTER() TRANSACTION_HANDLER_MASTER(encoder)
# define TRANSACTIONS_ENCODERS_SLAVE() TRANSACTION_HANDLER_SLAVE(encoder)
# define TRANSACTIONS_ENCODERS_SLAVE() TRANSACTION_HANDLER_SLAVE_AUTOLOCK(encoder)
# define TRANSACTIONS_ENCODERS_REGISTRATIONS \
[GET_ENCODERS_CHECKSUM] = trans_target2initiator_initializer(encoders.checksum), \
[GET_ENCODERS_DATA] = trans_target2initiator_initializer(encoders.state),
Expand Down Expand Up @@ -239,7 +258,7 @@ static void sync_timer_handlers_slave(matrix_row_t master_matrix[], matrix_row_t
}

# define TRANSACTIONS_SYNC_TIMER_MASTER() TRANSACTION_HANDLER_MASTER(sync_timer)
# define TRANSACTIONS_SYNC_TIMER_SLAVE() TRANSACTION_HANDLER_SLAVE(sync_timer)
# define TRANSACTIONS_SYNC_TIMER_SLAVE() TRANSACTION_HANDLER_SLAVE_AUTOLOCK(sync_timer)
# define TRANSACTIONS_SYNC_TIMER_REGISTRATIONS [PUT_SYNC_TIMER] = trans_initiator2target_initializer(sync_timer),

#else // DISABLE_SYNC_TIMER
Expand Down Expand Up @@ -273,7 +292,7 @@ static void layer_state_handlers_slave(matrix_row_t master_matrix[], matrix_row_

// clang-format off
# define TRANSACTIONS_LAYER_STATE_MASTER() TRANSACTION_HANDLER_MASTER(layer_state)
# define TRANSACTIONS_LAYER_STATE_SLAVE() TRANSACTION_HANDLER_SLAVE(layer_state)
# define TRANSACTIONS_LAYER_STATE_SLAVE() TRANSACTION_HANDLER_SLAVE_AUTOLOCK(layer_state)
# define TRANSACTIONS_LAYER_STATE_REGISTRATIONS \
[PUT_LAYER_STATE] = trans_initiator2target_initializer(layers.layer_state), \
[PUT_DEFAULT_LAYER_STATE] = trans_initiator2target_initializer(layers.default_layer_state),
Expand Down Expand Up @@ -304,7 +323,7 @@ static void led_state_handlers_slave(matrix_row_t master_matrix[], matrix_row_t
}

# define TRANSACTIONS_LED_STATE_MASTER() TRANSACTION_HANDLER_MASTER(led_state)
# define TRANSACTIONS_LED_STATE_SLAVE() TRANSACTION_HANDLER_SLAVE(led_state)
# define TRANSACTIONS_LED_STATE_SLAVE() TRANSACTION_HANDLER_SLAVE_AUTOLOCK(led_state)
# define TRANSACTIONS_LED_STATE_REGISTRATIONS [PUT_LED_STATE] = trans_initiator2target_initializer(led_state),

#else // SPLIT_LED_STATE_ENABLE
Expand Down Expand Up @@ -353,10 +372,15 @@ static bool mods_handlers_master(matrix_row_t master_matrix[], matrix_row_t slav
}

static void mods_handlers_slave(matrix_row_t master_matrix[], matrix_row_t slave_matrix[]) {
set_mods(split_shmem->mods.real_mods);
set_weak_mods(split_shmem->mods.weak_mods);
split_shared_memory_lock();
split_mods_sync_t mods;
memcpy(&mods, &split_shmem->mods, sizeof(split_mods_sync_t));
split_shared_memory_unlock();

set_mods(mods.real_mods);
set_weak_mods(mods.weak_mods);
# ifndef NO_ACTION_ONESHOT
set_oneshot_mods(split_shmem->mods.oneshot_mods);
set_oneshot_mods(mods.oneshot_mods);
# endif
}

Expand Down Expand Up @@ -384,7 +408,11 @@ static bool backlight_handlers_master(matrix_row_t master_matrix[], matrix_row_t
}

static void backlight_handlers_slave(matrix_row_t master_matrix[], matrix_row_t slave_matrix[]) {
backlight_set(split_shmem->backlight_level);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep this line - locking is not necessary for accessing a single byte.

split_shared_memory_lock();
uint8_t backlight_level = split_shmem->backlight_level;
split_shared_memory_unlock();

backlight_set(backlight_level);
}

# define TRANSACTIONS_BACKLIGHT_MASTER() TRANSACTION_HANDLER_MASTER(backlight)
Expand Down Expand Up @@ -417,10 +445,15 @@ static bool rgblight_handlers_master(matrix_row_t master_matrix[], matrix_row_t
}

static void rgblight_handlers_slave(matrix_row_t master_matrix[], matrix_row_t slave_matrix[]) {
split_shared_memory_lock();
// Update the RGB with the new data
if (split_shmem->rgblight_sync.status.change_flags != 0) {
rgblight_update_sync(&split_shmem->rgblight_sync, false);
split_shmem->rgblight_sync.status.change_flags = 0;
rgblight_syncinfo_t rgblight_sync;
memcpy(&rgblight_sync, &split_shmem->rgblight_sync, sizeof(rgblight_syncinfo_t));
split_shmem->rgblight_sync.status.change_flags = 0;
split_shared_memory_unlock();

if (rgblight_sync.status.change_flags != 0) {
rgblight_update_sync(&rgblight_sync, false);
}
}

Expand Down Expand Up @@ -450,8 +483,12 @@ static bool led_matrix_handlers_master(matrix_row_t master_matrix[], matrix_row_
}

static void led_matrix_handlers_slave(matrix_row_t master_matrix[], matrix_row_t slave_matrix[]) {
split_shared_memory_lock();
memcpy(&led_matrix_eeconfig, &split_shmem->led_matrix_sync.led_matrix, sizeof(led_eeconfig_t));
led_matrix_set_suspend_state(split_shmem->led_matrix_sync.led_suspend_state);
bool led_suspend_state = split_shmem->led_matrix_sync.led_suspend_state;
split_shared_memory_unlock();

led_matrix_set_suspend_state(led_suspend_state);
}

# define TRANSACTIONS_LED_MATRIX_MASTER() TRANSACTION_HANDLER_MASTER(led_matrix)
Expand Down Expand Up @@ -480,8 +517,12 @@ static bool rgb_matrix_handlers_master(matrix_row_t master_matrix[], matrix_row_
}

static void rgb_matrix_handlers_slave(matrix_row_t master_matrix[], matrix_row_t slave_matrix[]) {
split_shared_memory_lock();
memcpy(&rgb_matrix_config, &split_shmem->rgb_matrix_sync.rgb_matrix, sizeof(rgb_config_t));
rgb_matrix_set_suspend_state(split_shmem->rgb_matrix_sync.rgb_suspend_state);
bool rgb_suspend_state = split_shmem->rgb_matrix_sync.rgb_suspend_state;
split_shared_memory_unlock();

rgb_matrix_set_suspend_state(rgb_suspend_state);
}

# define TRANSACTIONS_RGB_MATRIX_MASTER() TRANSACTION_HANDLER_MASTER(rgb_matrix)
Expand Down Expand Up @@ -512,7 +553,7 @@ static void wpm_handlers_slave(matrix_row_t master_matrix[], matrix_row_t slave_
}

# define TRANSACTIONS_WPM_MASTER() TRANSACTION_HANDLER_MASTER(wpm)
# define TRANSACTIONS_WPM_SLAVE() TRANSACTION_HANDLER_SLAVE(wpm)
# define TRANSACTIONS_WPM_SLAVE() TRANSACTION_HANDLER_SLAVE_AUTOLOCK(wpm)
# define TRANSACTIONS_WPM_REGISTRATIONS [PUT_WPM] = trans_initiator2target_initializer(current_wpm),

#else // defined(WPM_ENABLE) && defined(SPLIT_WPM_ENABLE)
Expand All @@ -535,7 +576,11 @@ static bool oled_handlers_master(matrix_row_t master_matrix[], matrix_row_t slav
}

static void oled_handlers_slave(matrix_row_t master_matrix[], matrix_row_t slave_matrix[]) {
if (split_shmem->current_oled_state) {
split_shared_memory_lock();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The value of having a lock around a single read of a byte; either you read the value before or after the concurrent update, but there is no possibility of getting an inconsistent view of a single byte.

Copy link
Member Author

@KarlK90 KarlK90 Sep 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, I didn't think of that. I can surely change that, on the other hand I'm thinking: if someone changes the datatype in the future that doesn't have this atomic guarantees will they remember to introduce a lock here? I would bet that they don't do that, so while redundant at this point it could stay as a precaution? But I'm not commited to keeping it by all means. I totally forgot that the next step will be adding checksums to all transactions, so the atomic guarantees won't be there in the near future.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Appreciate the concern for defensive measures, though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing a checksum on a single byte isn't going to speed things up either -- may as well just transfer the actual byte unconditionally.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, but the checksum is actually meant to prevent/detect data corruption as we usually don't use any resilient to means of communication (except for odd/even parity checks for some USART peripherals).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood; but in that circumstance the request/response nature of the protocol should probably be modified such that at a certain amount of data transfer, you transmit payload+checksum in a single operation. That's a longer term discussion, at least.

Copy link
Member Author

@KarlK90 KarlK90 Oct 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure the refactoring should be done after some discussion, does this prevent merging the changes in this PR?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No.

uint8_t current_oled_state = split_shmem->current_oled_state;
split_shared_memory_unlock();

if (current_oled_state) {
oled_on();
} else {
oled_off();
Expand Down Expand Up @@ -566,15 +611,19 @@ static bool st7565_handlers_master(matrix_row_t master_matrix[], matrix_row_t sl
}

static void st7565_handlers_slave(matrix_row_t master_matrix[], matrix_row_t slave_matrix[]) {
if (split_shmem->current_st7565_state) {
split_shared_memory_lock();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

uint8_t current_st7565_state = split_shmem->current_st7565_state;
split_shared_memory_unlock();

if (current_st7565_state) {
st7565_on();
} else {
st7565_off();
}
}

# define TRANSACTIONS_ST7565_MASTER() TRANSACTION_HANDLER_MASTER(st7565)
# define TRANSACTIONS_ST7565_SLAVE() TRANSACTION_HANDLER_SLAVE(st7565)
# define TRANSACTIONS_ST7565_SLAVE() TRANSACTION_HANDLER_SLAVE_AUTOLOCK(st7565)
# define TRANSACTIONS_ST7565_REGISTRATIONS [PUT_ST7565] = trans_initiator2target_initializer(current_st7565_state),

#else // defined(ST7565_ENABLE) && defined(SPLIT_ST7565_ENABLE)
Expand Down Expand Up @@ -607,9 +656,9 @@ static bool pointing_handlers_master(matrix_row_t master_matrix[], matrix_row_t
bool okay = read_if_checksum_mismatch(GET_POINTING_CHECKSUM, GET_POINTING_DATA, &last_update, &temp_state, &split_shmem->pointing.report, sizeof(temp_state));
if (okay) pointing_device_set_shared_report(temp_state);
temp_cpi = pointing_device_get_shared_cpi();
if (temp_cpi && memcmp(&last_cpi, &temp_cpi, sizeof(temp_cpi)) != 0) {
memcpy(&split_shmem->pointing.cpi, &temp_cpi, sizeof(temp_cpi));
okay = transport_write(PUT_POINTING_CPI, &split_shmem->pointing.cpi, sizeof(split_shmem->pointing.cpi));
if (temp_cpi && last_cpi != temp_cpi) {
split_shmem->pointing.cpi = temp_cpi;
okay = transport_write(PUT_POINTING_CPI, &split_shmem->pointing.cpi, sizeof(split_shmem->pointing.cpi));
if (okay) {
last_cpi = temp_cpi;
}
Expand All @@ -629,26 +678,32 @@ static void pointing_handlers_slave(matrix_row_t master_matrix[], matrix_row_t s
return;
}
# endif
report_mouse_t temp_report;
uint16_t temp_cpi;
# if (POINTING_DEVICE_TASK_THROTTLE_MS > 0)
static uint32_t last_exec = 0;
if (timer_elapsed32(last_exec) < POINTING_DEVICE_TASK_THROTTLE_MS) {
return;
}
last_exec = timer_read32();
# endif
temp_cpi = !pointing_device_driver.get_cpi ? 0 : pointing_device_driver.get_cpi(); // check for NULL
if (split_shmem->pointing.cpi && memcmp(&split_shmem->pointing.cpi, &temp_cpi, sizeof(temp_cpi)) != 0) {
if (pointing_device_driver.set_cpi) {
pointing_device_driver.set_cpi(split_shmem->pointing.cpi);
}

uint16_t temp_cpi = !pointing_device_driver.get_cpi ? 0 : pointing_device_driver.get_cpi(); // check for NULL

split_shared_memory_lock();
split_slave_pointing_sync_t pointing;
memcpy(&pointing, &split_shmem->pointing, sizeof(split_slave_pointing_sync_t));
split_shared_memory_unlock();

if (pointing.cpi && pointing.cpi != temp_cpi && pointing_device_driver.set_cpi) {
pointing_device_driver.set_cpi(pointing.cpi);
}
memset(&temp_report, 0, sizeof(temp_report));
temp_report = pointing_device_driver.get_report(temp_report);
memcpy(&split_shmem->pointing.report, &temp_report, sizeof(temp_report));

pointing.report = pointing_device_driver.get_report((report_mouse_t){0});
// Now update the checksum given that the pointing has been written to
split_shmem->pointing.checksum = crc8(&temp_report, sizeof(temp_report));
pointing.checksum = crc8(&pointing.report, sizeof(report_mouse_t));

split_shared_memory_lock();
memcpy(&split_shmem->pointing, &pointing, sizeof(split_slave_pointing_sync_t));
split_shared_memory_unlock();
}

# define TRANSACTIONS_POINTING_MASTER() TRANSACTION_HANDLER_MASTER(pointing)
Expand Down