evdev: move the SYN_REPORT 1 filtering to the touchpad backend

In commit 9a9466b6a9 ("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
7140f13d82 ("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: 7140f13d82 ("evdev: track KEY_SYSRQ frames and pass them even as repeat frames")
Reverts: 9a9466b6a9 ("evdev: discard any frame with EV_SYN SYN_REPORT 1")

Closes #1165

Part-of: <https://gitlab.freedesktop.org/libinput/libinput/-/merge_requests/1282>

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);
 	}

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
+				evdev_device_dispatch_frame(libinput, device, frame);
-				 * 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_frame_reset(frame);
-				had_sysrq = false;
 			}
 		} else if (rc == -ENODEV) {
 			evdev_device_remove(device);

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 */

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)
 {

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,

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 */
 }

test/test-keyboard.c

@@ -475,45 +475,45 @@ START_TEST(keyboard_alt_printscreen)
 
 	litest_drain_events(li);
 
-	/* repeat frame, ignored */
+	/* normal key press, not 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 */
 	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
+	/* Note: The kernel first releases KEY_LEFTALT when pressing KEY_SYSRQ,
-	 * and the order is reshuffled so we have alt down, sysrq down, sysrq up, alt up
+	 * 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 */
 }