From af1f2bd4b292f45e6a0133c82fc0e299d1eb9930 Mon Sep 17 00:00:00 2001
From: RefactorFactory <108912458+RefactorFactory@users.noreply.github.com>
Date: Tue, 30 Aug 2022 14:05:41 -0700
Subject: [PATCH] Fix race condition with USBHID semaphore

The HID semaphore allows USBHID::SendReport() to wait for the completion of
report sending.

With a zero timeout xSemaphoreTake() after calling tud_hid_n_report(),
occasionally, the following would happening:

1. USBHID::SendReport() would send a report by calling tud_hid_n_report().
2. The send would complete and (presumably on another thread)
   tud_hid_report_complete_cb() would be called and it would xSemaphoreGive()
   the semaphore.
3. In USBHID::SendReport(), the zero timeout xSemaphoreTake(sem, 0) would
   succeed, taking the semaphore.
4. On the next line, xSemaphoreTake(sem, timeout_ms ...) would timeout
   because the semaphore was already taken by the previous line of code.

The result would be waiting timeout_ms for no reason.

The purpose of the zero timeout xSemaphoreTake() is to clear the semaphore in
case a previous SendReport() timed out waiting for the semaphore. In that case,
tud_hid_report_complete_cb() may be called after the timeout, giving the
semaphore. Then the next SendReport() would start with the semaphore given,
which isn't desired if we want to call xSemaphoreTake(sem, timeout_ms ...) on
it.

There have also been other cases where tud_hid_report_complete_cb() is called
an extra time, causing the same situation.

The fix is to move the zero timeout xSemaphoreTake() before the call to
tud_hid_n_report(). This eliminates the race between the zero timeout
xSemaphoreTake() and tud_hid_report_complete_cb() in the common case when no
timeout occurs.

There is still a possible race condition between the zero timeout
xSemaphoreTake() and tud_hid_report_complete_cb() in the case of a timeout,
but that should be rarer.
---
 libraries/USB/src/USBHID.cpp | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/libraries/USB/src/USBHID.cpp b/libraries/USB/src/USBHID.cpp
index a5bdb2d6739..7f2b36ea741 100644
--- a/libraries/USB/src/USBHID.cpp
+++ b/libraries/USB/src/USBHID.cpp
@@ -331,11 +331,16 @@ bool USBHID::SendReport(uint8_t id, const void* data, size_t len, uint32_t timeo
     if(!res){
         log_e("not ready");
     } else {
+        // The semaphore may be given if the last SendReport() timed out waiting for the report to
+        // be sent. Or, tud_hid_report_complete_cb() may be called an extra time, causing the
+        // semaphore to be given. In these cases, take the semaphore to clear its state so that
+        // we can wait for it to be given after calling tud_hid_n_report().
+        xSemaphoreTake(tinyusb_hid_device_input_sem, 0);
+
         res = tud_hid_n_report(0, id, data, len);
         if(!res){
             log_e("report %u failed", id);
         } else {
-            xSemaphoreTake(tinyusb_hid_device_input_sem, 0);
             if(xSemaphoreTake(tinyusb_hid_device_input_sem, timeout_ms / portTICK_PERIOD_MS) != pdTRUE){
                 log_e("report %u wait failed", id);
                 res = false;