Skip to content

fix(isoch): enforce RX completion ordering and clear stopped events - #220

Open
bbatarelo wants to merge 1 commit into
mrmidi:mainfrom
bbatarelo:codex/isoch-rx-completion-ordering
Open

bbatarelo wants to merge 1 commit into
mrmidi:mainfrom
bbatarelo:codex/isoch-rx-completion-ordering

Conversation

@bbatarelo

Copy link
Copy Markdown

Problem and change

RX retirement currently accepts a changed residual count even when transfer status is zero. Residual progress does not establish completion, so this can expose an incomplete payload and rearm a descriptor too early. Require nonzero transfer status, order payload reads after observing completion, and finish those reads before clearing descriptor status.

RX/TX stop also leaves a context's pending event behind. After ACTIVE clears, acknowledge only that context's event. Preserve sibling events and do not acknowledge on timeout or when the controller is unavailable.

Prior discussion

No linked issue. This is a focused correction to existing completion and stop paths, with regression tests; it introduces no new streaming architecture or device support.

Architecture and scope

Changes are confined to shared barriers, the isoch DMA interface, RX retirement, RX/TX stop, and their tests. The transport remains payload-opaque. The existing completion loop and stop/quiesce checks are retained.

RME support, bus-root policy, app/signing work, and packet-tracing infrastructure are outside this PR.

Testing and evidence

  • 87 host tests passed across the IsochReceiveContextTests, IsochTxDmaRingTests, and IsochServiceTxPreparationTests targets, using upstream 787683e5 plus this exact eight-file patch.
  • RX fixtures use the production DMA allocator and cover residual-only progress, completion/payload/rearm ordering, repeated wraps, full/empty states, tail handling, and hot append.
  • Stop tests cover acknowledgement after quiescence, preserved sibling events, timeout, and unavailable-controller paths. git diff --check passes.
  • Behavioral comparison: Linux at dba0bfded433d412506eb674d6af77cdca82e259, packet-per-buffer completion and CPU synchronization, payload consumption before requeue, and per-context event acknowledgement at drivers/firewire/ohci.c:2094-2107. References were used for behavior, without copying implementation.
  • No production build, installation, or hardware run was performed for this isolated upstream patch. Host tests cannot verify actual DMA ordering, DriverKit lifecycle, or controller behavior.

Regression risks

These paths are shared by isoch users across device families. The additional full barriers can affect RX timing; actual hardware performance and other controllers remain untested.

This establishes completion observation and CPU access ordering, but the circular RX program can still lap the consumer. It is not a complete DMA ownership redesign or evidence that the observed Fireface sample losses are resolved.

Wait for transfer status before consuming RX descriptors and order
payload reads before descriptor rearming.

Clear only each quiesced RX/TX context's pending event. Cover
incomplete transfers, ring wraps and stop acknowledgement ordering.
@mrmidi

mrmidi commented Oct 22, 2026

Copy link
Copy Markdown
Owner

Hi. Thanks for contributing. That's sounds reasonable to be honest. I'll verify on hardware a bit later.

@bbatarelo

Copy link
Copy Markdown
Author

Glad I can help. It took me some time to find thunderbolt to firewire adapter and a decent RME interface. This is a prequel to RME Fireface 800 support.

@mrmidi

mrmidi commented Oct 23, 2026

Copy link
Copy Markdown
Owner

Thanks for the work on this, Bruno. After a closer look, I can't merge it as is. These changes may fix nothing: no symptom, no log, no hardware before/after. It's new code for the sake of new code, in a receive path every device shares.

The DriverKit point is correct: OSSynchronizeIO is dmb st on Apple Silicon (IOLib.h:235). I think Apple picked it on purpose, because it's the cheapest instruction that does the one job OSSynchronizeIO exists for: telling the device something. A driver handing work to a device writes in this order:

write payload        (memory)
write descriptor     (memory)
--- OSSynchronizeIO ---
write doorbell / RUN (device register)

The only rule is that the device must not see the doorbell before the payload and descriptor exist. That's ordering writes against writes, which is exactly what dmb st does. A full barrier (dmb sy or dsb) would also stall the CPU on unrelated reads, for no benefit. That's also why a dmb sy on every received packet is the wrong fix.

Reading it back on the receive side is a different job: it needs reads ordered against reads. So yes, there's a gap in theory. But it can only hit the newest packet in a drain, within a window of nanoseconds, and nobody has seen it happen. That makes it an issue to track, not a change to make.

The other parts don't hold either. Start already clears the context's own event before unmasking (IsochReceiveContext.cpp:113, IsochTransmitContext.cpp:348), which is where Linux does it. And the pre-rearm barrier assumes Linux's append-based ring, while ours is circular, so it orders nothing.

As I wrote in my email: this looks like an AI agent's fix that sees one problem and not the whole stack.

You mention sample losses on your Fireface 800. How do you know? Is it audible (clicks, dropouts in a recording), or does it come from telemetry or logs? That matters, because telemetry isn't free: logging, tracing and polling counters on the isoch path take CPU time and can shift timing enough to cause the very losses they report. A counter can also be wrong itself. Please open an issue with the device, the macOS and ASFW build, how you reproduce it, and how you detected the loss.

I'm leaving this PR open for now. To move forward, it needs an observed symptom and a hardware run showing it gone.

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