HID output report race condition

[shuffle2]

Operating System

Others

Board

raspberry pi pico W

Firmware

using pico-sdk 1.5.1

What happened ?

I'm writing firmware for the pico which talks to pre-existing software on a usb host (read: I cannot change or easily inspect the host software/stack).

The host regularly sends HID output reports to the pico. At the same time, it also sends SET_ and GET_ FEATURE_REPORT reports. What I observe is that occasionally while I am processing a SET_FEATURE, the underlying buffer gets overwritten by one of the output reports the host is periodically sending. (And so, the report_id no longer corresponds to the buffer contents).

I note that the callers of tud_hid_set_report_cb use _hidd_itf[0]->epout_buf in both cases (output reports and HID_REQ_CONTROL_SET_REPORT). Am I missing something here (like required locking, time bound requirements, etc)? Is there a way to use a different buffer for each of output reports and SET_REPORTs ?

I've implemented a simple patch which, amazingly, seems to fix the problem I'm encountering:

diff --git a/src/class/hid/hid_device.c b/src/class/hid/hid_device.c
index 2d46d760f..c939c0117 100644
--- a/src/class/hid/hid_device.c
+++ b/src/class/hid/hid_device.c
@@ -52,6 +52,7 @@ typedef struct

   CFG_TUSB_MEM_ALIGN uint8_t epin_buf[CFG_TUD_HID_EP_BUFSIZE];
   CFG_TUSB_MEM_ALIGN uint8_t epout_buf[CFG_TUD_HID_EP_BUFSIZE];
+  CFG_TUSB_MEM_ALIGN uint8_t set_report_buf[CFG_TUD_HID_EP_BUFSIZE];

   // TODO save hid descriptor since host can specifically request this after enumeration
   // Note: HID descriptor may be not available from application after enumeration
@@ -307,15 +308,15 @@ bool hidd_control_xfer_cb (uint8_t rhport, uint8_t stage, tusb_control_request_t
       case  HID_REQ_CONTROL_SET_REPORT:
         if ( stage == CONTROL_STAGE_SETUP )
         {
-          TU_VERIFY(request->wLength <= sizeof(p_hid->epout_buf));
-          tud_control_xfer(rhport, request, p_hid->epout_buf, request->wLength);
+          TU_VERIFY(request->wLength <= sizeof(p_hid->set_report_buf));
+          tud_control_xfer(rhport, request, p_hid->set_report_buf, request->wLength);
         }
         else if ( stage == CONTROL_STAGE_ACK )
         {
           uint8_t const report_type = tu_u16_high(request->wValue);
           uint8_t const report_id   = tu_u16_low(request->wValue);

-          uint8_t const* report_buf = p_hid->epout_buf;
+          uint8_t const* report_buf = p_hid->set_report_buf;
           uint16_t report_len = tu_min16(request->wLength, CFG_TUD_HID_EP_BUFSIZE);

           // If host request a specific Report ID, extract report ID in buffer before invoking callback

I'm not sure if this is a good solution. Any comments / suggestions?
Should a similar thing be done for HID_REQ_CONTROL_GET_REPORT ?

How to reproduce ?

I unfortunately don't have good repro steps.

Debug Log as txt file (LOG/CFG_TUSB_DEBUG=2)

Example of the set_report, sandwiched between two output reports. It appears the second output report has overwritten contents of the buffer between first and second time it is parsed (the SET_FEATURE_REPORT_F0(8d, d7, 0): crc mismatch line prints out the first 3 bytes of the buffer, which are not the expected values of `01, 01, 001)

USBD Setup Received 21 09 F0 03 00 00 40 00 
  HID control request
  Queue EP 00 with 64 bytes ...
USBD Xfer Complete on EP 03 with 48 bytes
  HID xfer callback
output report
02 8d d7 00 00 00 00 00 00 00 80 05 00 00 00 00
00 00 00 00 00 00 05 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00 04 03 00 00 01 00 00 00 00 ff
  Queue EP 03 with 64 bytes ...
USBD Xfer Complete on EP 00 with 64 bytes
  0000:  F0 01 01 00 01 00 00 00 F7 14 F9 0D 7F F4 96 54  |...............T|
  0010:  D0 38 21 28 22 6B 13 A8 10 00 01 00 00 00 00 00  |.8!("k..........|
  0020:  00 00 00 00 00 00 00 00 30 30 30 31 30 30 30 31  |........00010001|
  0030:  30 30 30 30 30 30 30 30 BB 3D 50 84 F2 3C 04 59  |00000000.=P..<.Y|
  HID control complete
  Queue EP 80 with 0 bytes ...
  Queue EP 84 with 64 bytes ...
USBD Xfer Complete on EP 84 with 64 bytes
  HID xfer callback
USBD Xfer Complete on EP 80 with 0 bytes
set_feature_report(f0, 20002EF9,   3f)
01 01 00 01 00 00 00 f7 14 f9 0d 7f f4 96 54 d0
38 21 28 22 6b 13 a8 10 00 01 00 00 00 00 00 00
00 00 00 00 00 04 03 00 00 01 00 00 00 00 ff 30
30 30 30 30 30 30 30 bb 3d 50 84 f2 3c 04 59
SET_FEATURE_REPORT_F0(8d, d7,  0): crc mismatch
USBD Xfer Complete on EP 03 with 48 bytes
  HID xfer callback
output report
02 8d d7 00 00 00 00 00 00 00 80 05 00 00 00 00
00 00 00 00 00 00 05 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00 04 03 00 00 01 00 00 00 00 ff
  Queue EP 03 with 64 bytes ...
  Queue EP 84 with 64 bytes ...
USBD Xfer Complete on EP 84 with 64 bytes
  HID xfer callback

Screenshots

No response

I have checked existing issues, dicussion and documentation

• I confirm I have checked existing issues, dicussion and documentation.

[sadykowich]

Thanks @shuffle2, it was the solution in my case also.