Skip to content

Commit

Permalink
Add critsec to rp2040 xfer, check endpoint status
Browse files Browse the repository at this point in the history
 - 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.
  • Loading branch information
gemarcano committed Feb 16, 2024
1 parent b60d0ff commit ae077b0
Show file tree
Hide file tree
Showing 3 changed files with 35 additions and 8 deletions.
6 changes: 5 additions & 1 deletion src/portable/raspberrypi/rp2040/dcd_rp2040.c
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,10 @@ 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
Expand Down Expand Up @@ -178,6 +181,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)
Expand Down
16 changes: 13 additions & 3 deletions src/portable/raspberrypi/rp2040/rp2040_usb.c
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ 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;
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;
Expand Down Expand Up @@ -203,7 +203,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));

Expand All @@ -216,6 +220,12 @@ void hw_endpoint_xfer_start(struct hw_endpoint *ep, uint8_t *buffer, uint16_t to
{
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 )
{
return;
}

if ( ep->active )
{
// TODO: Is this acceptable for interrupt packets?
Expand Down Expand Up @@ -296,7 +306,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)
{
Expand Down
21 changes: 17 additions & 4 deletions src/portable/raspberrypi/rp2040/rp2040_usb.h
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -104,10 +105,22 @@ 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);
Expand Down

0 comments on commit ae077b0

Please sign in to comment.