[Bug] Keyboard freezes with Mod-Tap or ¿pressing two keys simultaneously?

Describe the Bug

I have MT(MOD_RALT, KC_APP) key in my layout and when I press and hold it and then then quickly press another key, the keyboard freezes. The same happens when I use other Mod-Tap keys or, occasionally, even when I type fast.

Key getting stuck after mashing

I have bisected the issue to 2078a56369af376e3275f02e21d48ab6cc39bc36 being the first bad commit.

cc @fauxpark who created https://github.com/qmk/qmk_firmware/pull/18631.

Keyboard Used

kinesis/kint41 with keymap jtojnar (https://github.com/qmk/qmk_firmware/pull/18802)

Operating System

NixOS Linux

qmk doctor Output

Details

```text Ψ QMK Doctor is checking your environment. Ψ CLI version: 1.1.1 Ψ QMK home: /home/jtojnar/Projects/qmk_firmware Ψ Detected Linux. ⚠ Missing or outdated udev rules for 'atmel-dfu' boards. Run 'sudo cp /home/jtojnar/Projects/qmk_firmware/util/udev/50-qmk.rules /etc/udev/rules.d/'. ⚠ Missing or outdated udev rules for 'kiibohd' boards. Run 'sudo cp /home/jtojnar/Projects/qmk_firmware/util/udev/50-qmk.rules /etc/udev/rules.d/'. ⚠ Missing or outdated udev rules for 'stm32-dfu' boards. Run 'sudo cp /home/jtojnar/Projects/qmk_firmware/util/udev/50-qmk.rules /etc/udev/rules.d/'. ⚠ Missing or outdated udev rules for 'apm32-dfu' boards. Run 'sudo cp /home/jtojnar/Projects/qmk_firmware/util/udev/50-qmk.rules /etc/udev/rules.d/'. ⚠ Missing or outdated udev rules for 'gd32v-dfu' boards. Run 'sudo cp /home/jtojnar/Projects/qmk_firmware/util/udev/50-qmk.rules /etc/udev/rules.d/'. ⚠ Missing or outdated udev rules for 'bootloadhid' boards. Run 'sudo cp /home/jtojnar/Projects/qmk_firmware/util/udev/50-qmk.rules /etc/udev/rules.d/'. ⚠ Missing or outdated udev rules for 'usbasploader' boards. Run 'sudo cp /home/jtojnar/Projects/qmk_firmware/util/udev/50-qmk.rules /etc/udev/rules.d/'. ⚠ Missing or outdated udev rules for 'usbtinyisp' boards. Run 'sudo cp /home/jtojnar/Projects/qmk_firmware/util/udev/50-qmk.rules /etc/udev/rules.d/'. ⚠ Missing or outdated udev rules for 'md-boot' boards. Run 'sudo cp /home/jtojnar/Projects/qmk_firmware/util/udev/50-qmk.rules /etc/udev/rules.d/'. ⚠ Detected ModemManager without the necessary udev rules. Please either disable it or set the appropriate udev rules if you are using a Pro Micro. ⚠ Missing or outdated udev rules for 'caterina' boards. Run 'sudo cp /home/jtojnar/Projects/qmk_firmware/util/udev/50-qmk.rules /etc/udev/rules.d/'. ⚠ Missing or outdated udev rules for 'hid-bootloader' boards. Run 'sudo cp /home/jtojnar/Projects/qmk_firmware/util/udev/50-qmk.rules /etc/udev/rules.d/'. Ψ Git branch: HEAD Ψ Repo version: 0.18.13 ⚠ Git has unstashed/uncommitted changes. Ψ - Latest HEAD: 2022-10-13 10:28:49 +1100 (2078a56369) -- Fix joystick functionality for ChibiOS and OTG (Blackpill) (#18631) Ψ - Latest upstream/master: 2022-10-21 12:02:11 +0700 (bd044ae5ff) -- Update Black E6.5 keymap issues on QMK Configurator (#18794) Ψ - Latest upstream/develop: 2022-10-21 05:02:49 +0000 (f97ef22873) -- Merge remote-tracking branch 'origin/master' into develop Ψ - Common ancestor with upstream/master: 2022-10-12 16:17:35 -0700 (a195f78200) -- Vertex ARC60 Layout Refactor (#18670) Ψ - Common ancestor with upstream/develop: 2022-10-13 10:28:49 +1100 (2078a56369) -- Fix joystick functionality for ChibiOS and OTG (Blackpill) (#18631) Ψ All dependencies are installed. Ψ Found arm-none-eabi-gcc version 10.3.1 Ψ Found avr-gcc version 8.5.0 Ψ Found avrdude version 7.0 Ψ Found dfu-util version 0.11 Ψ Found dfu-programmer version 0.7.2 Ψ Submodules are up to date. Ψ Submodule status: Ψ - lib/chibios: 2022-09-18 10:01:17 +0000 -- (0e9d558b5) Ψ - lib/chibios-contrib: 2022-10-03 18:09:41 +0200 -- (bb8356fb) Ψ - lib/googletest: 2021-06-11 06:37:43 -0700 -- (e2239ee6) Ψ - lib/lufa: 2022-08-26 12:09:55 +1000 -- (549b97320) Ψ - lib/pico-sdk: 2022-09-19 18:02:44 +0200 -- (8d56ea3) Ψ - lib/printf: 2022-06-29 23:59:58 +0300 -- (c2e3b4e) Ψ - lib/vusb: 2022-06-13 09:18:17 +1000 -- (819dbc1) Ψ QMK is ready to go, but minor problems were found ```

Additional Context

Looking at the USB traffic in Wireshark, normal key presses give me two USB packets containing HID data such as 00 00 04 00 00 00 00 00 (for letter A), followed by two packets with HID data 00 00 00 00 00 00 00 00.

Pressing the Alt Gr key plus another only ever gives me the first two “press” packets, with the “release“ packets never arriving, leading the computer to think the key continues to be pressed.

This looks like a bug in the USB LLD for MIMXRT1062 — it contains this code (repeated twice):

  } else if (direction == USB_IN) {
    printf_debug("    complete, IN ep=%d (IN), txbuf=%x", ep, epc->in_state->txbuf);
    (usbp)->transmitting &= ~(1 << ep);
    /* Endpoint Transmit Complete Event */
    /* Transfer Direction IN */
    if (epc->in_cb != NULL) {
      printf_debug("    invoking in_cb for ep %d", ep);
      _usb_isr_invoke_in_cb(usbp, ep);
    }
  }

So the LLD code skips the _usb_isr_invoke_in_cb(usbp, ep) call if epc->in_cb is NULL. However, the implementation of _usb_isr_invoke_in_cb() in the ChibiOS HAL is like this — in the USB_USE_WAIT case it also resumes the waiting thread in addition to invoking the callback:

#if (USB_USE_WAIT == TRUE) || defined(__DOXYGEN__)
#define _usb_isr_invoke_in_cb(usbp, ep) {                                   \
  (usbp)->transmitting &= ~(1 << (ep));                                     \
  if ((usbp)->epc[ep]->in_cb != NULL) {                                     \
    (usbp)->epc[ep]->in_cb(usbp, ep);                                       \
  }                                                                         \
  osalSysLockFromISR();                                                     \
  osalThreadResumeI(&(usbp)->epc[ep]->in_state->thread, MSG_OK);            \
  osalSysUnlockFromISR();                                                   \
}
#else
#define _usb_isr_invoke_in_cb(usbp, ep) {                                   \
  (usbp)->transmitting &= ~(1 << (ep));                                     \
  if ((usbp)->epc[ep]->in_cb != NULL) {                                     \
    (usbp)->epc[ep]->in_cb(usbp, ep);                                       \
  }                                                                         \
}
#endif

And the QMK code enables the USB_USE_WAIT config option and depends on it, e.g., here (there are several pieces of code like this dealing with different endpoints):

        /* need to wait until the previous packet has made it through */
        /* busy wait, should be short and not very common */
        if (usbGetTransmitStatusI(&USB_DRIVER, KEYBOARD_IN_EPNUM)) {
            /* Need to either suspend, or loop and call unlock/lock during
             * every iteration - otherwise the system will remain locked,
             * no interrupts served, so USB not going through as well.
             * Note: for suspend, need USB_USE_WAIT == TRUE in halconf.h */
            osalThreadSuspendS(&(&USB_DRIVER)->epc[KEYBOARD_IN_EPNUM]->in_state->thread);

            /* after osalThreadSuspendS returns USB status might have changed */
            if (usbGetDriverStateI(&USB_DRIVER) != USB_ACTIVE) {
                goto unlock;
            }
        }

That waiting code is triggered only when the keyboard tries to send another report before the previous one has been received by the host, so the bug might not manifest when typing normally using regular keys, but things like Mod-Tap or maybe even keycodes with modifiers would trigger it.

Looks like the same bug is also present in the KINETIS USB LLD (so the older Arm-based Teensy boards would be affected too). And the calls to _usb_isr_invoke_out_cb() also have the same broken condition there (but this part does not affect QMK, because the existing code does not use blocking waits on OUT endpoints, using the callbacks instead).

As a workaround until the broken LLDs get fixed it should be possible to partially revert the changes from #18631 (restore the IN notification callbacks that do nothing).

qmk / qmk_firmware