From 5095d47320c36e2346d41eb7e35dcbd215886557 Mon Sep 17 00:00:00 2001 From: Kimplul Date: Mon, 3 Nov 2025 20:17:32 +0200 Subject: minimize critical section in work handler + Effects handlers were being called in a locked context, which in this case happened to work due to how `hid_hw_request` is implemented. Now, only the selected state is updated and the heavy lifting of making the wheel aware of the requested changes is done after the critical section. --- src/hid-tmff2.c | 78 +++++++++++++++++++++++++++++---------------- src/hid-tmff2.h | 16 +++++----- src/tmt300rs/hid-tmt300rs.c | 24 +++++++------- 3 files changed, 71 insertions(+), 47 deletions(-) diff --git a/src/hid-tmff2.c b/src/hid-tmff2.c index b6bb834..ee3188a 100644 --- a/src/hid-tmff2.c +++ b/src/hid-tmff2.c @@ -302,15 +302,22 @@ static void tmff2_work_handler(struct work_struct *w) return; for (effect_id = 0; effect_id < tmff2->max_effects; ++effect_id) { - spin_lock_irqsave(&tmff2->lock, lock_flags); + unsigned long actions = 0; + struct tmff2_effect_state effect; time_now = JIFFIES2MS(jiffies); + state = &tmff2->states[effect_id]; + /* critical section for updating state flags, keep log of what + * actions to take after the critical section with actions */ + spin_lock_irqsave(&tmff2->lock, lock_flags); + effect_delay = state->effect.replay.delay; effect_length = state->effect.replay.length; if (test_bit(FF_EFFECT_PLAYING, &state->flags) && effect_length) { - if ((time_now - state->start_time) >= (effect_delay + effect_length) * state->count) { + if ((time_now - state->start_time) >= + (effect_delay + effect_length) * state->count) { __clear_bit(FF_EFFECT_PLAYING, &state->flags); __clear_bit(FF_EFFECT_QUEUE_UPDATE, &state->flags); @@ -319,47 +326,64 @@ static void tmff2_work_handler(struct work_struct *w) } if (test_bit(FF_EFFECT_QUEUE_UPLOAD, &state->flags)) { - if (tmff2->upload_effect(tmff2->data, state)) { - hid_warn(tmff2->hdev, "failed uploading effect\n"); - } else { - __clear_bit(FF_EFFECT_QUEUE_UPLOAD, &state->flags); - /* if we're uploading an effect, it's bound to be the up - * to date */ - __clear_bit(FF_EFFECT_QUEUE_UPDATE, &state->flags); - } + __set_bit(FF_EFFECT_QUEUE_UPLOAD, &actions); + __clear_bit(FF_EFFECT_QUEUE_UPLOAD, &state->flags); + /* if we're uploading an effect, it's bound to be up + * to date */ + __clear_bit(FF_EFFECT_QUEUE_UPDATE, &state->flags); } if (test_bit(FF_EFFECT_QUEUE_UPDATE, &state->flags)) { - if (tmff2->update_effect(tmff2->data, state)) - hid_warn(tmff2->hdev, "failed updating effect\n"); - else - __clear_bit(FF_EFFECT_QUEUE_UPDATE, &state->flags); + __set_bit(FF_EFFECT_QUEUE_UPDATE, &actions); + __clear_bit(FF_EFFECT_QUEUE_UPDATE, &state->flags); } if (test_bit(FF_EFFECT_QUEUE_START, &state->flags)) { - if (tmff2->play_effect(tmff2->data, state)) { - hid_warn(tmff2->hdev, "failed starting effect\n"); - } else { - __clear_bit(FF_EFFECT_QUEUE_START, &state->flags); - __set_bit(FF_EFFECT_PLAYING, &state->flags); - } - + __set_bit(FF_EFFECT_QUEUE_START, &actions); + __clear_bit(FF_EFFECT_QUEUE_START, &state->flags); + /* effect is playing since we're started it right now */ + __set_bit(FF_EFFECT_PLAYING, &state->flags); } if (test_bit(FF_EFFECT_QUEUE_STOP, &state->flags)) { - if (tmff2->stop_effect(tmff2->data, state)) { - hid_warn(tmff2->hdev, "failed stopping effect\n"); - } else { - __clear_bit(FF_EFFECT_PLAYING, &state->flags); - __clear_bit(FF_EFFECT_QUEUE_STOP, &state->flags); - } + __set_bit(FF_EFFECT_QUEUE_STOP, &actions); + __clear_bit(FF_EFFECT_QUEUE_STOP, &state->flags); + + /* the effect can't be playing if we're stopped, aye? */ + __clear_bit(FF_EFFECT_PLAYING, &state->flags); } if (state->count > max_count) max_count = state->count; + /* copy effect state to local variable so we can pass it around + * after the atomic section */ + effect = *state; + spin_unlock_irqrestore(&tmff2->lock, lock_flags); + /* perform the identified actions */ + if (test_bit(FF_EFFECT_QUEUE_UPLOAD, &actions) + && tmff2->upload_effect(tmff2->data, &effect)) { + hid_warn(tmff2->hdev, "failed uploading effect\n"); + } + + if (test_bit(FF_EFFECT_QUEUE_UPDATE, &actions) + && tmff2->update_effect(tmff2->data, &effect)) { + hid_warn(tmff2->hdev, "failed updating effect\n"); + } + + if (test_bit(FF_EFFECT_QUEUE_START, &actions) + && tmff2->play_effect(tmff2->data, &effect)) { + hid_warn(tmff2->hdev, "failed starting effect\n"); + } + + if (test_bit(FF_EFFECT_QUEUE_STOP, &actions) + && tmff2->stop_effect(tmff2->data, &effect)) { + hid_warn(tmff2->hdev, "failed stopping effect\n"); + } + + /* wait for each effect update to actually be sent out to avoid * filling up usb output queue */ hid_hw_wait(tmff2->hdev); diff --git a/src/hid-tmff2.h b/src/hid-tmff2.h index 666d249..f747f44 100644 --- a/src/hid-tmff2.h +++ b/src/hid-tmff2.h @@ -73,10 +73,10 @@ struct tmff2_device_entry { signed short supported_effects[FF_CNT]; /* obligatory callbacks */ - int (*play_effect)(void *data, struct tmff2_effect_state *state); - int (*upload_effect)(void *data, struct tmff2_effect_state *state); - int (*update_effect)(void *data, struct tmff2_effect_state *state); - int (*stop_effect)(void *data, struct tmff2_effect_state *state); + int (*play_effect)(void *data, const struct tmff2_effect_state *state); + int (*upload_effect)(void *data, const struct tmff2_effect_state *state); + int (*update_effect)(void *data, const struct tmff2_effect_state *state); + int (*stop_effect)(void *data, const struct tmff2_effect_state *state); int (*wheel_init)(struct tmff2_device_entry *tmff2, int open_mode); int (*wheel_destroy)(void *data); @@ -134,10 +134,10 @@ struct t300rs_device_entry { u8 *send_buffer; }; -int t300rs_play_effect(void *, struct tmff2_effect_state *); -int t300rs_upload_effect(void *, struct tmff2_effect_state *); -int t300rs_update_effect(void *, struct tmff2_effect_state *); -int t300rs_stop_effect(void *, struct tmff2_effect_state *); +int t300rs_play_effect(void *, const struct tmff2_effect_state *); +int t300rs_upload_effect(void *, const struct tmff2_effect_state *); +int t300rs_update_effect(void *, const struct tmff2_effect_state *); +int t300rs_stop_effect(void *, const struct tmff2_effect_state *); int t300rs_open(void *, int); int t300rs_close(void *, int); diff --git a/src/tmt300rs/hid-tmt300rs.c b/src/tmt300rs/hid-tmt300rs.c index 3966530..c247bd7 100644 --- a/src/tmt300rs/hid-tmt300rs.c +++ b/src/tmt300rs/hid-tmt300rs.c @@ -488,7 +488,7 @@ static void t300rs_fill_header(struct t300rs_packet_header *packet_header, packet_header->code = code; } -int t300rs_play_effect(void *data, struct tmff2_effect_state *state) +int t300rs_play_effect(void *data, const struct tmff2_effect_state *state) { struct t300rs_device_entry *t300rs = data; struct __packed t300rs_packet_play { @@ -515,7 +515,7 @@ int t300rs_play_effect(void *data, struct tmff2_effect_state *state) return ret; } -int t300rs_stop_effect(void *data, struct tmff2_effect_state *state) +int t300rs_stop_effect(void *data, const struct tmff2_effect_state *state) { struct t300rs_device_entry *t300rs = data; struct __packed t300rs_packet_stop { @@ -566,7 +566,7 @@ static void t300rs_fill_timing(struct t300rs_packet_timing *packet_timing, } static int t300rs_update_constant(struct t300rs_device_entry *t300rs, - struct tmff2_effect_state *state) + const struct tmff2_effect_state *state) { struct ff_effect effect = state->effect; struct ff_effect old = state->old; @@ -616,7 +616,7 @@ static int t300rs_update_constant(struct t300rs_device_entry *t300rs, } static int t300rs_update_ramp(struct t300rs_device_entry *t300rs, - struct tmff2_effect_state *state) + const struct tmff2_effect_state *state) { struct ff_effect effect = state->effect; struct ff_effect old = state->old; @@ -676,7 +676,7 @@ static int t300rs_update_ramp(struct t300rs_device_entry *t300rs, } static int t300rs_update_condition(struct t300rs_device_entry *t300rs, - struct tmff2_effect_state *state) + const struct tmff2_effect_state *state) { struct ff_effect effect = state->effect; struct ff_effect old = state->old; @@ -754,7 +754,7 @@ static int t300rs_update_condition(struct t300rs_device_entry *t300rs, } static int t300rs_update_periodic(struct t300rs_device_entry *t300rs, - struct tmff2_effect_state *state) + const struct tmff2_effect_state *state) { struct ff_effect effect = state->effect; struct ff_effect old = state->old; @@ -816,7 +816,7 @@ static int t300rs_update_periodic(struct t300rs_device_entry *t300rs, } static int t300rs_upload_constant(struct t300rs_device_entry *t300rs, - struct tmff2_effect_state *state) + const struct tmff2_effect_state *state) { struct ff_effect effect = state->effect; struct ff_constant_effect constant = state->effect.u.constant; @@ -853,7 +853,7 @@ static int t300rs_upload_constant(struct t300rs_device_entry *t300rs, } static int t300rs_upload_ramp(struct t300rs_device_entry *t300rs, - struct tmff2_effect_state *state) + const struct tmff2_effect_state *state) { struct ff_effect effect = state->effect; struct ff_ramp_effect ramp = state->effect.u.ramp; @@ -900,7 +900,7 @@ static int t300rs_upload_ramp(struct t300rs_device_entry *t300rs, } static int t300rs_upload_condition(struct t300rs_device_entry *t300rs, - struct tmff2_effect_state *state) + const struct tmff2_effect_state *state) { struct ff_effect effect = state->effect; /* we only care about the first axis */ @@ -965,7 +965,7 @@ static int t300rs_upload_condition(struct t300rs_device_entry *t300rs, } static int t300rs_upload_periodic(struct t300rs_device_entry *t300rs, - struct tmff2_effect_state *state) + const struct tmff2_effect_state *state) { struct ff_effect effect = state->effect; struct __packed t300rs_packet_periodic { @@ -1016,7 +1016,7 @@ static int t300rs_upload_periodic(struct t300rs_device_entry *t300rs, return ret; } -int t300rs_update_effect(void *data, struct tmff2_effect_state *state) +int t300rs_update_effect(void *data, const struct tmff2_effect_state *state) { struct t300rs_device_entry *t300rs = data; switch (state->effect.type) { @@ -1038,7 +1038,7 @@ int t300rs_update_effect(void *data, struct tmff2_effect_state *state) } } -int t300rs_upload_effect(void *data, struct tmff2_effect_state *state) +int t300rs_upload_effect(void *data, const struct tmff2_effect_state *state) { struct t300rs_device_entry *t300rs = data; switch (state->effect.type) { -- cgit v1.3