fix(async): complete statusless AT descriptors after a bus reset - #89
Open
gly11 wants to merge 13 commits into
Open
fix(async): complete statusless AT descriptors after a bus reset#89gly11 wants to merge 13 commits into
gly11 wants to merge 13 commits into
Conversation
The pin added earlier in this branch only bound CI. build.sh still ran whatever xcodegen was on PATH, and it regenerates ASFW.xcodeproj on every non-test-only build. Homebrew currently ships 2.46.0 while the pin was 2.45.4, and their output is not byte-identical -- 2.46.0 reorders the pbxproj `targets` array. So a single ./build.sh on a brew-current machine produces a project the drift check rejects, and that check's error message tells you to regenerate, which reproduces the same diff. Move the version and its SHA-256 into .xcodegen-version and have both the workflow and build.sh read it, so the two can no longer disagree. build.sh now stops with an actionable error when the installed version differs, rather than warning and skipping regeneration: a build silently missing a newly added source file is much harder to diagnose than a version mismatch. --no-xcodegen builds the committed project as-is for machines that can't install the pin. Bump the pin to 2.46.0 to match what Homebrew ships today, and regenerate ASFW.xcodeproj with it. The only change is the `targets` array ordering, which has no semantic effect. Document how to install the pinned release in README -- the same download, pin, and checksum CI uses -- since "install the pinned version" isn't actionable without it.
InspectChainTail() returns Pending for a recognized OUTPUT_MORE_IMMEDIATE +
OUTPUT_LAST chain whose tail status is still zero, and ScanCompletion() then
returns without consulting the orphan path. That is correct while the context
is running, but a quiesced context will never write a status, so the ring head
stayed pinned to the chain forever.
Gate the Pending result on the context still being live. run==0 && active==0
falls through to HandlePendingDescriptor(), restoring the drain that existed
before this branch was introduced.
The check is deliberately narrower than IsOrphanedDescriptor(): its second
clause (commandPtr != headIOVA) also matches a live chain whose command pointer
has advanced to a pending OUTPUT_LAST, which is the false positive the Pending
result was added to fix. Only the run/active bits are consulted here, so both
cases keep working.
Both references drain unconditionally once the context is stopped, rather than
inferring liveness during the scan:
- Linux gates handle_at_packet()'s early return on !ctx->flushing and calls
at_context_flush() after context_stop() (ohci.c:1364, 2000-2010), citing
OHCI 1.2 draft clause 7.2.3.3 — hardware may leave unsent packets in the
AT queues for software to drain.
- AppleFWOHCI 5.5.9 calls resetDMA() after stopDMA() in handleBusResetInt()
(symbol offsets 0x5fae-0x5fd2); resetDMA() frees every pending ATxElement
regardless of status.
Apple can also afford to keep scanning past a zero-status element because its
pending set is a linked list; a single head cursor cannot. Correct the
ScanCompletion() doc comment accordingly. An explicit flush entry point is the
real fix and is left to a follow-up; this keeps the ring releasable meanwhile.
BusResetCoordinator::StopFlushAT() already stops the AT contexts and then calls FlushATContexts(), matching the ordering both references use. But the flush was a plain DrainTxCompletions(), which only harvests descriptors hardware actually finished. Packets the reset left unsent keep a zero xferStatus forever, so they held the ring head and their transactions completed only by timing out. Add ATContextBase::FlushScope. While held, ScanCompletion() reports a zero-status descriptor as evt_flushed instead of treating it as pending, and the existing pipeline does the rest: TransactionCompletionHandler already maps kEvtFlushed to kIOReturnAborted, and DrainTxCompletions() already routes completions to the tracker. FlushATContexts() opens the scope around the drain. This mirrors Linux, which does not use a separate drain routine either — it sets ctx->flushing and re-runs the ordinary handle_at_packet() body (ohci.c:1331-1343, 1364), then calls at_context_flush() after context_stop() (ohci.c:2000-2010). Linux reports these as RCODE_GENERATION, "the same error as when we try to use a stale generation count" (ohci.c:1387-1393); kIOReturnAborted is this driver's equivalent. AppleFWOHCI reaches the same end state differently, via resetDMA() after stopDMA() in handleBusResetInt() (symbol offsets 0x5fae-0x5fd2), which frees every pending ATxElement regardless of status. OHCI 1.2 draft clause 7.2.3.3 is why both must do this at all. Drop the IsContextQuiesced() escape added in ca86043. It was the right fix while no flush existed, but it now races the flush it was standing in for: the watchdog drain runs on a timer and can land between StopATContextsOnly() and FlushATContexts(), where it would consume the descriptors through the orphan path and emit no completion at all — leaving exactly the timeouts this commit removes. Holding the chain until the flush is the single path now.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f916716dd7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ContextBase declared its own ACTIVE mask as 1u << 13. The bit is 0x0400
(bit 10) — the value OHCIConstants.hpp already defines and static_asserts,
and the value Linux uses as CONTEXT_ACTIVE (ohci.c:257). Bit 13 is reserved,
so IsActive() has always reported false.
Three callers were silently degraded:
- ATManager::clearRunAndPoll_() broke out of its 250-iteration quiesce poll
on the first pass, so clearing RUN was never followed by any wait.
- The "P1_ARM while ACTIVE after clearRun poll gave up" anomaly log guarding
OHCI §3.1.1 (programming CommandPtr on an active context) could not fire.
- DmaContextManagerBase::PollActiveUs() always timed out. It has no callers,
so nothing observed that.
Use Driver::kContextControlActiveBit and kContextControlRunBit directly rather
than redeclaring either. Per CLAUDE.md, OHCI register constants live only in
OHCIConstants.hpp; this header having private copies is what let one of them
drift from the value the rest of the driver uses.
Addresses the P1 raised on mrmidi#89. stopAT() returned as soon as the request context failed to stop, leaving the response context running while callers treated AT as quiesced. That was harmless when FlushATContexts() merely drained finished descriptors, but this PR made the flush rewrite descriptor words, so a live response context could have the chain it is traversing zeroed underneath it. Both references issue their stops unconditionally: Linux calls context_stop() on request and response back to back (ohci.c:1999-2000), and AppleFWOHCI calls stopDMA() on both in handleBusResetInt() (symbol offsets 0x5fae, 0x5fba). Do the same and report the first failure after attempting both. Neither reference can prove its stop succeeded — OHCI §7.2.3 lets a context hold ACTIVE past the stop timeout, and both flush regardless. Check ACTIVE per context before opening a FlushScope instead, and log loudly when one is skipped. A context that is still active has packets in flight rather than stranded, so leaving its ring for the next scan is correct; deferring the whole flush would reintroduce the wedge this PR removes.
gly11
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
evt_flushed, instead of waiting for a status hardware can no longer writeATContextBase::FlushScope, opened byFlushATContexts()around the existing completion drainACTIVEIsContextQuiesced()escape added in fix(async): complete block writes from OUTPUT_LAST status #84, which this supersedes and would otherwise raceRoot cause
BusResetCoordinator::StopFlushAT()already stops the AT contexts and then callsFlushATContexts(), which is the ordering both reference stacks use. But the flush was a plainDrainTxCompletions(), and that only harvests descriptors for which hardware wrote a completion status.OHCI 1.2 draft §7.2.3.3 (document p. 7-14) permits optional controller behavior that updates only descriptors whose final status is known after a bus reset. Outstanding AT descriptors may therefore retain
xferStatus == 0. That does not prove the corresponding packet was never transmitted, but after the context has stopped andACTIVEis clear, hardware can no longer update the descriptor. The scanner treated it as merely pending, so it held the ring head and its transaction failed only by timing out.Linux uses the same flush-mode scan shape:
at_context_flush()setsctx->flushingand re-runs the ordinary handler aftercontext_stop(). A zero/no-status entry is then completed asRCODE_GENERATION; Linux does not writeevt_flushedinto the descriptor.FlushScopefollows that scan pattern while using this driver's existingkEvtFlushed→kIOReturnAbortedcompletion mapping to produce the equivalent generation-cancel outcome. Seeohci.c:1331-1343,1354-1421, and1999-2010.AppleFWOHCI 5.5.9 reaches the same end state by a different route:
handleBusResetInt()callsresetDMA()afterstopDMA()(symbol offsets0x5fae-0x5fd2), andresetDMA()frees every pendingATxElementregardless of status. It can also keep scanning past a zero-status element because its pending set is a linked list; a single ring head cursor cannot, which is why the scoped flush flag fits this ring implementation.Impact
Changes what outstanding async transactions observe across a bus reset: an abort at reset time rather than a later timeout. This is bus-policy-layer behavior and applies to every device on the bus, not only the SBP-2 path that motivated #84.
Removing #84's
IsContextQuiesced()escape is deliberate. It was correct while no flush existed, but the watchdog drain runs on a timer and can land betweenStopATContextsOnly()andFlushATContexts(), where it would consume the descriptors through the orphan path and emit no completion at all. Holding the chain until the flush is now the single path.The flush is gated independently for each AT context. If a context still reports
ACTIVEafter the stop attempt, it is skipped so software cannot rewrite descriptors the controller may still be traversing.Validation
Host and build
ATContextCompletionTests: 9/9 passedAsyncSubsystemBusReset.cppis not part of the CMake test build; verified separately withclang++ -fsyntax-onlyandASFW_HOST_TESTgit diff --checkpassedHardware
Not validated on hardware. The reset path needs a device present to exercise, and the observable difference is whether outstanding transactions abort at reset or time out afterwards.
FlushATContexts()now logsdrained N AT completion(s) during bus-reset flushwhenN > 0. This confirms drain activity but intentionally does not claim that every drained completion was statusless or that its packet was never transmitted.Dependency and merge order
Stacked on #84, which is stacked on #88. Targets
dev.Until both merge, Files changed will also show their commits. The PR-specific review scope is the top four commits and these five files:
ASFWDriver/Async/Contexts/ATContextBase.hppASFWDriver/Async/Contexts/ContextBase.hppASFWDriver/Async/AsyncSubsystemBusReset.cppASFWDriver/Async/Engine/ContextManager.cpptests/async/ATContextCompletionTests.cppIntended merge order: #88 → #84 → this.