From c8baba10f9a7a6150348618aec74719522887c93 Mon Sep 17 00:00:00 2001 From: Tomas Rezucha Date: Wed, 9 Apr 2025 10:52:44 +0200 Subject: [PATCH] fix(dcd): Fixed race condition on device disconnect TinyUSB does not provide any locking means to protect the DCD variables. This can lead to race conditions when the user is trying to submit a transfer while the device is being disconnected. This can cause the device to be in an inconsistent state, leading to a crash or undefined behavior. This commit adds a spin-lock to protect the DCD variables during device disconnect. Closes https://github.com/espressif/esp-idf/issues/9691 Also reported in https://github.com/espressif/esp-usb/pull/131 --- src/portable/synopsys/dwc2/dcd_dwc2.c | 24 +++++++++++++++++++++ src/portable/synopsys/dwc2/dwc2_critical.h | 25 ++++++++++++++++++++++ 2 files changed, 49 insertions(+) create mode 100644 src/portable/synopsys/dwc2/dwc2_critical.h diff --git a/src/portable/synopsys/dwc2/dcd_dwc2.c b/src/portable/synopsys/dwc2/dcd_dwc2.c index 52d675611..23273cb78 100644 --- a/src/portable/synopsys/dwc2/dcd_dwc2.c +++ b/src/portable/synopsys/dwc2/dcd_dwc2.c @@ -40,6 +40,7 @@ #include "device/dcd.h" #include "dwc2_common.h" +#include "dwc2_critical.h" //--------------------------------------------------------------------+ // MACRO TYPEDEF CONSTANT ENUM @@ -52,6 +53,9 @@ 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 +325,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 +538,7 @@ 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; + DCD_ENTER_CRITICAL(); _dcd_data.allocated_epin_count = 0; // Disable non-control interrupt @@ -550,6 +558,7 @@ void dcd_edpt_close_all(uint8_t rhport) { dfifo_flush_rx(dwc2); dfifo_device_init(rhport); // re-init dfifo + DCD_EXIT_CRITICAL(); } bool dcd_edpt_iso_alloc(uint8_t rhport, uint8_t ep_addr, uint16_t largest_packet_size) { @@ -568,7 +577,12 @@ bool dcd_edpt_xfer(uint8_t rhport, uint8_t ep_addr, uint8_t* buffer, uint16_t to uint8_t const epnum = tu_edpt_number(ep_addr); uint8_t const dir = tu_edpt_dir(ep_addr); + DCD_ENTER_CRITICAL(); xfer_ctl_t* xfer = XFER_CTL_BASE(epnum, dir); + if (xfer->max_size == 0) { + DCD_EXIT_CRITICAL(); + return false; // Endpoint is closed + } xfer->buffer = buffer; xfer->ff = NULL; xfer->total_len = total_bytes; @@ -580,6 +594,7 @@ bool dcd_edpt_xfer(uint8_t rhport, uint8_t ep_addr, uint8_t* buffer, uint16_t to // Schedule packets to be sent within interrupt edpt_schedule_packets(rhport, epnum, dir); + DCD_EXIT_CRITICAL(); return true; } @@ -595,7 +610,12 @@ 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); + DCD_ENTER_CRITICAL(); xfer_ctl_t* xfer = XFER_CTL_BASE(epnum, dir); + if (xfer->max_size == 0) { + DCD_EXIT_CRITICAL(); + return false; // Endpoint is closed + } xfer->buffer = NULL; xfer->ff = ff; xfer->total_len = total_bytes; @@ -603,6 +623,7 @@ bool dcd_edpt_xfer_fifo(uint8_t rhport, uint8_t ep_addr, tu_fifo_t* ff, uint16_t // Schedule packets to be sent within interrupt // TODO xfer fifo may only available for slave mode edpt_schedule_packets(rhport, epnum, dir); + DCD_EXIT_CRITICAL(); return true; } @@ -631,6 +652,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); @@ -989,8 +1011,10 @@ void dcd_int_handler(uint8_t rhport) { if (gintsts & GINTSTS_USBRST) { // USBRST is start of reset. + DCD_ENTER_CRITICAL(); dwc2->gintsts = GINTSTS_USBRST; handle_bus_reset(rhport); + DCD_EXIT_CRITICAL(); } if (gintsts & GINTSTS_ENUMDNE) { diff --git a/src/portable/synopsys/dwc2/dwc2_critical.h b/src/portable/synopsys/dwc2/dwc2_critical.h new file mode 100644 index 000000000..e2508c8fd --- /dev/null +++ b/src/portable/synopsys/dwc2/dwc2_critical.h @@ -0,0 +1,25 @@ +/* + * SPDX-FileCopyrightText: 2025 Espressif Systems (Shanghai) CO LTD + * + * SPDX-License-Identifier: Apache-2.0 + */ + +#ifndef TUSB_DWC2_CRITICAL_H_ +#define TUSB_DWC2_CRITICAL_H_ + +#include "common/tusb_mcu.h" + +#if defined(TUP_USBIP_DWC2_ESP32) + #include "freertos/FreeRTOS.h" + static portMUX_TYPE dcd_lock = portMUX_INITIALIZER_UNLOCKED; + #define DCD_ENTER_CRITICAL() portENTER_CRITICAL(&dcd_lock) + #define DCD_EXIT_CRITICAL() portEXIT_CRITICAL(&dcd_lock) + +#else + // Define critical section macros for DWC2 as no-op if not defined + // This is to avoid breaking existing code that does not use critical section + #define DCD_ENTER_CRITICAL() // no-op + #define DCD_EXIT_CRITICAL() // no-op +#endif + +#endif // TUSB_DWC2_CRITICAL_H_