Skip to content

usb: pipelined register writes for the bring-up; Jaguar3 stage timing - #417

Open
gilankpam wants to merge 1 commit into
OpenIPC:masterfrom
gilankpam:perf/usb-pipelined-init-writes
Open

usb: pipelined register writes for the bring-up; Jaguar3 stage timing#417
gilankpam wants to merge 1 commit into
OpenIPC:masterfrom
gilankpam:perf/usb-pipelined-init-writes

Conversation

@gilankpam

Copy link
Copy Markdown
Contributor

Jaguar3 InitWrite on an RTL8812EU is ~14k synchronous EP0 round trips and
essentially nothing else — no waiting on hardware, just USB control-transfer
latency. This measures that, then removes most of it.

Measuring it

InitTimer already bracketed init stages with a duration. It now also reports
the number of USB vendor control transfers each stage spent
(src/UsbXferCount.h, a process-wide counter bumped by UsbTransport), which
is the unit the bring-up is actually paid in. init.timing gains an xfers
field; docs/logging.md carries the schema.

Every Jaguar3 rtw_hal_init / InitWrite stage is bracketed.

Removing it

A control transfer costs 76–80 µs synchronous on an embedded host (ssc338q)
and ~27 µs pipelined 8-deep. EP0 completes URBs in submission order, so a
queue of pending writes followed by a read behaves exactly like the
synchronous sequence — the host just doesn't sit through a round trip per
write.

IRtlTransport gains write_batch_begin / write_batch_end /
flush_writes. Inside a batch, UsbTransport submits register writes as
async URBs; a read (submitted behind the queue and waited on its own
completion), a bulk transfer, or an explicit flush is what waits. Jaguar3
InitWrite runs its whole bring-up in one RAII scope, ended before the coex
thread starts. The ms-scale settle delays flush first.

All three methods default to no-ops, so PcieTransport and every
generation other than Jaguar3 are untouched.

Contract

Batches are single-threaded: open one only while no other thread touches the
transport, and close it before any worker starts. This is enforced by
convention, not by an assert — worth a reviewer's attention.

Failure paths

A drain that times out cancels what is still submitted and keeps pumping for
the cancellations rather than declaring the queue empty. Zeroing the in-flight
count by hand would let a late callback drive it negative (silently disabling
every later drain), return a slot to the free list twice, and leave the
destructor calling libusb_free_transfer on a transfer libusb still owns.
Slots that genuinely cannot be reaped — dead event loop, yanked device — are
retired for the session and leaked at teardown instead, which is the lesser
evil against handing libusb a dangling transfer.

Separately: the RF radio-table load is write-only

Bits [31:20] of the direct window read back 0 for all 1540 entries, cold and
warm (histogrammed on one 8812EU unit), so the vendor's MASK20BITS
read-modify-write preserved nothing while paying a synchronous read per
entry — about half the RF-table stage.

Results and limits

Measured on one drone-side unit: warm InitWrite 1.30 → 0.65 s, cold
2.04 → ~0.7 s. One unit, one host — the transfer-count reduction is
deterministic, the wall-clock saving is not replicated across parts.

Init (RX-only) opens no batch yet and is unmeasured on a ground-station
card, so the RX bring-up path is unchanged by this PR and gets none of the
speedup.

cmake --build + ctest green (55/55, la_csi_math skipped — it needs
python numpy/scipy). No headless coverage of the pipelining itself: it needs a
real device, and the drain/teardown paths above need a wedged or unplugged
one, which is tests/tx_teardown_asan.sh territory rather than ctest. Those
have not been run against this branch.

🤖 Generated with Claude Code

https://claude.ai/code/session_01JiAS32956Z3cmTbnmcXYkT

InitWrite on the RTL8812EU is ~14k synchronous EP0 round trips and nothing
else. InitTimer now brackets every Jaguar3 rtw_hal_init / InitWrite stage and
reports the control-transfer count per stage (UsbXferCount.h), which is the
unit the bring-up is actually paid in. init.timing gains an `xfers` field;
docs/logging.md carries the schema.

A transfer costs 76-80 us synchronous on an embedded host (ssc338q) and ~27 us
pipelined 8-deep. EP0 completes URBs in submission order, so
IRtlTransport::write_batch_begin/end lets UsbTransport queue writes as async
URBs and wait only on reads (submitted behind the queue), bulk transfers and
flush_writes. Jaguar3 InitWrite runs its whole bring-up in one batch (RAII
scope, closed before the coex thread starts); the ms-scale settle delays flush
first. The three methods default to no-ops, so PCIe is unaffected.

Batches are single-threaded by contract: open one only while no other thread
touches the transport, and close it before any worker starts.

A drain that times out cancels what is still submitted and keeps pumping for
the cancellations rather than declaring the queue empty: an in-flight count
zeroed by hand goes negative on the late callback, which silently disables
every later drain, returns a slot to the free list twice, and leaves the
destructor freeing a transfer libusb still owns. Slots that genuinely cannot
be reaped (dead event loop, yanked device) are retired for the session and
leaked at teardown instead.

Separately, the RF radio-table load is write-only: bits [31:20] of the direct
window read back 0 for all 1540 entries, cold and warm, so the vendor's
MASK20BITS read-modify-write preserved nothing while paying a synchronous read
per entry -- about half the RF-table stage.

Measured on one drone-side unit: warm InitWrite 1.30 -> 0.65 s, cold
2.04 -> ~0.7 s. Init (RX-only) opens no batch yet and is unmeasured on a
ground-station card, so the RX bring-up path is unchanged by this commit.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JiAS32956Z3cmTbnmcXYkT
@qodo-free-for-open-source-projects

Copy link
Copy Markdown

PR Summary by Qodo

Pipeline Jaguar3 USB bring-up writes and add transfer timing

✨ Enhancement 📝 Documentation 🕐 40+ Minutes

Grey Divider

AI Description

• Pipeline Jaguar3 TX bring-up register writes over ordered asynchronous USB EP0 transfers.
• Add per-stage duration and control-transfer telemetry throughout Jaguar3 initialization.
• Remove redundant RF table reads and safely drain failed asynchronous transfers.
Diagram

sequenceDiagram
    participant Init as Jaguar3 Init
    participant Adapter as RtlAdapter
    participant USB as UsbTransport
    participant EP0 as USB EP0
    participant Count as Transfer Counter
    participant Timer as InitTimer
    participant Log as Event Log
    Init->>Adapter: Begin write batch
    Adapter->>USB: Enable pipeline
    loop Register writes
        Init->>Adapter: Write register
        Adapter->>USB: Queue write
        USB->>EP0: Submit async URB
        USB->>Count: Increment transfer
    end
    Init->>Adapter: Read or flush
    Adapter->>USB: Establish ordering barrier
    alt Read follows writes
        USB->>EP0: Submit ordered read
        EP0-->>USB: Complete queue and read
    else Explicit flush
        USB->>EP0: Wait for completions
        EP0-->>USB: Complete queued writes
    end
    USB-->>Init: Return result
    Init->>Adapter: End write batch
    Init->>Timer: Record stage
    Timer->>Count: Read transfer delta
    Timer->>Log: Emit ms and xfers
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Device-side multi-register command
  • ➕ Could reduce both host wakeups and USB request count beyond an eight-deep pipeline.
  • ➕ Would avoid managing multiple simultaneous libusb transfer objects.
  • ➖ Requires firmware or vendor-protocol support that is not currently available.
  • ➖ Creates substantially greater compatibility and hardware-validation risk.
2. Dedicated USB event thread
  • ➕ Could continuously reap completions without initialization code pumping libusb events.
  • ➕ Could support pipelining outside single-threaded bring-up scopes.
  • ➖ Introduces synchronization with existing RX and TX event handling.
  • ➖ Makes libusb teardown and device-removal ownership significantly harder.
3. Keep synchronous writes
  • ➕ Preserves the simplest transfer lifetime and failure model.
  • ➕ Requires no single-threaded batch contract.
  • ➖ Retains roughly 14,000 serial EP0 round trips during Jaguar3 bring-up.
  • ➖ Cannot deliver the measured initialization latency reduction.

Recommendation: The bounded, scoped libusb pipeline is the best available approach because it preserves register ordering without requiring firmware changes and leaves non-USB transports unchanged. Review should focus on slot ownership, cancellation callbacks, allocation failures, repeated batch termination, and enforcement of the single-threaded contract; hardware teardown testing remains important.

Files changed (13) +409 / -17

Enhancement (10) +384 / -15
InitTimer.hReport USB transfer deltas for initialization stages +22/-6

Report USB transfer deltas for initialization stages

• Snapshots the process-wide USB control-transfer counter and emits per-stage and total deltas alongside elapsed milliseconds.

src/InitTimer.h

RtlAdapter.hExpose register-write batching through the adapter +4/-0

Expose register-write batching through the adapter

• Adds adapter forwarding methods for beginning, ending, and explicitly flushing transport write batches.

src/RtlAdapter.h

RtlTransport.hDefine the transport write-batching contract +14/-0

Define the transport write-batching contract

• Adds default no-op batching and flushing hooks to 'IRtlTransport'. The documented contract preserves PCIe behavior while requiring exclusive, single-threaded transport access during a batch.

src/RtlTransport.h

UsbTransport.cppImplement bounded asynchronous USB register pipelines +193/-0

Implement bounded asynchronous USB register pipelines

• Adds an eight-slot libusb control-transfer pool with ordered reads, explicit drains, cancellation recovery, error accounting, and safe retirement of unreaped transfers. Bulk and byte transfers flush queued writes before proceeding.

src/UsbTransport.cpp

UsbTransport.hAdd USB pipeline state and count control transfers +63/-0

Add USB pipeline state and count control transfers

• Routes batched register reads and writes through asynchronous helpers while counting every vendor control transfer. Declares slot ownership, queue depth, completion state, and abandoned-transfer safeguards.

src/UsbTransport.h

UsbXferCount.hAdd a process-wide USB control-transfer counter +14/-0

Add a process-wide USB control-transfer counter

• Introduces a header-only relaxed atomic counter shared by 'UsbTransport' and 'InitTimer'.

src/UsbXferCount.h

HalJaguar3.cppInstrument Jaguar3 HAL stages and optimize RF table loading +40/-8

Instrument Jaguar3 HAL stages and optimize RF table loading

• Adds timing checkpoints around HAL initialization and table phases, flushing queued writes before millisecond settle delays. Replaces redundant masked RF-window read-modify-writes with direct 20-bit writes.

src/jaguar3/HalJaguar3.cpp

HalJaguar3.hAllow table loading to report timing checkpoints +1/-1

Allow table loading to report timing checkpoints

• Updates the table-loading helper to optionally receive an 'InitTimer' for detailed phase instrumentation.

src/jaguar3/HalJaguar3.h

Halrf8822e.cppDrain queued writes before calibration delays +1/-0

Drain queued writes before calibration delays

• Flushes pipelined register writes before millisecond RF calibration sleeps so settle periods begin after hardware receives the writes.

src/jaguar3/Halrf8822e.cpp

RtlJaguar3Device.cppBatch and time the Jaguar3 TX bring-up path +32/-0

Batch and time the Jaguar3 TX bring-up path

• Wraps 'InitWrite' in an RAII write batch, adds checkpoints across major stages, and closes the batch before starting the coexistence thread. The RX-only 'Init' path intentionally remains synchronous.

src/jaguar3/RtlJaguar3Device.cpp

Refactor (1) +1 / -1
Halrf8822e.hMake millisecond delays transport-aware +1/-1

Make millisecond delays transport-aware

• Converts 'delay_ms' from static to instance-bound so it can flush the device transport before sleeping.

src/jaguar3/Halrf8822e.h

Documentation (2) +24 / -1
logging.mdDocument transfer counts in initialization timing events +1/-1

Document transfer counts in initialization timing events

• Extends the documented 'init.timing' schema with the 'xfers' field and clarifies that PCIe reports zero.

docs/logging.md

CLAUDE.mdDocument Jaguar3 bring-up performance and batching constraints +23/-0

Document Jaguar3 bring-up performance and batching constraints

• Records measured initialization costs and speedups, the single-threaded pipeline contract, required delay flushes, RX-path limitation, and write-only RF table rationale.

src/jaguar3/CLAUDE.md

gilankpam added a commit to gilankpam/devourer that referenced this pull request Sep 9, 2026
…e timing

Jaguar3 InitWrite is ~14k synchronous EP0 round trips and nothing else.
InitTimer now reports the control-transfer count per stage, and
IRtlTransport::write_batch_begin/end pipelines the bring-up's register writes
behind EP0's in-order completion: warm InitWrite 1.30 -> 0.65 s, cold
2.04 -> ~0.7 s on one drone-side unit. The RX-only Init path opens no batch
yet and is unchanged. Submitted upstream as OpenIPC#417.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JiAS32956Z3cmTbnmcXYkT
@qodo-free-for-open-source-projects

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (3) 📘 Rule violations (1) 📜 Skill insights (0)

Grey Divider


Action required

1. Transport guidance can drift 📘 Rule violation ⚙ Maintainability
Description
src/jaguar3/CLAUDE.md restates the batching API's ordering, wait, threading, and delay-flush
contract instead of referring to IRtlTransport and its implementation documentation. A later
transport-contract change can update the authoritative headers without updating this backend guide,
leaving Jaguar3 maintainers with conflicting instructions.
Code

src/jaguar3/CLAUDE.md[R46-48]

+in submission order, so `UsbTransport` queues writes asynchronously inside
+a `write_batch_begin/end` scope and only reads (submitted behind the queue,
+waited on their own completion), bulk transfers and `flush_writes` wait.
Evidence
Compliance rule 1 requires guidance to refer to authoritative source headers rather than copying
their API documentation. The new Jaguar3 guide repeats the ordering and wait semantics from
IRtlTransport, as well as its single-threaded contract and flush behavior.

CLAUDE.md: Keep Repository Guidance Narrowly Scoped and Non-Duplicative
src/jaguar3/CLAUDE.md[39-55]
src/RtlTransport.h[71-83]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The Jaguar3 guidance duplicates the pipelined-write API contract already documented in the transport headers.

## Issue Context
Keep only Jaguar3-specific bring-up facts here and refer readers to the authoritative transport interface for batching semantics, ordering, waits, and threading requirements.

## Fix Focus Areas
- src/jaguar3/CLAUDE.md[39-55]
- src/RtlTransport.h[71-83]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. USB teardown can access freed memory 🐞 Bug ☼ Reliability
Description
flush_writes() can mark submitted control transfers abandoned and return, while each unreaped
AsyncWrite retains a raw UsbTransport *self that async_write_cb() uses to update transport
counters and its free-list vector. If cancellation callbacks cannot be pumped before destruction but
a shared live libusb context later dispatches the transfer, the leaked slot survives while its
transport does not, and the callback dereferences freed transport state.
Code

src/UsbTransport.cpp[R505-513]

+      /* The event loop itself is gone (a yanked device reports the error
+       * immediately, so the turns above cost nothing). Leave the slots
+       * submitted and off the free list; the destructor leaks them. */
+      _aw_errors += _aw_inflight;
+      _aw_abandoned = true;
+      _logger->error("USB: {} pipelined transfer(s) could not be reaped; "
+                     "their slots are retired for this session",
+                     _aw_inflight);
+    }
Evidence
The abandoned-transfer path can leave control transfers submitted, and the destructor deliberately
skips deleting their inflight slots, preserving only the transfer, slot, and buffer memory while
each slot still stores self = this. The callback unconditionally dereferences that pointer to
mutate _aw_inflight, _aw_completed, _aw_errors, and _aw_free; because multiple adapters can
share a process and another active libusb event pump may deliver the completion after this transport
is destroyed, the retained pointer can be dangling. The destructor’s later TX teardown does not
prevent this because it handles a separate bulk-transfer collection.

src/UsbTransport.cpp[338-370]
src/UsbTransport.cpp[404-419]
src/UsbTransport.cpp[486-514]
src/UsbTransport.cpp[1005-1022]
src/UsbTransport.cpp[338-355]
src/UsbTransport.cpp[385-419]
src/UsbTransport.cpp[486-515]
examples/common/DeviceSession.h[72-87]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Abandoned asynchronous control transfers retain a callback pointer to `UsbTransport`, but the destructor may complete while libusb still owns those transfers. A later callback can then access destroyed transport counters and free-list state.

## Issue Context
Leaking an unreaped transfer slot preserves its transfer and buffer memory, but it does not preserve the `UsbTransport` referenced by `AsyncWrite::self`. Make callback-owned bookkeeping independent of the transport lifetime, or otherwise guarantee that every submitted transfer is reaped and its callback cannot execute after transport destruction, including when another adapter sharing the process continues pumping the libusb context.

## Fix Focus Areas
- src/UsbTransport.cpp[338-355]
- src/UsbTransport.cpp[404-420]
- src/UsbTransport.cpp[486-515]
- src/UsbTransport.h[107-140]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Low-memory bring-up can crash USB setup 🐞 Bug ☼ Reliability
Description
write_batch_begin() inserts each AsyncWrite into both pool vectors without checking whether
libusb_alloc_transfer(0) returned a null t pointer. Under allocation pressure during Jaguar3 TX
initialization, the next batched read or write passes that null transfer to the fill and submit path
instead of falling back to synchronous bring-up or returning a controlled error.
Code

src/UsbTransport.cpp[R383-390]

+  if (_aw_all.empty()) {
+    for (int i = 0; i < kAsyncWriteDepth; ++i) {
+      auto *w = new AsyncWrite{};
+      w->t = libusb_alloc_transfer(0);
+      w->self = this;
+      _aw_all.push_back(w);
+      _aw_free.push_back(w);
+    }
Evidence
The pool-construction loop stores every result of libusb_alloc_transfer(0) without validation,
while both asynchronous read and write paths unconditionally pass the selected slot's transfer
pointer to libusb_fill_control_transfer() and then submit it. Existing RX and asynchronous TX
allocation paths in the same file explicitly check for null before filling or using a transfer,
demonstrating that allocation is fallible and must be handled.

src/UsbTransport.cpp[383-390]
src/UsbTransport.cpp[439-450]
src/UsbTransport.cpp[518-530]
src/UsbTransport.cpp[608-627]
src/UsbTransport.cpp[1076-1080]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The asynchronous slot pool can accept an `AsyncWrite` whose libusb transfer allocation failed, leaving a null transfer for later fill and submission. Do not enter batch mode with an unusable slot.

## Issue Context
Build the pool transactionally and validate every `libusb_alloc_transfer(0)` result before inserting or exposing a slot. If any allocation fails, release all partially allocated batch slots and leave batching disabled so register access falls back to the synchronous path, or return a controlled error. Other transfer-allocation paths in this transport already check for null before filling or submitting a transfer.

## Fix Focus Areas
- src/UsbTransport.cpp[376-394]
- src/UsbTransport.cpp[439-469]
- src/UsbTransport.cpp[518-539]
- src/UsbTransport.cpp[608-627]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

4. Timing records mix adapters' USB work 🐞 Bug ◔ Observability
Description
InitTimer computes each stage's xfers delta from a process-wide usb_ctrl_xfers atomic instead
of a counter associated with the timer's transport or adapter. When adapters initialize concurrently
or any USB worker performs control transfers during the checkpoint interval, those transfers are
attributed to unrelated stages and can also reach PCIe timer metrics that are documented to remain
zero.
Code

src/InitTimer.h[R44-45]

+  static uint64_t xfers() {
+    return devourer::usb_ctrl_xfers().load(std::memory_order_relaxed);
Evidence
usb_ctrl_xfers is a function-local static singleton with no adapter identity, and InitTimer
calculates its emitted field by subtracting construction and checkpoint snapshots of that shared
atomic. Because device creation, locking, transports, and timers are per adapter, independent
operations can overlap while incrementing the same counter, necessarily mixing their transfers into
each timer's delta.

src/InitTimer.h[28-45]
src/UsbXferCount.h[9-13]
src/RtlAdapter.h[4-7]
src/RtlAdapter.cpp[19-28]
src/jaguar1/RadioManagementModule.cpp[602-628]
src/PhydmWatchdog.cpp[48-55]
src/InitTimer.h[25-45]
src/WiFiDriver.h[24-42]
src/UsbOpen.cpp[82-103]
docs/logging.md[76-80]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`init.timing.xfers` is intended to describe the control-transfer work performed by a single initialization stage, but `InitTimer` derives it from a process-wide USB counter. Concurrent adapters or workers therefore contaminate one another's stage measurements, and PCIe timers can report USB activity despite being documented to report zero.

## Issue Context
Associate transfer accounting with each `UsbTransport` or adapter, or inject the relevant transport-specific counter into `InitTimer`, so each timer snapshots only its own instance's activity. Preserve a zero-valued source for PCIe timers and count transfers consistently with the documented metric semantics.

## Fix Focus Areas
- src/InitTimer.h[25-45]
- src/UsbXferCount.h[1-14]
- src/UsbTransport.h[55-73]
- src/UsbTransport.h[212-237]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context sources
Review mode: 🧠 Deep: This is a high-risk, bug-dense USB asynchronous transport change spanning callback lifetimes, cancellation/teardown, batching contracts, initialization sequencing, and RF behavior across many independent code paths.

Grey Divider

Tip of the day
💡 Did you know, you can commit Qodo's fix in one click with committable suggestions (GitHub & GitLab)

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread src/jaguar3/CLAUDE.md
Comment on lines +46 to +48
in submission order, so `UsbTransport` queues writes asynchronously inside
a `write_batch_begin/end` scope and only reads (submitted behind the queue,
waited on their own completion), bulk transfers and `flush_writes` wait.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

1. Transport guidance can drift 📘 Rule violation ⚙ Maintainability

src/jaguar3/CLAUDE.md restates the batching API's ordering, wait, threading, and delay-flush
contract instead of referring to IRtlTransport and its implementation documentation. A later
transport-contract change can update the authoritative headers without updating this backend guide,
leaving Jaguar3 maintainers with conflicting instructions.
Agent Prompt
## Issue description
The Jaguar3 guidance duplicates the pipelined-write API contract already documented in the transport headers.

## Issue Context
Keep only Jaguar3-specific bring-up facts here and refer readers to the authoritative transport interface for batching semantics, ordering, waits, and threading requirements.

## Fix Focus Areas
- src/jaguar3/CLAUDE.md[39-55]
- src/RtlTransport.h[71-83]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Comment thread src/UsbTransport.cpp
Comment on lines +505 to +513
/* The event loop itself is gone (a yanked device reports the error
* immediately, so the turns above cost nothing). Leave the slots
* submitted and off the free list; the destructor leaks them. */
_aw_errors += _aw_inflight;
_aw_abandoned = true;
_logger->error("USB: {} pipelined transfer(s) could not be reaped; "
"their slots are retired for this session",
_aw_inflight);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

2. Usb teardown can access freed memory 🐞 Bug ☼ Reliability

flush_writes() can mark submitted control transfers abandoned and return, while each unreaped
AsyncWrite retains a raw UsbTransport *self that async_write_cb() uses to update transport
counters and its free-list vector. If cancellation callbacks cannot be pumped before destruction but
a shared live libusb context later dispatches the transfer, the leaked slot survives while its
transport does not, and the callback dereferences freed transport state.
Agent Prompt
## Issue description
Abandoned asynchronous control transfers retain a callback pointer to `UsbTransport`, but the destructor may complete while libusb still owns those transfers. A later callback can then access destroyed transport counters and free-list state.

## Issue Context
Leaking an unreaped transfer slot preserves its transfer and buffer memory, but it does not preserve the `UsbTransport` referenced by `AsyncWrite::self`. Make callback-owned bookkeeping independent of the transport lifetime, or otherwise guarantee that every submitted transfer is reaped and its callback cannot execute after transport destruction, including when another adapter sharing the process continues pumping the libusb context.

## Fix Focus Areas
- src/UsbTransport.cpp[338-355]
- src/UsbTransport.cpp[404-420]
- src/UsbTransport.cpp[486-515]
- src/UsbTransport.h[107-140]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Comment thread src/UsbTransport.cpp
Comment on lines +383 to +390
if (_aw_all.empty()) {
for (int i = 0; i < kAsyncWriteDepth; ++i) {
auto *w = new AsyncWrite{};
w->t = libusb_alloc_transfer(0);
w->self = this;
_aw_all.push_back(w);
_aw_free.push_back(w);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

3. Low-memory bring-up can crash usb setup 🐞 Bug ☼ Reliability

write_batch_begin() inserts each AsyncWrite into both pool vectors without checking whether
libusb_alloc_transfer(0) returned a null t pointer. Under allocation pressure during Jaguar3 TX
initialization, the next batched read or write passes that null transfer to the fill and submit path
instead of falling back to synchronous bring-up or returning a controlled error.
Agent Prompt
## Issue description
The asynchronous slot pool can accept an `AsyncWrite` whose libusb transfer allocation failed, leaving a null transfer for later fill and submission. Do not enter batch mode with an unusable slot.

## Issue Context
Build the pool transactionally and validate every `libusb_alloc_transfer(0)` result before inserting or exposing a slot. If any allocation fails, release all partially allocated batch slots and leave batching disabled so register access falls back to the synchronous path, or return a controlled error. Other transfer-allocation paths in this transport already check for null before filling or submitting a transfer.

## Fix Focus Areas
- src/UsbTransport.cpp[376-394]
- src/UsbTransport.cpp[439-469]
- src/UsbTransport.cpp[518-539]
- src/UsbTransport.cpp[608-627]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Comment thread src/InitTimer.h
Comment on lines +44 to +45
static uint64_t xfers() {
return devourer::usb_ctrl_xfers().load(std::memory_order_relaxed);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remediation recommended

4. Timing records mix adapters' usb work 🐞 Bug ◔ Observability

InitTimer computes each stage's xfers delta from a process-wide usb_ctrl_xfers atomic instead
of a counter associated with the timer's transport or adapter. When adapters initialize concurrently
or any USB worker performs control transfers during the checkpoint interval, those transfers are
attributed to unrelated stages and can also reach PCIe timer metrics that are documented to remain
zero.
Agent Prompt
## Issue description
`init.timing.xfers` is intended to describe the control-transfer work performed by a single initialization stage, but `InitTimer` derives it from a process-wide USB counter. Concurrent adapters or workers therefore contaminate one another's stage measurements, and PCIe timers can report USB activity despite being documented to report zero.

## Issue Context
Associate transfer accounting with each `UsbTransport` or adapter, or inject the relevant transport-specific counter into `InitTimer`, so each timer snapshots only its own instance's activity. Preserve a zero-valued source for PCIe timers and count transfers consistently with the documented metric semantics.

## Fix Focus Areas
- src/InitTimer.h[25-45]
- src/UsbXferCount.h[1-14]
- src/UsbTransport.h[55-73]
- src/UsbTransport.h[212-237]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

@josephnef josephnef left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mechanism is sound and the reasoning behind it is the strongest part of the PR — never zeroing _aw_inflight by hand, retiring unreapable slots rather than handing libusb a dangling transfer, and WriteBatchScope's destructor closing the batch on a throw are all correct, and those comments earn their length. EP0 submission ordering does make the read-behind-writes trick safe, and the tx_async/tx_sync/write_bytes flushes cover the bulk paths.

I'd still hold it. Two of the four substantive issues are cross-variant validation gaps rather than logic errors, which on a bring-up-critical path is the expensive kind: every number in this PR is one 8812EU, and two of the changes ship to 8822C/8812CU as well.

1. The 8822C settle delays don't flush (inline on Halrf8822e.cpp). Halrf8822c::delay_ms is still static and untouched, while the batch scope covers dac_calibrate() and run_iqk() on both variants.

2. The write-only RF-table load is variant-agnostic (inline on HalJaguar3.cpp). The argument is airtight given readback 0 — an RMW that reads 0 writes exactly data, so the two are bit-identical — so the entire risk is whether 8822C's direct window behaves the same, and that's unmeasured.

3. _aw_abandoned is set but never acted on mid-batch (inline on UsbTransport.cpp).

4. libusb_alloc_transfer(0) isn't null-checked (inline).

The rebase is not mechanical. The PR is CONFLICTING. Post-#415 there is no IRtlTransportRtlAdapter forwards to devourer::ITransport (src/Transport.h), so the three virtuals have to land there, and the IRtlTransport::write_batch_begin doc references in RtlAdapter.h, HalJaguar3.cpp and src/jaguar3/CLAUDE.md need retargeting.

Validation, by the repo's own bar. Neither the pipelining nor the RF-table rewrite has an on-air or regress.py run, and #417 changes how the RF radio tables are programmed on both dies. I'd want a 2x2 on an 8822EU and an 8822CU, plus tests/tx_teardown_asan.sh over the new drain/cancel paths, before this merges — the description already flags that those haven't been run, which is the right call, but it's also the reason to hold.

Everything else is inline and minor.

std::this_thread::sleep_for(std::chrono::microseconds(us));
}
void Halrf8822e::delay_ms(uint32_t ms) {
_device.flush_writes(); /* the settle time must follow the writes */

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This flush landed on the 8822E calibration only. Halrf8822c::delay_ms (Halrf8822c.h:114) is still static and so structurally cannot flush — and WriteBatchScope in InitWrite covers _cal->dac_calibrate() and _hal.run_iqk() on both variants. Halrf8822c.cpp is full of the same bb_write(...); delay_ms(1..10) settle pairs (188, 957, 1110-1138, 1180-1206, ...).

If the flush is load-bearing here it is load-bearing there; if it isn't, this one is noise. Either way the 8822C DACK/IQK path now runs pipelined with no measurement behind it — every number in the PR is 8812EU. Make it non-static and flush, or say explicitly why 8822C is exempt.

* no-op that cost a synchronous read per entry -- half the RF table
* stage, ~80 ms on the ssc338q. Write-only also pipelines
* (IRtlTransport::write_batch_begin). */
_device.rtw_write32(static_cast<uint16_t>(base + ((addr & 0xff) << 2)),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rf_writer is variant-agnostic — the same lambda walks radioA/radioB for 8822C and 8822E alike (:1000-1001), so this drops the MASK20BITS RMW on 8812CU/8822CU too.

The argument itself is airtight given readback 0: an RMW whose read returns 0 writes exactly data, so the old and new writes are bit-identical. The whole risk is therefore the premise, and the histogram is one 8812EU. Cheap to close either way — rerun the same 1540-entry histogram on an 8822CU, or gate the plain write on _variant == C8822E until someone does and leave 8822C on phy_set_bb_reg.

Comment thread src/UsbTransport.cpp
* immediately, so the turns above cost nothing). Leave the slots
* submitted and off the free list; the destructor leaks them. */
_aw_errors += _aw_inflight;
_aw_abandoned = true;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_aw_abandoned is only ever read in write_batch_begin, so setting it here doesn't affect the batch that is currently open. _batch stays true, and the retired slots keep inflight == true, so _aw_inflight > 0 forever.

Every subsequent register access then walks async_take_slot -> async_wait_progress (8 x 250 ms) -> flush_writes (another ~2 s plus the cancel turns) -> nullptr. That's roughly 4 s per register for the remainder of a bring-up that issues thousands of them.

The trigger is narrow — USB_TIMEOUT is 500 ms so libusb normally completes stuck transfers itself, and a genuinely dead loop fails async_wait_progress immediately — but the fix is one line: set _batch = false here so the batch degrades to synchronous for the rest of the session, which is what the comment above already describes as the intent.

Comment thread src/UsbTransport.cpp
while (_aw_inflight > 0) {
if (async_wait_progress())
continue;
/* Stuck queue (~2 s without a completion): cancel what is still submitted

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: with USB_TIMEOUT at 500 ms (UsbTransport.h:28), libusb times a stuck transfer out long before 2 s and completes it through the callback. This path is really only reachable when the event loop isn't being pumped at all — which is what the code below handles correctly, but the "~2 s without a completion" framing points at the wrong cause.

Comment thread src/UsbTransport.cpp
if (_aw_all.empty()) {
for (int i = 0; i < kAsyncWriteDepth; ++i) {
auto *w = new AsyncWrite{};
w->t = libusb_alloc_transfer(0);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

libusb_alloc_transfer can return NULL; w->t then flows straight into libusb_fill_control_transfer in async_write/async_read. OOM-only, but a one-line guard here (bail out of the batch, leave _batch false) costs nothing.

Comment thread src/UsbTransport.h
if (async_read(static_cast<uint16_t>(addr & 0xFFFF),
static_cast<uint16_t>(addr >> 16), &data, sizeof(data)))
return data;
return 0xFFFFFFFFu;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The batch path returns the 0xFFFFFFFF sentinel silently while the batch path in ctrl_read below logs and throws. That asymmetry is pre-existing between read32_wide and ctrl_read, but a failed pipelined read is a new and more interesting failure than a failed synchronous one — worth an _logger->error here so it isn't indistinguishable from a register that genuinely reads all-ones.

Comment thread src/RtlTransport.h
* Single-threaded by contract: open a batch only while no other thread
* touches the transport (the Jaguar3 InitWrite/Init bring-up), and close
* it before any worker thread starts. Defaults are no-ops (PCIe). */
virtual void write_batch_begin() {}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebase note: after #415 (90e1cad) there is no IRtlTransportRtlAdapter forwards to devourer::ITransport in src/Transport.h, so these three virtuals need to land there instead, and the IRtlTransport::write_batch_begin references in RtlAdapter.h, HalJaguar3.cpp and src/jaguar3/CLAUDE.md need retargeting. Flagging because it's a real conflict resolution, not a textual one.

Comment thread src/UsbXferCount.h
#include <cstdint>

namespace devourer {
inline std::atomic<uint64_t> &usb_ctrl_xfers() {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth stating here that the counter is process-wide while InitTimer is per-device: two adapters brought up in one process cross-attribute their xfers. The demos are one-device-per-process so it doesn't bite today, but the field is going into docs/logging.md as "the transfers the stage spent", which is stronger than what this can promise.

@@ -1,3 +1,4 @@
#include "InitTimer.h"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this puts the module's own header second. Convention everywhere else in the tree is HalJaguar3.h first (it's what proves the header is self-contained), then the rest. Same in RtlJaguar3Device.cpp:1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants