Add critsec to rp2040 xfer, check endpoint status

 - Implemented a critical section for the rp2040 port, which now
   prevents an IRQ from clearing the endpoint data structures while a
   transfer was in progress, which would then lead to a null pointer
   derefence.
 - Fixed a null-pointer dereference regarding ep->endpoint_control for
   endpoint 0, which does not have a control register.

Author: gemarcano

src/portable/raspberrypi/rp2040/dcd_rp2040.c

@@ -90,10 +90,12 @@ static void _hw_endpoint_alloc(struct hw_endpoint* ep, uint8_t transfer_type) {
   *ep->endpoint_control = reg;
 }
 
-static void _hw_endpoint_close(struct hw_endpoint* ep) {
+static void _hw_endpoint_close(struct hw_endpoint *ep) {
   // Clear hardware registers and then zero the struct
   // Clears endpoint enable
-  *ep->endpoint_control = 0;
+  if (ep->endpoint_control) {
+    *ep->endpoint_control = 0;
+  }
   // Clears buffer available, etc
   *ep->buffer_control = 0;
   // Clear any endpoint state
@@ -160,6 +162,7 @@ static void hw_endpoint_init(uint8_t ep_addr, uint16_t wMaxPacketSize, uint8_t t
     // alloc a buffer and fill in endpoint control register
     _hw_endpoint_alloc(ep, transfer_type);
   }
+  ep->configured = true;
 }
 
 static void hw_endpoint_xfer(uint8_t ep_addr, uint8_t* buffer, uint16_t total_bytes) {

src/portable/raspberrypi/rp2040/rp2040_usb.c

@@ -145,8 +145,8 @@ static uint32_t __tusb_irq_path_func(prepare_ep_buffer)(struct hw_endpoint* ep,
 }
 
 // Prepare buffer control register value
-void __tusb_irq_path_func(hw_endpoint_start_next_buffer)(struct hw_endpoint* ep) {
-  uint32_t ep_ctrl = *ep->endpoint_control;
+void __tusb_irq_path_func(hw_endpoint_start_next_buffer)(struct hw_endpoint *ep) {
+  uint32_t ep_ctrl = ep->endpoint_control ? *ep->endpoint_control : 0;
 
   // always compute and start with buffer 0
   uint32_t buf_ctrl = prepare_ep_buffer(ep, 0) | USB_BUF_CTRL_SEL;
@@ -175,7 +175,11 @@ void __tusb_irq_path_func(hw_endpoint_start_next_buffer)(struct hw_endpoint* ep)
     ep_ctrl |= EP_CTRL_INTERRUPT_PER_BUFFER;
   }
 
-  *ep->endpoint_control = ep_ctrl;
+  uint8_t epnum = tu_edpt_number(ep->ep_addr);
+  if (epnum != 0) // There's no endpoint control for endpoint 0
+  {
+      *ep->endpoint_control = ep_ctrl;
+  }
 
   TU_LOG(3, "  Prepare BufCtrl: [0] = 0x%04x  [1] = 0x%04x\r\n", tu_u32_low16(buf_ctrl), tu_u32_high16(buf_ctrl));
 
@@ -187,7 +191,12 @@ void __tusb_irq_path_func(hw_endpoint_start_next_buffer)(struct hw_endpoint* ep)
 void hw_endpoint_xfer_start(struct hw_endpoint* ep, uint8_t* buffer, uint16_t total_len) {
   hw_endpoint_lock_update(ep, 1);
 
-  if (ep->active) {
+  // We need to make sure the ep didn't get cleared from under us by an IRQ
+  if ( !ep->configured ) {
+    return;
+  }
+
+  if ( ep->active ) {
     // TODO: Is this acceptable for interrupt packets?
     TU_LOG(1, "WARN: starting new transfer on already active ep %02X\r\n", ep->ep_addr);
     hw_endpoint_reset_transfer(ep);
@@ -256,7 +265,7 @@ static void __tusb_irq_path_func(_hw_endpoint_xfer_sync)(struct hw_endpoint* ep)
   uint16_t buf0_bytes = sync_ep_buffer(ep, 0);
 
   // sync buffer 1 if double buffered
-  if ((*ep->endpoint_control) & EP_CTRL_DOUBLE_BUFFERED_BITS) {
+  if ( ep->endpoint_control && ((*ep->endpoint_control) & EP_CTRL_DOUBLE_BUFFERED_BITS) ) {
     if (buf0_bytes == ep->wMaxPacketSize) {
       // sync buffer 1 if not short packet
       sync_ep_buffer(ep, 1);

src/portable/raspberrypi/rp2040/rp2040_usb.h

@@ -8,6 +8,7 @@
 #include "common/tusb_common.h"
 
 #include "pico.h"
+#include "pico/critical_section.h"
 #include "hardware/structs/usb.h"
 #include "hardware/irq.h"
 #include "hardware/resets.h"
@@ -104,10 +105,20 @@ bool hw_endpoint_xfer_continue(struct hw_endpoint *ep);
 void hw_endpoint_reset_transfer(struct hw_endpoint *ep);
 void hw_endpoint_start_next_buffer(struct hw_endpoint *ep);
 
-TU_ATTR_ALWAYS_INLINE static inline void hw_endpoint_lock_update(__unused struct hw_endpoint * ep, __unused int delta) {
-  // todo add critsec as necessary to prevent issues between worker and IRQ...
-  //  note that this is perhaps as simple as disabling IRQs because it would make
-  //  sense to have worker and IRQ on same core, however I think using critsec is about equivalent.
+TU_ATTR_ALWAYS_INLINE static inline void hw_endpoint_lock_update(__unused struct hw_endpoint * ep, int delta) {
+  static critical_section_t hw_endpoint_crit_sec;
+  static int hw_endpoint_crit_sec_ref = 0;
+  if ( !critical_section_is_initialized(&hw_endpoint_crit_sec) ) {
+    critical_section_init(&hw_endpoint_crit_sec);
+  }
+
+  if ( delta > 0 && !hw_endpoint_crit_sec_ref ) {
+    critical_section_enter_blocking(&hw_endpoint_crit_sec);
+  }
+  hw_endpoint_crit_sec_ref += delta;
+  if ( hw_endpoint_crit_sec_ref == 0 ) {
+    critical_section_exit(&hw_endpoint_crit_sec);
+  }
 }
 
 void _hw_endpoint_buffer_control_update32(struct hw_endpoint *ep, uint32_t and_mask, uint32_t or_mask);