From 48ae65865b025cd7f684acfd5bd62e735d408ba0 Mon Sep 17 00:00:00 2001
From: Gabriel Marcano <gabemarcano@yahoo.com>
Date: Thu, 15 Feb 2024 23:47:43 -0800
Subject: [PATCH] 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.
---
 src/portable/raspberrypi/rp2040/dcd_rp2040.c |  1 +
 src/portable/raspberrypi/rp2040/rp2040_usb.c |  6 ++++++
 src/portable/raspberrypi/rp2040/rp2040_usb.h | 21 +++++++++++++++-----
 3 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/src/portable/raspberrypi/rp2040/dcd_rp2040.c b/src/portable/raspberrypi/rp2040/dcd_rp2040.c
index 240e6c7273..fba9555051 100644
--- a/src/portable/raspberrypi/rp2040/dcd_rp2040.c
+++ b/src/portable/raspberrypi/rp2040/dcd_rp2040.c
@@ -106,6 +106,7 @@ static void hw_endpoint_init(hw_endpoint_t *ep, uint8_t ep_addr, uint16_t wMaxPa
     hard_assert(hw_buffer_ptr < usb_dpram->epx_data + sizeof(usb_dpram->epx_data));
     pico_info("  Allocated %d bytes (0x%p)\r\n", size, ep->hw_data_buf);
   }
+  ep->configured = true;
 }
 
 static void hw_endpoint_enable(hw_endpoint_t *ep, uint8_t transfer_type) {
diff --git a/src/portable/raspberrypi/rp2040/rp2040_usb.c b/src/portable/raspberrypi/rp2040/rp2040_usb.c
index 3b65f57a42..5d9833dbb3 100644
--- a/src/portable/raspberrypi/rp2040/rp2040_usb.c
+++ b/src/portable/raspberrypi/rp2040/rp2040_usb.c
@@ -229,6 +229,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, tu_fifo_t *ff, uint16_t total_len) {
   hw_endpoint_lock_update(ep, 1);
 
+  // We need to make sure the ep didn't get cleared from under us by an IRQ
+  if (!ep->configured) {
+    hw_endpoint_lock_update(ep, -1);
+    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);
diff --git a/src/portable/raspberrypi/rp2040/rp2040_usb.h b/src/portable/raspberrypi/rp2040/rp2040_usb.h
index c03dc34b23..b653d9a550 100644
--- a/src/portable/raspberrypi/rp2040/rp2040_usb.h
+++ b/src/portable/raspberrypi/rp2040/rp2040_usb.h
@@ -2,6 +2,7 @@
 #define RP2040_COMMON_H_
 
 #include "pico.h"
+#include "pico/critical_section.h"
 #include "hardware/structs/usb.h"
 #include "hardware/irq.h"
 #include "hardware/resets.h"
@@ -72,8 +73,8 @@ typedef struct hw_endpoint {
   uint8_t pending;     // Transfer scheduled but not active
 #endif
 
-#if CFG_TUH_ENABLED
   bool    configured;    // Is this a valid struct
+#if CFG_TUH_ENABLED
   uint8_t dev_addr;
   uint8_t interrupt_num; // for host interrupt endpoints
 #endif
@@ -107,10 +108,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);
+  }
 }
 
 // #if CFG_TUD_ENABLED