fix(dwc2): bound RXFLVL/IEPINT-OEPINT storms to avoid Interrupt WDT crashes

[hirokada]

Description

Under a rapid, sustained sequence of USB bus resets (e.g. frequent physical cable plug/unplug cycling, or a downstream host repeatedly re-enumerating), dcd_int_handler() could be starved of CPU for 300ms+ by two different unbounded/high-rate conditions, tripping the ESP32 Interrupt WDT and crashing the device. This PR bounds both, and also fixes a separate NULL-pointer crash left over from a stale TXFE interrupt-enable bit after a bus reset.

Specifically:

• The RXFLVL drain loop had no iteration cap and ran until hardware reported the RX FIFO empty. Under a hostile refill rate (FIFO filling faster than 64 packets/iteration can drain), it could spin unbounded entirely inside the ISR. Added a per-entry iteration cap, and when hit, mask RXFLVLM for a short cooldown instead of re-enabling immediately — otherwise many individually-bounded ISR entries back to back can still starve the CPU in aggregate.
• IEPINT/OEPINT re-entering dcd_int_handler at a pathological rate (e.g. during rapid, repeated re-enumeration attempts) produced the same CPU-starvation crash via a different path. handle_ep_irq() itself is already bounded (a fixed small loop over endpoint count), so this tracks entry rate in a rolling window instead of a per-call iteration count, and backs off with the same cooldown pattern as RXFLVL when the rate is judged pathological.
• handle_bus_reset() left a stale TXFE interrupt-enable bit (diepempmsk) set for endpoints with no queued transfer after a reset. A re-opened endpoint could get a TXFE interrupt before the class driver queued anything, and handle_epin_slave() would dereference the resulting NULL xfer->buffer in dfifo_write_packet() — a LoadProhibited crash. Cleared diepempmsk on bus reset, plus added a direct NULL guard in handle_epin_slave() as defense-in-depth.

Both cooldown activation counts are exposed via lock-free counters (dcd_dwc2_rxflvl_cooldown_count() / dcd_dwc2_ep_irq_cooldown_count()), intended to be read from normal task context — never logged from inside the ISR itself, since ESP_LOG/vprintf's newlib lock is unsafe to call from this interrupt level.

Related

Companion changes in Backbone-Labs/tori and Backbone-Labs/bblib (PR hathach#116) that consume the two new counter functions and drive the crash scenario this fixes.

Testing

Verified on hardware with serial log capture under sustained rapid USB cable plug-unplug cycling and repeated device-side reboots (the specific scenario that reliably reproduced the Interrupt WDT / LoadProhibited crashes before this fix). No crashes reproduced over extended (several-minutes-plus) sessions after this fix; the cooldown activation counters were confirmed via log to actually engage under real stress conditions without causing regressions in normal HID/audio traffic.

---

Checklist

• 🚨 This PR does not introduce breaking changes.
• All CI checks (GH Actions) pass.
• Documentation is updated as needed.
• Tests are updated or added as necessary.
• Code is well-commented, especially in complex areas.
• Git history is clean — commits are squashed to the minimum necessary.

[sodank808]

Note

This review was generated by Claude (Claude Code), run by @soconnor-backbone. Findings below were verified against the code on this branch, but a human should confirm the hardware-behavior claims — particularly the deadlock analysis in item 1, which I could not test on real silicon.

Thanks for the detailed writeup — the problem statement is well-argued and the third fix (diepempmsk clear on bus reset) looks correct and self-contained. My concerns are concentrated in the two cooldown mechanisms.

1. Both cooldowns can latch off permanently (blocking)

This is the one I'd want resolved before merge. Both cooldowns are expired only from inside dcd_int_handler, but each cooldown masks off the interrupt source that causes dcd_int_handler to be entered. If no other unmasked interrupt happens to fire after the deadline, the mask is never restored and the endpoint stays dead.

For the EP cooldown this is the clearer case. GINTMSK_IEPINT | GINTMSK_OEPINT are masked, and the remaining unmasked sources after dcd_init() are OTGINT, USBSUSPM, USBRST, ENUMDNEM, WUIM, RXFLVLM (slave mode), and SOFM only when enabled. On an idle-but-connected bus with SOF disabled, none of those fire — so a device that trips the EP cooldown while the host is simply not talking to it has its IN and OUT endpoints masked indefinitely. Recovery depends on the host initiating something, which is exactly the state a wedged device can't provoke.

The RXFLVL cooldown has a narrower but real version of the same problem: handle_epout_slave()'s xfer_complete path depends on OUT packets being popped by handle_rxflvl_irq(), so with RXFLVLM masked the OUT-completion interrupts that would re-enter the handler don't arrive either.

Two options:

• Preferred: arm a one-shot esp_timer for the cooldown deadline and restore the mask from the timer callback. That decouples recovery from interrupt traffic entirely.
• If you'd rather not add a timer, at minimum enable GINTMSK_SOFM for the duration of the cooldown to guarantee a 1 ms wake source, and clear it again on restore.

Either way, the invariant worth stating explicitly in a comment is "a masked cooldown must have a guaranteed wake source." Right now that's assumed rather than established.

2. esp_timer.h in a portable file breaks non-ESP builds (blocking for upstream)

src/portable/synopsys/dwc2/dcd_dwc2.c is shared by STM32, GD32, EFM32, XMC, and BCM (see the sibling dwc2_*.h headers). The unconditional #include "esp_timer.h" at line 43 and the bare esp_timer_get_time() calls will fail to compile on every one of those targets. The file already has the right idiom for this — TUP_USBIP_DWC2_ESP32, used at lines 483 and 500.

Suggest guarding the include and routing the time source through a small shim so the non-ESP path compiles (and, ideally, so the whole feature compiles out where it isn't needed). Worth deciding now whether this is meant to stay a fork-local patch or eventually go upstream to hathach/tinyusb — if upstream is the goal, this needs to be portable, and upstream will likely also want the whole mechanism behind an opt-in CFG_TUD_DWC2_* config knob rather than always-on.

3. The EP rate limiter measures the wrong thing

kMaxEpIrqEntriesPerWindow = 300 per 50 ms is described as "well above any legitimate rate," but the counter increments once per dcd_int_handler entry where either IEPINT or OEPINT is set — not per unit of work done. In slave mode a single IN transfer larger than one FIFO's worth generates a TXFE interrupt per FIFO refill, so a legitimate high-bandwidth IN endpoint (audio streaming, bulk) can produce entries at multiples of the 6 kHz that threshold implies, with each entry doing only a few hundred cycles of work. That's not a storm — it's normal operation — but it trips the limiter and stalls the endpoint for 5 ms.

Also note handle_ep_irq() is skipped entirely for both directions during the window, including EP0. A control transfer in flight when the cooldown engages is delayed 5 ms, which the comment's "no data is lost" claim is technically right about but which can still surface as an enumeration timeout on a strict host.

Two things would make this more defensible:

• Gate on cumulative time spent in the ISR rather than entry count. That measures the actual thing the Interrupt WDT cares about, and it can't be tripped by legitimate-but-fast traffic. Something like: accumulate esp_timer_get_time() deltas across the handler, back off when the accumulated time in the window exceeds a fraction of the WDT budget.
• If you keep entry counting, please document how 300/50 ms was derived, and sanity-check it against the highest-bandwidth IN endpoint configuration this firmware actually ships.

4. Rolling-window edge cases

• ep_irq_rate_window_start_us initializes to 0, so the first entry after boot sees now_us - 0 >= 50000 and starts a fresh window. Benign, but it means the very first window boundary is arbitrary rather than anchored.
• When the cooldown engages, ep_irq_rate_window_count = 0 but ep_irq_rate_window_start_us is left at the old value. After the 5 ms cooldown, the remainder of that same 50 ms window is still active with a zeroed count — so the effective post-cooldown budget is a full 300 entries in whatever time is left, which is a higher instantaneous rate than the limiter was trying to enforce. Resetting ep_irq_rate_window_start_us = now_us alongside the count would make the backoff monotonic.
• The window is a "reset on expiry" counter, not a true sliding window, so a burst straddling a boundary can pass up to ~600 entries in ~50 ms. Probably fine for this purpose, but the comment says "rolling window," which implies stricter behavior than what's implemented.

5. State not reset across init / bus reset

rxflvl_cooldown_until_us, ep_irq_cooldown_until_us, and the window counters are file-scope statics that survive dcd_init() and handle_bus_reset(). Since dwc2_core_init() does dwc2->gintmsk = 0 and rebuilds the mask, a cooldown pending across a re-init leaves the software state believing a mask is off when the hardware has already restored it — the next handler entry will |= a bit that's already set (harmless) but the bookkeeping is out of sync. Clearing all five in dcd_init() would remove that class of surprise.

Relatedly, these statics are single-instance while dcd_int_handler takes an rhport. Fine for the single-port ESP32-S3, but on a multi-port DWC2 part the two ports would share one cooldown. Worth a comment noting the assumption, or indexing by rhport.

6. Counter API and volatile

• dcd_dwc2_rxflvl_cooldown_count() / dcd_dwc2_ep_irq_cooldown_count() are defined with external linkage but have no declaration in any header, so callers need their own extern. Consider declaring them in a header alongside the driver, or at least adding a comment pointing at where the declaration lives.
• volatile uint32_t is right for tearing-free reads of a 32-bit aligned word on Xtensa, and the ISR-writes/task-reads pattern is fine here. The restrict-free plain increment is not atomic, but since the ISR is the only writer that's correct — worth a one-line comment saying so, since "lock-free counter" in the PR description reads like a stronger claim than what's needed.

7. Minor

• // crash-fix: as a comment prefix is a temporal marker that ages poorly — six months from now "crash-fix" doesn't say which crash. Referencing the tracking ticket, or just describing the invariant being maintained, will read better in git blame.
• The comment blocks are quite long relative to the code. The reasoning is genuinely valuable, but consider moving the multi-paragraph narrative into a single block comment at the top of the section and keeping the inline comments to one or two lines each.
• The new esp_timer entry in requirements_private is correct for the ESP-IDF component build, and pairs with item 2 above.

What I like

The diepempmsk fix is the strongest part of this PR — the root cause is precisely identified (register lifetime not matching xfer_status lifetime across a reset), the fix is minimal, and adding the NULL guard in handle_epin_slave() as defense-in-depth rather than instead of the root-cause fix is the right call. If items 1–3 turn out to need more design work, that fix stands on its own and could land separately.

[hirokada]

@sodank808, thanks for the detailed review — the deadlock/latch-off risk in item 1 and the portability break in item 2 are clear blockers and I'll fix those first.

For item 3 (EP IRQ rate limiter), I agree entry-count is the wrong metric — it can't distinguish a legitimately fast transfer (many cheap FIFO refills) from a genuinely pathological one. I'm switching to measuring cumulative time spent in the handler instead, but I want to set the threshold from real hardware measurements rather than guess a number — planning to add ISR-safe instrumentation (a volatile counter, following the same pattern as the existing cooldown counters, not logged from ISR context) to capture actual per-call timing under both normal high-throughput traffic and a reproduced version of the original crash scenario, then pick the threshold from that data.

Also planning to address 4-7: resetting the cooldown/window state in dcd_init(), fixing the post-cooldown window-reset edge case, adding a proper header declaration for the counter getters, and cleaning up the comments.

Will follow up here once these are in.

[hirokada]

@sodank808 all 7 items are now in:

1. Cooldown latch-off: while either cooldown is pending, dcd_int_handler() now force-enables GINTMSK_SOFM so a 1ms SOF wake source is guaranteed even on an otherwise-idle bus — a masked cooldown can no longer latch off permanently.
2. Portability: esp_timer.h and the entire cooldown mechanism (statics, getters, and the timing logic in the handler) are now gated behind #ifdef TUP_USBIP_DWC2_ESP32. Non-ESP DWC2 ports (STM32/GD32/EFM32/XMC/BCM) compile with the iteration cap only, re-enabling the mask immediately with no cooldown/timer dependency.
3. EP IRQ rate metric: switched from an entry-count threshold to a cumulative-time-in-handle_ep_irq() threshold per 50ms window. Instrumented with ISR-safe max-value counters and measured on hardware under normal high-load traffic (a concurrent high-bandwidth wireless workload while USB-active) — cumulative time topped out at ~2.2ms/window. Set the threshold at 30ms (~13x that ceiling, ~10x margin under the 300ms Interrupt WDT budget), keeping the old entry-count check as a backstop only (now 2000/window instead of 300).
4. Window-reset edge case: the window start (not just its counters) is now reset when a cooldown engages, so the post-cooldown window gets a full fresh budget instead of inheriting leftover time from the window that tripped it.
5. dcd_init() reset: cooldown/window state is now reset in dcd_init(), since dwc2_core_init() rebuilds the hardware mask but doesn't touch this software bookkeeping. Diagnostic counters (*_cooldown_count, *_max_us) are deliberately left alone across re-inits for field visibility. Also documented that these statics are single-instance (not indexed by rhport) since ESP32-S3 — the only TUP_USBIP_DWC2_ESP32 target — is single-port.
6. Header declaration: added a small standalone header (dcd_dwc2_esp32_diag.h) declaring the six diagnostic getters, included from dcd_dwc2.c itself so the definitions get checked against the declarations, instead of bare extern "C" decls. Kept separate from dwc2_esp32.h since that header pulls in FreeRTOS/esp_intr_alloc/soc internals not needed just to call these.
7. Comment cleanup: removed the crash-fix: comment prefixes throughout, and the temporary measurement-instrumentation labels once the item-3 threshold was set from real data.

Verified on hardware: extended (several-minutes+) sessions covering repeated USB cable plug/unplug (both host and device side) plus additional concurrent radio activity — no regressions, cooldown activation counters remained at 0 throughout normal operation (window cumulative time topped out well under the new 30ms threshold), confirming no false-positive triggering under real traffic.

[hirokada]

Correction on item 6: the new header (dcd_dwc2_esp32_diag.h) is only consumed within this repo (dcd_dwc2.c includes it to get a definition-vs-declaration check on its own getters). Downstream callers of these getters can't include it, since they build against the registry's espressif/tinyusb package rather than this fork, and the header isn't available there — that side stays on the original bare extern "C" declarations, no changes needed downstream.

[sodank808]

Note

Follow-up review generated by Claude (Claude Code), run by @soconnor-backbone. I verified each of the 7 items against the code on abc42871 rather than against the summary. Hardware-behavior claims (the ~2.2ms measurement, the SOF timing) still need human confirmation.

Nice work — I checked all seven items in the code and they're genuinely addressed, not just described. The item-3 rewrite in particular is the right change: gating on cumulative handle_ep_irq() time is the metric that actually corresponds to the WDT budget, and deriving the threshold from measurement rather than guessing is exactly what I was hoping for. Keeping the entry count as an explicitly-labeled backstop for ISR entry/exit overhead (which the cumulative-time measurement genuinely doesn't capture) is a good call.

Two things before this is ready — one is a real gap in the item-1 fix, the other is a public-repo hygiene issue.

A. dcd_sof_enable(false) can still strand a cooldown (blocking, small fix)

The SOFM wake source closes the common case, and re-asserting it on every entry while a cooldown is outstanding is the right shape. But there's still a path that clears it from outside the handler:

void dcd_sof_enable(uint8_t rhport, bool en) {
  _dcd_data.sof_en = en;
  ...
  } else {
    dwc2->gintmsk &= ~GINTMSK_SOFM;   // no cooldown awareness
  }
}

dcd_sof_enable() runs from task context and unconditionally clears SOFM. The file's own comment at line 556 notes that audio/video/iso class drivers call this deliberately to toggle their ISR. So: a cooldown engages (masking IEPINT/OEPINT), the handler sets SOFM, then before the next SOF arrives an iso class driver calls dcd_sof_enable(rhport, false) — SOFM is cleared, and with EP interrupts still masked and the bus otherwise idle, nothing re-enters the handler to expire the cooldown. That's the same permanent latch-off as the original item 1, just through a narrower window (sub-millisecond, needing a concurrent iso-driver call).

It's a narrow race, but it's reachable on exactly the audio-streaming workload this firmware runs, and the failure mode is a permanently dead endpoint. Suggest making the else branch cooldown-aware:

  } else {
#ifdef TUP_USBIP_DWC2_ESP32
    // Don't drop the cooldown's guaranteed wake source (see dcd_int_handler).
    if (rxflvl_cooldown_until_us == 0 && ep_irq_cooldown_until_us == 0)
#endif
    dwc2->gintmsk &= ~GINTMSK_SOFM;
  }

Note this also needs a read of the cooldown statics from task context while the ISR writes them — worth a brief comment on why that's acceptable here (same single-writer reasoning as the diagnostic counters), or wrap it in a critical section if you'd rather not reason about it.

Two smaller notes on the same mechanism, non-blocking:

• The handler sets SOFM before the gintsts snapshot, and the SOF block later in the same pass clears it again when !sof_en. That works — the cooldown re-asserts it next entry — but it means SOFM flickers on/off every millisecond for the cooldown's duration. Correct, just worth a comment so a future reader doesn't "fix" the apparent redundancy.
• Enabling SOFM produces a dcd_event_sof() per millisecond, which is forwarded to the stack even though nothing requested SOF events. Harmless for the 2–5ms cooldowns here, but it does mean a cooldown has a side effect on sof_en=false consumers. A one-line note in the comment would cover it.

B. Internal product name in a code comment (blocking for a public repo)

dcd_dwc2.c line ~1305:

// measurement across normal high-load operation (HMB wireless audio
// streaming while USB-active) topped out at ~2.2ms of cumulative

This is a public fork, so internal product codenames shouldn't land in it. The measurement context is genuinely valuable — please just describe it generically, e.g. "a concurrent high-bandwidth wireless audio workload while USB-active." Same substance, no internal reference. Worth a quick scan of the branch for any others before merge; that was the only one I found in the added lines.

Verified as addressed

For the record, what I confirmed in the code:

#	Item	Status
1	Cooldown latch-off	Fixed, modulo A above — SOFM force-enabled while either cooldown is pending, re-asserted each entry
2	Portability	Fixed — esp_timer.h, statics, getters, and all timing logic behind TUP_USBIP_DWC2_ESP32; both #else branches re-enable the mask immediately, so non-ESP ports keep the iteration cap with no timer dependency
3	Rate metric	Fixed — cumulative handle_ep_irq() time (30ms/50ms window) is now the primary trigger; entry count retained at 2000 as a labeled backstop
4	Window-reset edge case	Fixed — ep_irq_rate_window_start_us reset alongside count and cumulative time at line ~1347
5	dcd_init() reset	Fixed — all five bookkeeping statics cleared after dwc2_core_init(); diagnostics deliberately preserved, and the single-port rhport assumption is now documented
6	Header declaration	Fixed — dcd_dwc2_esp32_diag.h included from dcd_dwc2.c itself, so definitions are checked against declarations
7	Comment cleanup	Done — crash-fix: prefixes removed

On item 6, thanks for the correction about downstream consumers building against the registry package — that's a real constraint I didn't have visibility into, and including the header from dcd_dwc2.c still gets the signature check where it's checkable, which is the part that mattered.

One forward-looking note, not a request: the review fix (item #N) comment prefixes now in the code have the same aging problem the crash-fix: prefixes did — they reference a review that won't be findable from git blame in a year. Consider dropping the (item #N) back-references before merge and letting the comments just state the invariant.

Aside from A and B, this looks good to me.

[hirokada]

@sodank808 thanks for the thorough re-review — both items addressed in ba20081:

A (SOF latch-off gap): dcd_sof_enable(false) now checks that no cooldown is outstanding before clearing SOFM, matching your suggested patch. Also added brief comments on the intentional SOFM flicker and the per-ms dcd_event_sof() side effect while a cooldown is pending, per your two non-blocking notes.

B (internal product name): replaced with a generic description. Scanned the rest of the diff and didn't find any other instances.

Also dropped the (item #N) back-references per your forward-looking note — comments now just state the invariant directly.

On verification: I can confirm no regressions under repeated USB cable plug/unplug cycling (the existing stress scenario), same as prior rounds. I want to be upfront that I can't personally exercise the specific condition item A fixes — it requires a storm cooldown to be genuinely engaged (which hasn't happened in any of our testing to date; cooldown activation counters have stayed at 0 throughout) at the same moment USB Audio streaming stops. So this fix is verified at the code level (matches your suggested patch and the surrounding invariants) rather than empirically reproduced on hardware. Wanted to flag that rather than imply more hardware confidence than I actually have.