Skip to content

Commit ccf0d87

Browse files
Fix race condition with USBHID semaphore (#7205)
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.
1 parent b473fc6 commit ccf0d87

File tree

1 file changed

+6
-1
lines changed

1 file changed

+6
-1
lines changed

Diff for: libraries/USB/src/USBHID.cpp

+6-1
Original file line numberDiff line numberDiff line change
@@ -345,11 +345,16 @@ bool USBHID::SendReport(uint8_t id, const void* data, size_t len, uint32_t timeo
345345
if(!res){
346346
log_e("not ready");
347347
} else {
348+
// The semaphore may be given if the last SendReport() timed out waiting for the report to
349+
// be sent. Or, tud_hid_report_complete_cb() may be called an extra time, causing the
350+
// semaphore to be given. In these cases, take the semaphore to clear its state so that
351+
// we can wait for it to be given after calling tud_hid_n_report().
352+
xSemaphoreTake(tinyusb_hid_device_input_sem, 0);
353+
348354
res = tud_hid_n_report(0, id, data, len);
349355
if(!res){
350356
log_e("report %u failed", id);
351357
} else {
352-
xSemaphoreTake(tinyusb_hid_device_input_sem, 0);
353358
if(xSemaphoreTake(tinyusb_hid_device_input_sem, timeout_ms / portTICK_PERIOD_MS) != pdTRUE){
354359
log_e("report %u wait failed", id);
355360
res = false;

0 commit comments

Comments
 (0)