From 3ca34aa88a4dd1fb99e1abe8160d3a327d091277 Mon Sep 17 00:00:00 2001 From: Peter Hutterer Date: Tue, 29 Jul 2025 12:34:38 +1000 Subject: [PATCH] evdev: move the SYN_REPORT 1 filtering to the touchpad backend In commit 9a9466b6a92c ("evdev: discard any frame with EV_SYN SYN_REPORT 1") all frames with a SYN_REPORT 1 were discarded on the assumption of those being key repeat frames. Unfortunately the kernel uses the same sequence to simply mark *any* injected/emulated event, regardless of the cause. Key repeat events are merely the most numerous ones but as shown in commit 7140f13d82db ("evdev: track KEY_SYSRQ frames and pass them even as repeat frames") Alt+PrintScreen is also an emulated event. Issue #1165 details another case: keyboards with n-key rollover can exceed the kernel-internal event buffer, typically 8 events for devices without EV_REL/EV_ABS. Those events will be broken up by the kernel into multiple frames - once nevents == buffer_size the current state is flushed as SYN_REPORT 1 frame. Then, if any more events are pending those are flushed as SYN_REPORT 0 frame. In the case of exactly 8 events, the second frame is never present, so we cannot easily detect if another one is coming. Issue #1145 only affects us in the touchpad code, the rest of the backends seem to (so far) be fine. So let's move the discarding of SYN_REPORT 1 to the touchpad backend and leave the rest of the code as-is. This effectively Reverts: 7140f13d82db ("evdev: track KEY_SYSRQ frames and pass them even as repeat frames") Reverts: 9a9466b6a92c ("evdev: discard any frame with EV_SYN SYN_REPORT 1") Closes #1165 Part-of: --- src/evdev-mt-touchpad.c | 16 ++++++++ src/evdev.c | 21 +--------- test/litest-device-keyboard.c | 2 + test/litest.c | 10 +++++ test/litest.h | 7 ++++ test/test-device.c | 35 ---------------- test/test-keyboard.c | 77 +++++++++++++++++++++++++++++------ 7 files changed, 100 insertions(+), 68 deletions(-) diff --git a/src/evdev-mt-touchpad.c b/src/evdev-mt-touchpad.c index d161ab6a..7d8735ae 100644 --- a/src/evdev-mt-touchpad.c +++ b/src/evdev-mt-touchpad.c @@ -1889,6 +1889,22 @@ tp_interface_process(struct evdev_dispatch *dispatch, size_t nevents; struct evdev_event *events = evdev_frame_get_events(frame, &nevents); + struct evdev_event *ev_syn = &events[nevents - 1]; + if (evdev_usage_enum(ev_syn->usage) == EVDEV_SYN_REPORT) { + /* A SYN_REPORT 1 event is a kernel-inserted SYN_REPORT. + * This happens most commonly on key repeat but in the + * touchpad code this causes issues with + * timestamp deltas (see e.g. #1145). + * + * Let's drop this frame, hoping it wasn't important. + */ + if (ev_syn->value == 1) { + return; + } + } else { + evdev_log_bug_libinput(device, "Terminating event is not a SYN_REPORT"); + } + for (size_t i = 0; i < nevents; i++) { tp_interface_process_event(dispatch, device, &events[i], time); } diff --git a/src/evdev.c b/src/evdev.c index b5f860c9..1457a254 100644 --- a/src/evdev.c +++ b/src/evdev.c @@ -1043,7 +1043,6 @@ evdev_device_dispatch(void *data) struct input_event ev; int rc; bool once = false; - bool had_sysrq = false; _unref_(evdev_frame) *frame = evdev_frame_new(64); /* If the compositor is repainting, this function is called only once @@ -1084,27 +1083,9 @@ evdev_device_dispatch(void *data) device, "event frame overflow, discarding events.\n"); } - /* Alt+Printscreen is always a repeat frame, see - * drivers/tty/sysrq.c:sysrq_reinject_alt_sysrq() in the - * kernel - */ - if (ev.type == EV_KEY && ev.code == KEY_SYSRQ) - had_sysrq = true; - if (ev.type == EV_SYN && ev.code == SYN_REPORT) { - /* A SYN_REPORT 1 event is a kernel-inserted - * auto-repeat. Nothing in libinput cares about kernel - * repeats and the inserted frame causes issues with - * timestamp deltas (see e.g. #1145) - * - * (well, except Alt+Printscreen (KEY_SYSRQ)) - */ - if (ev.value != 1 || had_sysrq) - evdev_device_dispatch_frame(libinput, - device, - frame); + evdev_device_dispatch_frame(libinput, device, frame); evdev_frame_reset(frame); - had_sysrq = false; } } else if (rc == -ENODEV) { evdev_device_remove(device); diff --git a/test/litest-device-keyboard.c b/test/litest-device-keyboard.c index 307c796b..39103f18 100644 --- a/test/litest-device-keyboard.c +++ b/test/litest-device-keyboard.c @@ -189,6 +189,8 @@ static int events[] = { EV_LED, LED_NUML, EV_LED, LED_CAPSL, EV_LED, LED_SCROLLL, + + EV_MSC, MSC_SCAN, -1, -1, }; /* clang-format on */ diff --git a/test/litest.c b/test/litest.c index 2e0c16c3..ad484ea0 100644 --- a/test/litest.c +++ b/test/litest.c @@ -2402,6 +2402,16 @@ litest_device_destroy(struct litest_device *d) path); } +void +litest_event_unchecked(struct litest_device *d, + unsigned int type, + unsigned int code, + int value) +{ + int ret = libevdev_uinput_write_event(d->uinput, type, code, value); + litest_assert_neg_errno_success(ret); +} + void litest_event(struct litest_device *d, unsigned int type, unsigned int code, int value) { diff --git a/test/litest.h b/test/litest.h index 79152b91..4b310146 100644 --- a/test/litest.h +++ b/test/litest.h @@ -879,6 +879,13 @@ _litest_dispatch(struct libinput *li, const char *func, int line); void litest_event(struct litest_device *t, unsigned int type, unsigned int code, int value); + +void +litest_event_unchecked(struct litest_device *d, + unsigned int type, + unsigned int code, + int value); + int litest_auto_assign_value(struct litest_device *d, const struct input_event *ev, diff --git a/test/test-device.c b/test/test-device.c index 036f70bc..660754fe 100644 --- a/test/test-device.c +++ b/test/test-device.c @@ -1656,36 +1656,6 @@ START_TEST(device_button_down_remove) } END_TEST -START_TEST(device_ignore_repeat_frames) -{ - struct litest_device *dev = litest_current_device(); - struct libinput *li = dev->libinput; - - unsigned int code = BTN_LEFT; - if (!libevdev_has_event_code(dev->evdev, EV_KEY, code)) - code = BTN_0; - if (!libevdev_has_event_code(dev->evdev, EV_KEY, code)) - code = KEY_A; - if (!libevdev_has_event_code(dev->evdev, EV_KEY, code)) - return LITEST_NOT_APPLICABLE; - - litest_drain_events(li); - - /* Send a button/key press event with a repeat frame - * (SYN_REPORT value 1). Notably, the actual event in this frame is - * *not* a repeat (value 2), the whole event frame is a repeat frame - * though. */ - litest_event(dev, EV_KEY, code, 1); - litest_event(dev, EV_SYN, SYN_REPORT, 1); - litest_dispatch(li); - litest_event(dev, EV_KEY, code, 0); - litest_event(dev, EV_SYN, SYN_REPORT, 1); - litest_dispatch(li); - - litest_assert_empty_queue(li); -} -END_TEST - TEST_COLLECTION(device) { /* clang-format off */ @@ -1774,10 +1744,5 @@ TEST_COLLECTION(device) litest_add(device_button_down_remove, LITEST_BUTTON, LITEST_ANY); - /* Run for various combinations of devices to hopefully cover most backends */ - litest_add(device_ignore_repeat_frames, LITEST_BUTTON, LITEST_ANY); - litest_add(device_ignore_repeat_frames, LITEST_KEYS, LITEST_ANY); - litest_add(device_ignore_repeat_frames, LITEST_TABLET, LITEST_ANY); - litest_add(device_ignore_repeat_frames, LITEST_TABLET_PAD, LITEST_ANY); /* clang-format off */ } diff --git a/test/test-keyboard.c b/test/test-keyboard.c index 9d4f8687..1b002acd 100644 --- a/test/test-keyboard.c +++ b/test/test-keyboard.c @@ -475,45 +475,45 @@ START_TEST(keyboard_alt_printscreen) litest_drain_events(li); - /* repeat frame, ignored */ - litest_event(dev, EV_KEY, KEY_A, 1); - litest_event(dev, EV_SYN, SYN_REPORT, 1); - litest_dispatch(li); - litest_assert_empty_queue(li); - - /* not a repeat frame */ + /* normal key press, not ignored */ litest_event(dev, EV_KEY, KEY_LEFTALT, 1); litest_event(dev, EV_SYN, SYN_REPORT, 0); litest_dispatch(li); litest_assert_key_event(li, KEY_LEFTALT, LIBINPUT_KEY_STATE_PRESSED); - /* normal repeat frame, ignored */ + /* normal key repeat, ignored */ litest_event(dev, EV_KEY, KEY_LEFTALT, 2); litest_event(dev, EV_SYN, SYN_REPORT, 1); litest_dispatch(li); litest_assert_empty_queue(li); - /* not repeat frame */ + /* not a repeat, not ignored */ litest_event(dev, EV_KEY, KEY_LEFTALT, 0); litest_event(dev, EV_SYN, SYN_REPORT, 0); litest_dispatch(li); litest_assert_key_event(li, KEY_LEFTALT, LIBINPUT_KEY_STATE_RELEASED); + litest_assert_empty_queue(li); - /* special alt+printscreen repeat frame, *not* ignored */ + /* special alt+printscreen frame, *not* ignored */ litest_event(dev, EV_KEY, KEY_LEFTALT, 1); litest_event(dev, EV_KEY, KEY_SYSRQ, 1); litest_event(dev, EV_SYN, SYN_REPORT, 1); litest_dispatch(li); - /* special alt+printscreen repeat frame, *not* ignored */ + /* special alt+printscreen frame, *not* ignored */ litest_event(dev, EV_KEY, KEY_LEFTALT, 0); litest_event(dev, EV_KEY, KEY_SYSRQ, 0); litest_event(dev, EV_SYN, SYN_REPORT, 1); litest_dispatch(li); - /* Note: the kernel doesn't release the key combo until both keys are released - * and the order is reshuffled so we have alt down, sysrq down, sysrq up, alt up + /* Note: The kernel first releases KEY_LEFTALT when pressing KEY_SYSRQ, + * then later generates press/release for KEY_LEFTALT + KEY_SYSRQ + * once *both* keys are released. The order is reshuffled so we have + * alt down, sysrq down, sysrq up, alt up. */ + litest_assert_key_event(li, KEY_LEFTALT, LIBINPUT_KEY_STATE_PRESSED); + litest_assert_key_event(li, KEY_LEFTALT, LIBINPUT_KEY_STATE_RELEASED); + litest_assert_key_event(li, KEY_LEFTALT, LIBINPUT_KEY_STATE_PRESSED); litest_assert_key_event(li, KEY_SYSRQ, LIBINPUT_KEY_STATE_PRESSED); litest_assert_key_event(li, KEY_SYSRQ, LIBINPUT_KEY_STATE_RELEASED); @@ -569,6 +569,52 @@ START_TEST(keyboard_keycode_obfuscation) } END_TEST +START_TEST(keyboard_nkey_rollover) +{ + struct litest_device *dev = litest_current_device(); + struct libinput *li = dev->libinput; + + int nkeys = litest_test_param_get_i32(test_env->params, "nkeys"); + + litest_drain_events(li); + + /* The kernel allocates a 7 + 1 buffer for devices without EV_ABS and + * EV_REL, see + * drivers/input/input.c:input_estimate_events_per_packet() + * + * If the device exceeds that buffer the current set is flushed out + * as SYN_REPORT 1 followed by (if any) the remaining events immediately + * after. + * + * Either way, we expect n keys to arrive, regardless what the kernel + * does. + */ + for (int i = 0; i < nkeys; i++) { + litest_event_unchecked(dev, EV_KEY, KEY_A + i, 1); + litest_event_unchecked(dev, EV_MSC, MSC_SCAN, 0x1000 + i); + } + litest_event_unchecked(dev, EV_SYN, SYN_REPORT, 0); + + for (int i = 0; i < nkeys; i++) { + litest_event_unchecked(dev, EV_KEY, KEY_A + i, 0); + litest_event_unchecked(dev, EV_MSC, MSC_SCAN, 0x1000 + i); + } + litest_event_unchecked(dev, EV_SYN, SYN_REPORT, 0); + + litest_dispatch(li); + + for (int i = 0; i < nkeys; i++) { + litest_checkpoint("here for %d", i); + litest_assert_key_event(li, KEY_A + i, LIBINPUT_KEY_STATE_PRESSED); + } + for (int i = 0; i < nkeys; i++) { + litest_assert_key_event(li, KEY_A + i, LIBINPUT_KEY_STATE_RELEASED); + } + + litest_assert_empty_queue(li); +} +END_TEST + TEST_COLLECTION(keyboard) { /* clang-format off */ @@ -588,5 +634,10 @@ TEST_COLLECTION(keyboard) litest_add_for_device(keyboard_alt_printscreen, LITEST_KEYBOARD); litest_add_for_device(keyboard_keycode_obfuscation, LITEST_KEYBOARD); + + /* Adding for one device only to be able to hardcode buffer sizes */ + litest_with_parameters(params, "nkeys", 'i', 4, 3, 4, 5, 6) + litest_add_parametrized_for_device(keyboard_nkey_rollover, LITEST_KEYBOARD, params); + /* clang-format on */ }