Skip to content

Commit d4a39f5

Browse files
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 espressif/esp-idf#9691 Also reported in espressif/esp-usb#131
1 parent c83fb98 commit d4a39f5

File tree

2 files changed

+49
-0
lines changed

2 files changed

+49
-0
lines changed

src/portable/synopsys/dwc2/dcd_dwc2.c

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@
4040

4141
#include "device/dcd.h"
4242
#include "dwc2_common.h"
43+
#include "dwc2_critical.h"
4344

4445
#if TU_CHECK_MCU(OPT_MCU_GD32VF103)
4546
#define DWC2_EP_COUNT(_dwc2) DWC2_EP_MAX
@@ -58,6 +59,9 @@ typedef struct {
5859
uint8_t interval;
5960
} xfer_ctl_t;
6061

62+
/*
63+
This variable is modified from ISR context, so it must be protected by critical section
64+
*/
6165
static xfer_ctl_t xfer_status[DWC2_EP_MAX][2];
6266
#define XFER_CTL_BASE(_ep, _dir) (&xfer_status[_ep][_dir])
6367

@@ -321,6 +325,9 @@ static void edpt_disable(uint8_t rhport, uint8_t ep_addr, bool stall) {
321325
}
322326
}
323327

328+
// Since this function returns void, it is not possible to return a boolean success message
329+
// We must make sure that this function is not called when the EP is disabled
330+
// Must be called from critical section
324331
static void edpt_schedule_packets(uint8_t rhport, const uint8_t epnum, const uint8_t dir) {
325332
dwc2_regs_t* dwc2 = DWC2_REG(rhport);
326333
xfer_ctl_t* const xfer = XFER_CTL_BASE(epnum, dir);
@@ -540,6 +547,7 @@ void dcd_edpt_close_all(uint8_t rhport) {
540547
dwc2_regs_t* dwc2 = DWC2_REG(rhport);
541548
uint8_t const ep_count = _dwc2_controller[rhport].ep_count;
542549

550+
DCD_ENTER_CRITICAL();
543551
_dcd_data.allocated_epin_count = 0;
544552

545553
// Disable non-control interrupt
@@ -559,6 +567,7 @@ void dcd_edpt_close_all(uint8_t rhport) {
559567
dfifo_flush_rx(dwc2);
560568

561569
dfifo_device_init(rhport); // re-init dfifo
570+
DCD_EXIT_CRITICAL();
562571
}
563572

564573
bool dcd_edpt_iso_alloc(uint8_t rhport, uint8_t ep_addr, uint16_t largest_packet_size) {
@@ -577,7 +586,12 @@ bool dcd_edpt_xfer(uint8_t rhport, uint8_t ep_addr, uint8_t* buffer, uint16_t to
577586
uint8_t const epnum = tu_edpt_number(ep_addr);
578587
uint8_t const dir = tu_edpt_dir(ep_addr);
579588

589+
DCD_ENTER_CRITICAL();
580590
xfer_ctl_t* xfer = XFER_CTL_BASE(epnum, dir);
591+
if (xfer->max_size == 0) {
592+
DCD_EXIT_CRITICAL();
593+
return false; // Endpoint is closed
594+
}
581595
xfer->buffer = buffer;
582596
xfer->ff = NULL;
583597
xfer->total_len = total_bytes;
@@ -589,6 +603,7 @@ bool dcd_edpt_xfer(uint8_t rhport, uint8_t ep_addr, uint8_t* buffer, uint16_t to
589603

590604
// Schedule packets to be sent within interrupt
591605
edpt_schedule_packets(rhport, epnum, dir);
606+
DCD_EXIT_CRITICAL();
592607

593608
return true;
594609
}
@@ -604,14 +619,20 @@ bool dcd_edpt_xfer_fifo(uint8_t rhport, uint8_t ep_addr, tu_fifo_t* ff, uint16_t
604619
uint8_t const epnum = tu_edpt_number(ep_addr);
605620
uint8_t const dir = tu_edpt_dir(ep_addr);
606621

622+
DCD_ENTER_CRITICAL();
607623
xfer_ctl_t* xfer = XFER_CTL_BASE(epnum, dir);
624+
if (xfer->max_size == 0) {
625+
DCD_EXIT_CRITICAL();
626+
return false; // Endpoint is closed
627+
}
608628
xfer->buffer = NULL;
609629
xfer->ff = ff;
610630
xfer->total_len = total_bytes;
611631

612632
// Schedule packets to be sent within interrupt
613633
// TODO xfer fifo may only available for slave mode
614634
edpt_schedule_packets(rhport, epnum, dir);
635+
DCD_EXIT_CRITICAL();
615636

616637
return true;
617638
}
@@ -640,6 +661,7 @@ void dcd_edpt_clear_stall(uint8_t rhport, uint8_t ep_addr) {
640661
//--------------------------------------------------------------------
641662

642663
// 7.4.1 Initialization on USB Reset
664+
// Must be called from critical section
643665
static void handle_bus_reset(uint8_t rhport) {
644666
dwc2_regs_t *dwc2 = DWC2_REG(rhport);
645667
const uint8_t ep_count = DWC2_EP_COUNT(dwc2);
@@ -990,8 +1012,10 @@ void dcd_int_handler(uint8_t rhport) {
9901012

9911013
if (gintsts & GINTSTS_USBRST) {
9921014
// USBRST is start of reset.
1015+
DCD_ENTER_CRITICAL();
9931016
dwc2->gintsts = GINTSTS_USBRST;
9941017
handle_bus_reset(rhport);
1018+
DCD_EXIT_CRITICAL();
9951019
}
9961020

9971021
if (gintsts & GINTSTS_ENUMDNE) {
Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
/*
2+
* SPDX-FileCopyrightText: 2025 Espressif Systems (Shanghai) CO LTD
3+
*
4+
* SPDX-License-Identifier: Apache-2.0
5+
*/
6+
7+
#ifndef TUSB_DWC2_CRITICAL_H_
8+
#define TUSB_DWC2_CRITICAL_H_
9+
10+
#include "common/tusb_mcu.h"
11+
12+
#if defined(TUP_USBIP_DWC2_ESP32)
13+
#include "freertos/FreeRTOS.h"
14+
static portMUX_TYPE dcd_lock = portMUX_INITIALIZER_UNLOCKED;
15+
#define DCD_ENTER_CRITICAL() portENTER_CRITICAL(&dcd_lock)
16+
#define DCD_EXIT_CRITICAL() portEXIT_CRITICAL(&dcd_lock)
17+
18+
#else
19+
// Define critical section macros for DWC2 as no-op if not defined
20+
// This is to avoid breaking existing code that does not use critical section
21+
#define DCD_ENTER_CRITICAL() // no-op
22+
#define DCD_EXIT_CRITICAL() // no-op
23+
#endif
24+
25+
#endif // TUSB_DWC2_CRITICAL_H_

0 commit comments

Comments
 (0)