Assert triggered by CDC upon rapid connect/disconnect cycle

[michaelajax]

I am developing an application that operates as a composite device with both UAC 2.0 and CDC support, running on an STM32G474RET6 MCU.

During some stress testing where I had audio playing on the device and the serial port open, then start cycling unplug/replug and connect/disconnect to the CDC (audio automatically switches back to the device as soon as the endpoint enumerates on Windows), the board occasionally asserts here in usbd.c:

// Attempt to transfer on a busy endpoint, sound like an race condition !
TU_ASSERT((_usbd_dev.ep_status[epnum][dir] & TU_EDPT_STATE_BUSY) == 0);

This happens after plugging back in and attempting to open the serial port - the last thing in my log before the assert was receiving a GET_LINE_CODING from the CDC host. However, I have a hard time describing exactly what conditions led up to this, because it happens on rapid combinations of sending data, receiving audio, connect/disconnect, or hot-plugging. Meaning I can't reliably reproduce this failure to understand the specific conditions leading to it.

But if I make two changes in usbd.c, I can no longer repo the crash after many attempts.

In status_stage_xact, check whether the EP0 direction selected for the status stage already has an active transfer marked busy. If so, return false without submitting another transfer:

TU_ATTR_ALWAYS_INLINE static inline bool status_stage_xact(uint8_t rhport, uint8_t ep_status) {
+ TU_VERIFY(!usbd_edpt_busy(rhport, ep_status));
  return usbd_edpt_xfer(rhport, ep_status, NULL, 0, false);
}

Then in data_stage_xact, determine whether the data stage uses EP0 IN or EP0 OUT, then check whether that direction already has an active transfer marked busy. If so, return false before modifying the shared EP0 buffer or submitting another transfer:

// Queue a transaction in Data Stage. Each transaction has up to Endpoint0's max
// packet size. This function can also transfer a zero-length packet.
static bool data_stage_xact(uint8_t rhport) {
  usbd_control_xfer_t* const ctrl_xfer = &_usbd_dev.ctrl_xfer;
  const uint16_t xact_len = tu_min16(ctrl_xfer->data_len - ctrl_xfer->total_xferred, CFG_TUD_ENDPOINT0_BUFSIZE);
-  uint8_t ep_addr = TU_EP0_OUT;
+  const uint8_t ep_addr = ctrl_xfer->request.bmRequestType_bit.direction == TUSB_DIR_IN ? TU_EP0_IN : TU_EP0_OUT;
+  TU_VERIFY(!usbd_edpt_busy(rhport, ep_addr));

  if (ctrl_xfer->request.bmRequestType_bit.direction == TUSB_DIR_IN) {
-    ep_addr = TU_EP0_IN;
    if (0u != xact_len && ctrl_xfer->buffer != _ctrl_epbuf.buf) {
      TU_VERIFY(0 == tu_memcpy_s(_ctrl_epbuf.buf, CFG_TUD_ENDPOINT0_BUFSIZE, ctrl_xfer->buffer, xact_len));
    }
  }

  return usbd_edpt_xfer(rhport, ep_addr, xact_len ? _ctrl_epbuf.buf : NULL, xact_len, false);
}

While this works and I can't seem to get my application to fail anymore under these stress test conditions, I'm not familiar enough with what exactly the implications are of just dropping old transfers, to understand the risks of making these changes. I'm curious if anyone has any thoughts about this type of issue and this part of the code. Thanks!