dcd(ip3511): fix DEVCMDSTAT write-1-to-clear handling and EP0 setup races

DEVCMDSTAT mixes read/write fields with write-1-to-clear latches, so a blind
read-modify-write writes a pending latch back as a one and silently clears it -
a setup consumed that way strands EP0. Mask the latches on every update.

The setup path follows the manual's order: acknowledge the latch, then read the
payload. The EP0 IN interrupt is cleared along with EP0 OUT, as the control
endpoint flowchart requires - a control IN completion latched before the setup
must not reach usbd after it, where it would be applied to the request the setup
just started and arm its status stage early.

The payload is copied a byte at a time out of a buffer now declared volatile:
the controller DMAs a new setup packet into it as soon as the latch is cleared,
and C orders volatile accesses only against each other, so gcc sinks a plain
memcpy below the guard read that follows at -O2 and -O3 - leaving only -Os, the
level CI builds, correct.

1 files changed, 85 insertions, 21 deletions

diff --git a/src/portable/nxp/lpc_ip3511/dcd_lpc_ip3511.c b/src/portable/nxp/lpc_ip3511/dcd_lpc_ip3511.c
index d5b03e4b1..42f6750b1 100644
--- a/src/portable/nxp/lpc_ip3511/dcd_lpc_ip3511.c
+++ b/src/portable/nxp/lpc_ip3511/dcd_lpc_ip3511.c
@@ -87,6 +87,10 @@ enum {
   DEVCMDSTAT_SUSPEND_CHANGE_MASK = TU_BIT(25),
   DEVCMDSTAT_RESET_CHANGE_MASK   = TU_BIT(26),
   DEVCMDSTAT_VBUS_DEBOUNCED_MASK = TU_BIT(28),
+
+  // write-1-to-clear latches
+  DEVCMDSTAT_W1C_MASK            = DEVCMDSTAT_SETUP_RECEIVED_MASK | DEVCMDSTAT_CONNECT_CHANGE_MASK |
+                                   DEVCMDSTAT_SUSPEND_CHANGE_MASK | DEVCMDSTAT_RESET_CHANGE_MASK,
 };
 
 enum {
@@ -171,7 +175,9 @@ typedef struct
   ep_cmd_sts_t ep[2*MAX_EP_PAIRS][2];
   xfer_dma_t dma[2*MAX_EP_PAIRS];
 
-  TU_ATTR_ALIGNED(64) uint8_t setup_packet[8];
+  // volatile: the controller DMAs a new setup packet into this buffer as soon as the SETUP
+  // latch is cleared, so reads of it must stay ordered against the register accesses around them
+  TU_ATTR_ALIGNED(64) volatile uint8_t setup_packet[8];
 }dcd_data_t;
 
 // EP list must be 256-byte aligned
@@ -180,8 +186,12 @@ typedef struct
 //    Use CFG_TUD_MEM_SECTION to place it accordingly.
 CFG_TUD_MEM_SECTION TU_ATTR_ALIGNED(256) static dcd_data_t _dcd;
 
-// Dummy buffer to fix ZLPs overwriting the buffer (probably an USB/DMA controller bug)
-// TODO find way to save memory
+// Dummy buffer to fix ZLPs overwriting the buffer: Errata LPC55S6x USB.5 / LPC55S2x USB.4 - the
+// HS device controller always DMA-writes OUT data in 8-byte units, so up to 7 bytes land past the
+// received length. This redirects the ZLP case; the general short-OUT case is unhandled here
+// (TinyUSB's own endpoint buffers are sized/aligned so the spill stays inside them, but a tight
+// caller buffer can be overrun by up to 7 bytes - the SDK's documented workaround is a bounce
+// buffer). TODO find way to save memory
 CFG_TUD_MEM_SECTION TU_ATTR_ALIGNED(64) static uint8_t dummy[8];
 
 //--------------------------------------------------------------------+
@@ -221,7 +231,7 @@ static const dcd_controller_t _dcd_controller[] = {
 // INTERNAL OBJECT & FUNCTION DECLARATION
 //--------------------------------------------------------------------+
 
-TU_ATTR_ALWAYS_INLINE static inline uint16_t get_buf_offset(void const * buffer) {
+TU_ATTR_ALWAYS_INLINE static inline uint16_t get_buf_offset(void const volatile * buffer) {
   uint32_t addr = (uint32_t) buffer;
   TU_ASSERT( (addr & 0x3f) == 0, 0 );
   return ( (addr >> 6) & 0xFFFFUL ) ;
@@ -247,6 +257,16 @@ TU_ATTR_ALWAYS_INLINE static inline bool rhport_is_highspeed(uint8_t rhport) {
   return _dcd_controller[rhport].is_highspeed;
 }
 
+
+// DEVCMDSTAT mixes RW fields with write-1-to-clear latches (SETUP + the 3 change bits): a blind
+// RMW writes a pending latch back as 1 and silently clears it (a SETUP eaten this way strands
+// EP0). Mask the latches on every update; pass one in set_mask only to clear it.
+TU_ATTR_ALWAYS_INLINE static inline void devcmdstat_update(dcd_registers_t* dcd_reg,
+                                                           uint32_t clear_mask, uint32_t set_mask) {
+  const uint32_t v = dcd_reg->DEVCMDSTAT & ~(DEVCMDSTAT_W1C_MASK | clear_mask);
+  dcd_reg->DEVCMDSTAT = v | set_mask;
+}
+
 //--------------------------------------------------------------------+
 // CONTROLLER API
 //--------------------------------------------------------------------+
@@ -284,8 +304,10 @@ bool dcd_init(uint8_t rhport, const tusb_rhport_init_t* rh_init) {
   dcd_reg->DATABUFSTART = tu_align((uint32_t) &_dcd, TU_BIT(22)); // 22-bit alignment
   dcd_reg->INTSTAT      = dcd_reg->INTSTAT; // clear all pending interrupt
   dcd_reg->INTEN        = INT_DEVICE_STATUS_MASK;
-  dcd_reg->DEVCMDSTAT  |= DEVCMDSTAT_DEVICE_ENABLE_MASK | DEVCMDSTAT_DEVICE_CONNECT_MASK |
-                           DEVCMDSTAT_RESET_CHANGE_MASK | DEVCMDSTAT_CONNECT_CHANGE_MASK | DEVCMDSTAT_SUSPEND_CHANGE_MASK;
+  // deliberately clear every latch (incl. a SETUP left by a bootloader/warm start) for a
+  // deterministic init state
+  devcmdstat_update(dcd_reg, 0, DEVCMDSTAT_DEVICE_ENABLE_MASK | DEVCMDSTAT_DEVICE_CONNECT_MASK |
+                    DEVCMDSTAT_W1C_MASK);
 
   NVIC_ClearPendingIRQ(_dcd_controller[rhport].irqnum);
 
@@ -309,8 +331,7 @@ void dcd_set_address(uint8_t rhport, uint8_t dev_addr)
   // Response with status first before changing device address
   dcd_edpt_xfer(rhport, tu_edpt_addr(0, TUSB_DIR_IN), NULL, 0, false);
 
-  dcd_reg->DEVCMDSTAT &= ~DEVCMDSTAT_DEVICE_ADDR_MASK;
-  dcd_reg->DEVCMDSTAT |= dev_addr;
+  devcmdstat_update(dcd_reg, DEVCMDSTAT_DEVICE_ADDR_MASK, dev_addr);
 }
 
 void dcd_remote_wakeup(uint8_t rhport)
@@ -321,13 +342,13 @@ void dcd_remote_wakeup(uint8_t rhport)
 void dcd_connect(uint8_t rhport)
 {
   dcd_registers_t* dcd_reg = _dcd_controller[rhport].regs;
-  dcd_reg->DEVCMDSTAT |= DEVCMDSTAT_DEVICE_CONNECT_MASK;
+  devcmdstat_update(dcd_reg, 0, DEVCMDSTAT_DEVICE_CONNECT_MASK);
 }
 
 void dcd_disconnect(uint8_t rhport)
 {
   dcd_registers_t* dcd_reg = _dcd_controller[rhport].regs;
-  dcd_reg->DEVCMDSTAT &= ~DEVCMDSTAT_DEVICE_CONNECT_MASK;
+  devcmdstat_update(dcd_reg, DEVCMDSTAT_DEVICE_CONNECT_MASK, 0);
 }
 
 void dcd_sof_enable(uint8_t rhport, bool en)
@@ -380,9 +401,17 @@ void dcd_edpt_clear_stall(uint8_t rhport, uint8_t ep_addr)
 
   uint8_t const ep_id = ep_addr2id(ep_addr);
 
+  // Preserve rf_tv: for non-control endpoints it is a TYPE bit, not the toggle value (UM11126:
+  // T=1 + RF 1/0 = interrupt/iso). Zeroing it here turned HS periodic interrupt endpoints into
+  // isochronous - no handshake on OUT, dead IN (usbtest cases 25/26 on lpc55 HS port).
+  // TODO implement the Errata LPC546xx USB.13 work-around (same semantics in UM11126): with RF/TV preserved at 1, TR
+  // loads the toggle from TV, so an HS interrupt endpoint restarts on DATA1 after clear-halt and
+  // the host discards one packet as a retransmission. The documented workaround needs an
+  // interrupt-on-NAK state machine (park as generic TR=1/TV=0, wait for a NAKed token to latch
+  // toggle 0 via EPTOGGLE, restore the type) - deferred; one lost packet beats the fully broken
+  // endpoint the old rf_tv clear caused.
   _dcd.ep[ep_id][0].cmd_sts.stall        = 0;
   _dcd.ep[ep_id][0].cmd_sts.toggle_reset = 1;
-  _dcd.ep[ep_id][0].cmd_sts.rf_tv        = 0;
 }
 
 bool dcd_edpt_open(uint8_t rhport, tusb_desc_endpoint_t const * p_endpoint_desc)
@@ -432,7 +461,7 @@ void dcd_edpt_close_all (uint8_t rhport)
 {
   for (uint8_t ep_id = 0; ep_id < 2*_dcd_controller[rhport].ep_pairs; ++ep_id)
   {
-    _dcd.ep[ep_id][0].cmd_sts.active = _dcd.ep[ep_id][0].cmd_sts.active = 0; // TODO proper way is to EPSKIP then wait ep[][].active then write ep[][].disable (see table 778 in LPC55S69 Use Manual)
+    _dcd.ep[ep_id][0].cmd_sts.active = _dcd.ep[ep_id][1].cmd_sts.active = 0; // TODO proper way is to EPSKIP then wait ep[][].active then write ep[][].disable (see table 778 in LPC55S69 Use Manual)
     _dcd.ep[ep_id][0].cmd_sts.disable = _dcd.ep[ep_id][1].cmd_sts.disable = 1;
   }
 }
@@ -538,7 +567,7 @@ static void bus_reset(uint8_t rhport)
   dcd_reg->EPSKIP       = 0xFFFFFFFF;
 
   dcd_reg->INTSTAT      = dcd_reg->INTSTAT;                               // clear all pending interrupt
-  dcd_reg->DEVCMDSTAT  |= DEVCMDSTAT_SETUP_RECEIVED_MASK;                    // clear setup received interrupt
+  devcmdstat_update(dcd_reg, 0, DEVCMDSTAT_SETUP_RECEIVED_MASK);          // clear setup received interrupt
   dcd_reg->INTEN        = INT_DEVICE_STATUS_MASK | TU_BIT(0) | TU_BIT(1); // enable device status & control endpoints
 }
 
@@ -597,18 +626,25 @@ void dcd_int_handler(uint8_t rhport)
 {
   dcd_registers_t* dcd_reg = _dcd_controller[rhport].regs;
 
-  uint32_t const cmd_stat = dcd_reg->DEVCMDSTAT;
-
   uint32_t int_status = dcd_reg->INTSTAT;
-	int_status &= dcd_reg->INTEN;
+  int_status &= dcd_reg->INTEN;
   dcd_reg->INTSTAT = int_status; // Acknowledge handled interrupt
 
   if (int_status == 0) return;
 
+  // Snapshot after the INTSTAT ack: latch bits persist (RWC) so nothing is lost, while the reverse
+  // order could consume INTSTAT bit0 for a SETUP not yet visible in the snapshot - stranding the
+  // SETUP (INTSTAT is edge-latched) and feeding bit0 to process_xfer_isr as a bogus completion.
+  uint32_t const cmd_stat = dcd_reg->DEVCMDSTAT;
+
   //------------- Device Status -------------//
   if ( int_status & INT_DEVICE_STATUS_MASK )
   {
-    dcd_reg->DEVCMDSTAT |= DEVCMDSTAT_RESET_CHANGE_MASK | DEVCMDSTAT_CONNECT_CHANGE_MASK | DEVCMDSTAT_SUSPEND_CHANGE_MASK;
+    // clear only the change latches observed in the snapshot: one latched by hardware between the
+    // snapshot and this write would be acknowledged unseen (its DEV_INT re-latches and dispatches
+    // next pass instead)
+    devcmdstat_update(dcd_reg, 0, cmd_stat &
+                      (DEVCMDSTAT_RESET_CHANGE_MASK | DEVCMDSTAT_CONNECT_CHANGE_MASK | DEVCMDSTAT_SUSPEND_CHANGE_MASK));
 
     if ( cmd_stat & DEVCMDSTAT_RESET_CHANGE_MASK) // bus reset
     {
@@ -653,15 +689,43 @@ void dcd_int_handler(uint8_t rhport)
     _dcd.ep[0][0].cmd_sts.active = _dcd.ep[1][0].cmd_sts.active = 0;
     _dcd.ep[0][0].cmd_sts.stall = _dcd.ep[1][0].cmd_sts.stall = 0;
 
-    dcd_reg->DEVCMDSTAT |= DEVCMDSTAT_SETUP_RECEIVED_MASK;
+    // UM flow: ack the latch FIRST, then read the payload. This IP has no setup lockout, so a
+    // back-to-back SETUP can overwrite _dcd.setup_packet at any time - but with the latch already
+    // released, any such overwrite re-latches SETUP_RECEIVED and is redelivered (worst case a
+    // superseded duplicate, absorbed by usbd's queued-setup counter). The reverse order can
+    // consume the newer SETUP's latch unseen and lose it.
+    devcmdstat_update(dcd_reg, 0, DEVCMDSTAT_SETUP_RECEIVED_MASK);
+
+    // UM11126 Fig 163 (control EP0 flowchart) requires clearing the EP0IN interrupt here: a
+    // control IN completion latched before this SETUP must not reach usbd after it, where it
+    // would be applied to the new request and arm its status stage early. EP0OUT goes with it -
+    // bit0 is set by SETUP reception too, and left set it would replay next pass as a phantom
+    // completion. Neither can discard live work: the SETUP latch NAKs all EP0 traffic until the
+    // update above, and both EP0 Active bits were cleared a few lines up.
+    dcd_reg->INTSTAT = TU_BIT(0) | TU_BIT(1);
 
-    dcd_event_setup_received(rhport, _dcd.setup_packet, true);
+    // Copied a byte at a time rather than with memcpy: C orders volatile accesses only against
+    // each other, so a non-volatile copy of this buffer may be sunk below the guard read that
+    // follows - gcc does exactly that at -O2 and -O3, leaving only -Os correct.
+    uint8_t setup_copy[8];
+    for (uint8_t i = 0; i < sizeof(setup_copy); i++) {
+      setup_copy[i] = _dcd.setup_packet[i];
+    }
+
+    // a SETUP that raced in after the acks (its bit0 consumed above) makes this copy suspect:
+    // its latch is visible again, so re-raise the endpoint interrupt and let the next pass
+    // deliver the newer payload rather than passing up bytes that may be torn between the two
+    if (dcd_reg->DEVCMDSTAT & DEVCMDSTAT_SETUP_RECEIVED_MASK) {
+      dcd_reg->INTSETSTAT = TU_BIT(0);
+    } else {
+      dcd_event_setup_received(rhport, setup_copy, true);
+    }
 
     // keep waiting for next setup
     prepare_setup_packet(rhport);
 
-    // clear bit0
-    int_status = tu_bit_clear(int_status, 0);
+    // drop both EP0 bits: acked above, and neither belongs to the request this SETUP starts
+    int_status &= ~(TU_BIT(0) | TU_BIT(1));
   }
 
   // Endpoint transfer complete interrupt