Merge pull request #3127 from hathach/fix/dcd_race_condition

add osal spinlock API, Fix/dcd dwc2 race condition 

2 files changed, 53 insertions, 22 deletions

diff --git a/src/portable/synopsys/dwc2/dcd_dwc2.c b/src/portable/synopsys/dwc2/dcd_dwc2.c
index 52d675611..865c51894 100644
--- a/src/portable/synopsys/dwc2/dcd_dwc2.c
+++ b/src/portable/synopsys/dwc2/dcd_dwc2.c
@@ -39,6 +39,7 @@
 #define DWC2_DEBUG    2
 
 #include "device/dcd.h"
+#include "device/usbd_pvt.h"
 #include "dwc2_common.h"
 
 //--------------------------------------------------------------------+
@@ -52,6 +53,7 @@ typedef struct {
   uint8_t interval;
 } xfer_ctl_t;
 
+// This variable is modified from ISR context, so it must be protected by critical section
 static xfer_ctl_t xfer_status[DWC2_EP_MAX][2];
 #define XFER_CTL_BASE(_ep, _dir) (&xfer_status[_ep][_dir])
 
@@ -321,6 +323,9 @@ static void edpt_disable(uint8_t rhport, uint8_t ep_addr, bool stall) {
   }
 }
 
+// Since this function returns void, it is not possible to return a boolean success message
+// We must make sure that this function is not called when the EP is disabled
+// Must be called from critical section
 static void edpt_schedule_packets(uint8_t rhport, const uint8_t epnum, const uint8_t dir) {
   dwc2_regs_t* dwc2 = DWC2_REG(rhport);
   xfer_ctl_t* const xfer = XFER_CTL_BASE(epnum, dir);
@@ -531,6 +536,8 @@ void dcd_edpt_close_all(uint8_t rhport) {
   dwc2_regs_t* dwc2 = DWC2_REG(rhport);
   uint8_t const ep_count = _dwc2_controller[rhport].ep_count;
 
+  usbd_spin_lock(false);
+
   _dcd_data.allocated_epin_count = 0;
 
   // Disable non-control interrupt
@@ -548,8 +555,9 @@ void dcd_edpt_close_all(uint8_t rhport) {
 
   dfifo_flush_tx(dwc2, 0x10); // all tx fifo
   dfifo_flush_rx(dwc2);
-
   dfifo_device_init(rhport); // re-init dfifo
+
+  usbd_spin_unlock(false);
 }
 
 bool dcd_edpt_iso_alloc(uint8_t rhport, uint8_t ep_addr, uint16_t largest_packet_size) {
@@ -567,21 +575,31 @@ bool dcd_edpt_iso_activate(uint8_t rhport,  tusb_desc_endpoint_t const * p_endpo
 bool dcd_edpt_xfer(uint8_t rhport, uint8_t ep_addr, uint8_t* buffer, uint16_t total_bytes) {
   uint8_t const epnum = tu_edpt_number(ep_addr);
   uint8_t const dir = tu_edpt_dir(ep_addr);
-
   xfer_ctl_t* xfer = XFER_CTL_BASE(epnum, dir);
-  xfer->buffer = buffer;
-  xfer->ff = NULL;
-  xfer->total_len = total_bytes;
+  bool ret;
 
-  // EP0 can only handle one packet
-  if (epnum == 0) {
-    _dcd_data.ep0_pending[dir] = total_bytes;
+  usbd_spin_lock(false);
+
+  if (xfer->max_size == 0) {
+    ret = false;  // Endpoint is closed
+  } else {
+    xfer->buffer = buffer;
+    xfer->ff = NULL;
+    xfer->total_len = total_bytes;
+
+    // EP0 can only handle one packet
+    if (epnum == 0) {
+      _dcd_data.ep0_pending[dir] = total_bytes;
+    }
+
+    // Schedule packets to be sent within interrupt
+    edpt_schedule_packets(rhport, epnum, dir);
+    ret = true;
   }
 
-  // Schedule packets to be sent within interrupt
-  edpt_schedule_packets(rhport, epnum, dir);
+  usbd_spin_unlock(false);
 
-  return true;
+  return ret;
 }
 
 // The number of bytes has to be given explicitly to allow more flexible control of how many
@@ -594,17 +612,27 @@ bool dcd_edpt_xfer_fifo(uint8_t rhport, uint8_t ep_addr, tu_fifo_t* ff, uint16_t
 
   uint8_t const epnum = tu_edpt_number(ep_addr);
   uint8_t const dir = tu_edpt_dir(ep_addr);
-
   xfer_ctl_t* xfer = XFER_CTL_BASE(epnum, dir);
-  xfer->buffer = NULL;
-  xfer->ff = ff;
-  xfer->total_len = total_bytes;
+  bool ret;
 
-  // Schedule packets to be sent within interrupt
-  // TODO xfer fifo may only available for slave mode
-  edpt_schedule_packets(rhport, epnum, dir);
+  usbd_spin_lock(false);
 
-  return true;
+  if (xfer->max_size == 0) {
+    ret = false;  // Endpoint is closed
+  } else {
+    xfer->buffer = NULL;
+    xfer->ff = ff;
+    xfer->total_len = total_bytes;
+
+    // Schedule packets to be sent within interrupt
+    // TODO xfer fifo may only available for slave mode
+    edpt_schedule_packets(rhport, epnum, dir);
+    ret = true;
+  }
+
+  usbd_spin_unlock(false);
+
+  return ret;
 }
 
 void dcd_edpt_stall(uint8_t rhport, uint8_t ep_addr) {
@@ -631,6 +659,7 @@ void dcd_edpt_clear_stall(uint8_t rhport, uint8_t ep_addr) {
 //--------------------------------------------------------------------
 
 // 7.4.1 Initialization on USB Reset
+// Must be called from critical section
 static void handle_bus_reset(uint8_t rhport) {
   dwc2_regs_t *dwc2 = DWC2_REG(rhport);
   const uint8_t ep_count =  dwc2_ep_count(dwc2);
@@ -983,14 +1012,16 @@ static void handle_ep_irq(uint8_t rhport, uint8_t dir) {
  */
 void dcd_int_handler(uint8_t rhport) {
   dwc2_regs_t* dwc2 = DWC2_REG(rhport);
-
   const uint32_t gintmask = dwc2->gintmsk;
   const uint32_t gintsts = dwc2->gintsts & gintmask;
 
   if (gintsts & GINTSTS_USBRST) {
     // USBRST is start of reset.
     dwc2->gintsts = GINTSTS_USBRST;
+
+    usbd_spin_lock(true);
     handle_bus_reset(rhport);
+    usbd_spin_unlock(true);
   }
 
   if (gintsts & GINTSTS_ENUMDNE) {
diff --git a/src/portable/synopsys/dwc2/dwc2_esp32.h b/src/portable/synopsys/dwc2/dwc2_esp32.h
index 3309760ff..49b8c54cb 100644
--- a/src/portable/synopsys/dwc2/dwc2_esp32.h
+++ b/src/portable/synopsys/dwc2/dwc2_esp32.h
@@ -55,8 +55,8 @@ static const dwc2_controller_t _dwc2_controller[] = {
 // On ESP32 for consistency we associate
 // - Port0 to OTG_FS, and Port1 to OTG_HS
 static const dwc2_controller_t _dwc2_controller[] = {
-{ .reg_base = DWC2_FS_REG_BASE, .irqnum = ETS_USB_OTG11_CH0_INTR_SOURCE, .ep_count = 7, .ep_in_count = 5, .ep_fifo_size = 1024 },
-{ .reg_base = DWC2_HS_REG_BASE, .irqnum = ETS_USB_OTG_INTR_SOURCE, .ep_count = 16, .ep_in_count = 8, .ep_fifo_size = 4096 }
+  { .reg_base = DWC2_FS_REG_BASE, .irqnum = ETS_USB_OTG11_CH0_INTR_SOURCE, .ep_count = 7, .ep_in_count = 5, .ep_fifo_size = 1024 },
+  { .reg_base = DWC2_HS_REG_BASE, .irqnum = ETS_USB_OTG_INTR_SOURCE, .ep_count = 16, .ep_in_count = 8, .ep_fifo_size = 4096 }
 };
 #endif