From 1eb62ba493465a59488c27b233bb5de865a1f2bd Mon Sep 17 00:00:00 2001 From: Gabriel Marcano 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. - Fixed a null-pointer dereference regarding ep->endpoint_control for endpoint 0, which does not have a control register. --- src/portable/raspberrypi/rp2040/dcd_rp2040.c | 7 +++++-- src/portable/raspberrypi/rp2040/rp2040_usb.c | 19 ++++++++++++++----- src/portable/raspberrypi/rp2040/rp2040_usb.h | 19 +++++++++++++++---- 3 files changed, 34 insertions(+), 11 deletions(-) diff --git a/src/portable/raspberrypi/rp2040/dcd_rp2040.c b/src/portable/raspberrypi/rp2040/dcd_rp2040.c index 2e177d5cbf..cb8559d5d6 100644 --- a/src/portable/raspberrypi/rp2040/dcd_rp2040.c +++ b/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) { diff --git a/src/portable/raspberrypi/rp2040/rp2040_usb.c b/src/portable/raspberrypi/rp2040/rp2040_usb.c index 43f48da396..642b760f4a 100644 --- a/src/portable/raspberrypi/rp2040/rp2040_usb.c +++ b/src/portable/raspberrypi/rp2040/rp2040_usb.c @@ -152,8 +152,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; @@ -182,7 +182,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)); @@ -194,7 +198,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); @@ -263,7 +272,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); diff --git a/src/portable/raspberrypi/rp2040/rp2040_usb.h b/src/portable/raspberrypi/rp2040/rp2040_usb.h index d4d29a816e..fae723abd9 100644 --- a/src/portable/raspberrypi/rp2040/rp2040_usb.h +++ b/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);