fix(hid): Eliminate data race in USB pathway causing dropped keys

[khoek]

As I found out diagnosing #2253, there is a data race in the USB HID core on many platforms; the call to hid_int_ep_write(), a Zephyr function, doesn't always copy the passed buffer before beginning the USB transaction. I've only read a few of the HAL drivers, but e.g. nrfx and stm32 don't, while rpi_pico, native_posix, and kinetis do.

So the USB keycode report is changing underneath the HAL driver while, for example, a tapped &lt quickly activate-deactivates the key. I originally thought this had something to do with extended NKRO, but actually noone ever noticed for smaller keycode bits just because they were first and thus sent more quickly!

Amazing to me that this hadn't come up before! I guess there are many more bluetooth users, where it seems in practice that there is no such problem (though I haven't checked the code).

Tested working on a real Adv360.

Closes #2253

[khoek]

An alternative here is define the buffer size to be a certain fixed constant and then add compile time checks that the buffer is large enough to handle any possible response.

[petejohanson]

Good catch! I am on the fence on your fix. In practice, the semaphore gating should properly prevent the issue with the buffer changing out from under the USB device driver, but I also share the concerns about the buffer sizing macros being... painful, but perhaps necessary.

Any chance you found any upstream guidance on whether the "copy early" or not approach in the drivers is expected? Accidental? Suggested "best practice" for handling?

[khoek]

@petejohanson Thanks so much for reviewing this so quickly, given how much activity the project has!

I'm likely saying something you totally already understand, but while the semaphore completely prevents another call to hid_int_ep_write() while a write is in progress, i.e. until Zephyr invokes the in_ready_cb() callback to tell us it's safe to call hid_int_ep_write() again, changes to the underlying buffer during the write are still not protected against. (i.e. the passed buffer pointer is literally the live bitmask of actively pressed keys, as the keyboard processes state changes the buffer will immediately update, and this is the cause of the problem.)

That said, deferring to your confidence that the current implementation is supposed to work, I just had a look at all of the Zephyr samples/ which invoke hid_int_ep_write(), and all but one have the same race condition (the exception not having it because it is too simple). So I now agree that this a bug in Zephyr.

The question is whether it is a bug in hid_int_ep_write() or usb_write(); since usb_write() is described as part of the "Low level API" in the documentation (https://docs.zephyrproject.org/latest/connectivity/usb/device/api/usb_device.html) and the HID code wraps things to make them considerably simpler and is more stateful, I'm inclined to believe it is hid_int_ep_write()'s fault. I'll to make a PR to Zephyr right away to see what the maintainers think.

In the meantime, since this bug means that really wired-USB key reports are getting through by luck, would you at all consider nonetheless implementing a fix along the lines described here temporarily while we depend on a buggy version of Zephyr?

An alternative less-intrusive workaround is that we could arrange that for now the semaphore just blocks the entire function zmk_usb_hid_send_report() until the send is complete, roughly speaking just by moving the semaphore. I don't think there would really be a latency penalty in practice since successive fast calls to zmk_usb_hid_send_report() (e.g. that caused by &lt here) still currently block anyway, just due to waiting on the hid_int_ep_write() callback.

Another option which solves the problem and also potentially improves the current implementation would be for usb_hid.c to maintain a message queue which it processes at its own pace. But this would be a bit more complex and a few more lines of code compared to e.g. the handful of the previous suggestion. And you would know better than I whether the additional complexity was totally unnecessary (there would certainly be a latency improvement, but would this mean much given the speed of current MCUs?).

Please let me know what you think. I'm just sad my F24 key doesn't work! :)

[khoek]

FYI, after trying to carefully read all of the Zephyr USB drivers I believe the problem is localized to the nrfx family and have filed zephyrproject-rtos/zephyr#71306.

[khoek]

It seems that the current view is that Zephyr is unable to make the guarantee that hid_int_ep_write() copies, and that the examples should be changed to reflect that. In the background a new USB device stack API is being developed (including updated HID API) for the future.

So it appears to me that something along the lines of this PR will be necessary for the time being, at least for nrfx boards.