From dfdd9fee2acb4a26ebce0807234dd113f6ea410a Mon Sep 17 00:00:00 2001 From: gly11 Date: Sun, 26 Jul 2026 18:46:17 +0800 Subject: [PATCH 01/13] ci: pin XcodeGen release for project verification --- .github/workflows/build-and-test.yml | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/.github/workflows/build-and-test.yml b/.github/workflows/build-and-test.yml index 9e6ae591c..3459ecd0e 100644 --- a/.github/workflows/build-and-test.yml +++ b/.github/workflows/build-and-test.yml @@ -26,19 +26,29 @@ jobs: # ASFW.xcodeproj is generated from project.yml (XcodeGen) and committed. # Fail if they drifted apart — someone edited project.yml or added source # files without regenerating, or hand-edited the pbxproj. If this fails - # after an xcodegen version bump changed the output format, regenerate - # locally with the new version and commit. + # after the pinned xcodegen version changes, regenerate locally with the + # same version and commit. - name: Verify ASFW.xcodeproj matches project.yml (XcodeGen) + env: + XCODEGEN_VERSION: 2.45.4 + XCODEGEN_SHA256: 090ec29491aad50aec10631bf6e62253fed733c50f3aab0f5ffc86bc170bdbef run: | - brew install xcodegen - xcodegen --version - xcodegen generate --quiet + XCODEGEN_ARCHIVE="$RUNNER_TEMP/xcodegen.zip" + XCODEGEN_ROOT="$RUNNER_TEMP/xcodegen-release" + curl --fail --location --retry 3 \ + --output "$XCODEGEN_ARCHIVE" \ + "https://github.com/yonaskolb/XcodeGen/releases/download/${XCODEGEN_VERSION}/xcodegen.zip" + echo "${XCODEGEN_SHA256} ${XCODEGEN_ARCHIVE}" | shasum -a 256 -c - + ditto -x -k "$XCODEGEN_ARCHIVE" "$XCODEGEN_ROOT" + XCODEGEN_BIN="$XCODEGEN_ROOT/xcodegen/bin/xcodegen" + "$XCODEGEN_BIN" --version + "$XCODEGEN_BIN" generate --quiet # Diff only the pbxproj: scheme files are cosmetically rewritten by # any open Xcode (version attr, BuildableName flavor, empty blocks), # so they flip-flop between xcodegen and Xcode styles; their semantic # content comes from project.yml either way. if ! git diff --exit-code --stat -- ASFW.xcodeproj/project.pbxproj project.yml; then - echo "::error::ASFW.xcodeproj is out of sync with project.yml. Run 'xcodegen generate' locally and commit the regenerated project (see README → Building)." + echo "::error::ASFW.xcodeproj is out of sync with project.yml. Regenerate it with XcodeGen ${XCODEGEN_VERSION} and commit the result (see README → Building)." exit 1 fi From da9e0d39968bbb713f1f4cd6dc641eae79f503d3 Mon Sep 17 00:00:00 2001 From: gly11 Date: Sun, 26 Jul 2026 19:13:18 +0800 Subject: [PATCH 02/13] ci: defer GoogleTest discovery until test execution --- tests/CMakeLists.txt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index d0eb481a0..ba76bfb64 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -50,6 +50,8 @@ if (NOT GTest_FOUND) endif() include(GoogleTest) +# Avoid parallel post-build discovery racing on empty GoogleTest JSON output. +set(CMAKE_GTEST_DISCOVER_TESTS_DISCOVERY_MODE PRE_TEST) # Define common test settings interface library add_library(asfw_test_common_interface INTERFACE) From 3e19cb1498fcae18ca16cabd121e4b4b4ed7bcc7 Mon Sep 17 00:00:00 2001 From: gly11 Date: Sun, 26 Jul 2026 21:38:43 +0800 Subject: [PATCH 03/13] fix(build): preserve Swift test failures in quiet mode --- build.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/build.sh b/build.sh index 08ec15ab5..48c7a1440 100755 --- a/build.sh +++ b/build.sh @@ -231,7 +231,7 @@ run_swift_tests() { if $VERBOSE; then xcodebuild "${XCODEBUILD_ARGS[@]}" 2>&1 else - xcodebuild "${XCODEBUILD_ARGS[@]}" 2>&1 | grep -E '(Test Case|passed|failed|error:)' || true + xcodebuild "${XCODEBUILD_ARGS[@]}" 2>&1 | grep -E '(Test Case|passed|failed|error:)' fi local test_status=${PIPESTATUS[0]} set -e From 94b9fd31d8c569e247f614f3f863ec082f72b3c4 Mon Sep 17 00:00:00 2001 From: gly11 Date: Mon, 27 Jul 2026 02:06:53 +0800 Subject: [PATCH 04/13] ci: enforce the XcodeGen pin in build.sh, not just CI 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. --- .github/workflows/build-and-test.yml | 6 ++-- .xcodegen-version | 18 ++++++++++++ ASFW.xcodeproj/project.pbxproj | 2 +- README.md | 42 +++++++++++++++++++++++----- build.sh | 38 ++++++++++++++++++++++--- 5 files changed, 91 insertions(+), 15 deletions(-) create mode 100644 .xcodegen-version diff --git a/.github/workflows/build-and-test.yml b/.github/workflows/build-and-test.yml index 3459ecd0e..6c67bf1d7 100644 --- a/.github/workflows/build-and-test.yml +++ b/.github/workflows/build-and-test.yml @@ -29,10 +29,10 @@ jobs: # after the pinned xcodegen version changes, regenerate locally with the # same version and commit. - name: Verify ASFW.xcodeproj matches project.yml (XcodeGen) - env: - XCODEGEN_VERSION: 2.45.4 - XCODEGEN_SHA256: 090ec29491aad50aec10631bf6e62253fed733c50f3aab0f5ffc86bc170bdbef run: | + # Single source of truth, shared with build.sh, so a contributor's + # locally installed xcodegen can never disagree with this check. + . ./.xcodegen-version XCODEGEN_ARCHIVE="$RUNNER_TEMP/xcodegen.zip" XCODEGEN_ROOT="$RUNNER_TEMP/xcodegen-release" curl --fail --location --retry 3 \ diff --git a/.xcodegen-version b/.xcodegen-version new file mode 100644 index 000000000..dbafa5344 --- /dev/null +++ b/.xcodegen-version @@ -0,0 +1,18 @@ +# XcodeGen release pinned for generating ASFW.xcodeproj from project.yml. +# +# Both CI (.github/workflows/build-and-test.yml) and build.sh read this file, so +# the version that regenerates the committed project is the same everywhere. +# Homebrew's xcodegen floats and its output is NOT byte-identical across +# releases (2.45.4 -> 2.46.0 reorders the pbxproj `targets` list), which is why +# the pin exists and why build.sh refuses to regenerate with a different one. +# +# Upgrading — do all three steps together, or CI's drift check will fail: +# 1. bump XCODEGEN_VERSION below +# 2. curl the release zip, put its `shasum -a 256` in XCODEGEN_SHA256 +# curl -fL -o /tmp/xcodegen.zip \ +# https://github.com/yonaskolb/XcodeGen/releases/download//xcodegen.zip +# shasum -a 256 /tmp/xcodegen.zip +# 3. regenerate ASFW.xcodeproj with that exact version and commit it alongside +# +XCODEGEN_VERSION=2.46.0 +XCODEGEN_SHA256=4d9e34b62172d645eed6457cac13fc222569974098ef4ee9c3368bedf0196806 diff --git a/ASFW.xcodeproj/project.pbxproj b/ASFW.xcodeproj/project.pbxproj index cbbb809ad..44ec75d46 100644 --- a/ASFW.xcodeproj/project.pbxproj +++ b/ASFW.xcodeproj/project.pbxproj @@ -3000,8 +3000,8 @@ projectDirPath = ""; projectRoot = ""; targets = ( - EE5A446B3669D0171F9606EE /* ASFW */, 0EB9A8DA75D08971084A440A /* ASFWDriver */, + EE5A446B3669D0171F9606EE /* ASFW */, 348672D607701677ABCB567C /* ASFWTests */, ); }; diff --git a/README.md b/README.md index 3503420c4..f168981c4 100644 --- a/README.md +++ b/README.md @@ -427,20 +427,48 @@ Build scripts or CMakeLists are for quick testing and creating compile_commands. ### Xcode project is generated (XcodeGen) `ASFW.xcodeproj` is generated from the root [`project.yml`](project.yml) with -[XcodeGen](https://github.com/yonaskolb/XcodeGen) (`brew install xcodegen`). -The generated project is committed, so plain checkouts (and CI) build without -XcodeGen installed — but **never edit the pbxproj or project settings in the -Xcode UI**; change `project.yml` instead. +[XcodeGen](https://github.com/yonaskolb/XcodeGen). The generated project is +committed, so plain checkouts (and CI) build without XcodeGen installed — but +**never edit the pbxproj or project settings in the Xcode UI**; change +`project.yml` instead. + +**The XcodeGen version is pinned** in [`.xcodegen-version`](.xcodegen-version), +because its output is *not* byte-identical across releases (2.45.4 → 2.46.0 +reorders the pbxproj `targets` list) and CI diffs the regenerated project +against the committed one. Homebrew's `xcodegen` floats, so install the pinned +release rather than `brew install xcodegen` — this is the same download, pin, +and checksum CI uses: + +```bash +. ./.xcodegen-version +curl --fail --location --retry 3 --output /tmp/xcodegen.zip \ + "https://github.com/yonaskolb/XcodeGen/releases/download/${XCODEGEN_VERSION}/xcodegen.zip" +echo "${XCODEGEN_SHA256} /tmp/xcodegen.zip" | shasum -a 256 -c - +ditto -x -k /tmp/xcodegen.zip /tmp/xcodegen-release +sudo /tmp/xcodegen-release/xcodegen/install.sh # -> /usr/local/bin/xcodegen +# no-sudo variant: install.sh takes a prefix, e.g. +# /tmp/xcodegen-release/xcodegen/install.sh "$HOME/.local" +``` + +If `xcodegen` isn't on your `PATH` at all, nothing above applies — `build.sh` +skips regeneration entirely and builds the committed project. The pin only +matters once you have *some* `xcodegen` installed, because `build.sh` +regenerates on every build; if that version doesn't match the pin it stops with +an error rather than writing a pbxproj CI would reject. Use `--no-xcodegen` to +build anyway. After **adding, removing, or renaming source files**, regenerate the project and commit it together with your change: ```bash -xcodegen generate # ./build.sh does this automatically when xcodegen is installed +xcodegen generate # ./build.sh does this automatically, and refuses to run + # if your xcodegen doesn't match the pin ``` -Output is deterministic — regenerating with no changes produces an identical -pbxproj. +Output is deterministic *for a given XcodeGen version* — regenerating with no +changes produces an identical pbxproj. If your machine has a different version +and you only need to build, pass `./build.sh --no-xcodegen` to use the committed +project as-is (added or removed sources will not be picked up). NOTE: You need an Apple Developer account (paid) and appropriate entitlements — or a free account plus SIP disabled — to build/load the driver on your machine. See Apple's documentation for details: https://developer.apple.com/documentation/driverkit/debugging-and-testing-system-extensions diff --git a/build.sh b/build.sh index 48c7a1440..cef264804 100755 --- a/build.sh +++ b/build.sh @@ -60,6 +60,10 @@ SWIFT_COVERAGE_LCOV="${BUILD_DIR}/swift_coverage.lcov" # See README "SCSI HBA — opt-in". ENABLE_SCSI=false +# Skip the XcodeGen regeneration in preflight() and build the committed project +# as-is. Escape hatch for a machine whose xcodegen differs from the pin. +NO_XCODEGEN=false + usage() { cat </dev/null 2>&1; then + # source globs pick up added/removed files; output is deterministic *for a + # given xcodegen version*, so this is a no-op when nothing changed. Falls + # through to the committed project when xcodegen isn't installed (e.g. CI + # runners, which use the pinned release directly). + if [[ -f "project.yml" ]] && ! $NO_XCODEGEN && command -v xcodegen >/dev/null 2>&1; then + # XcodeGen output is not byte-identical across releases, and CI diffs the + # regenerated pbxproj against the committed one. Regenerating with an + # unpinned version silently produces a project that CI will reject, so + # stop here instead — a build missing a newly added source file is much + # harder to diagnose than this message. + if [[ -f ".xcodegen-version" ]]; then + . ./.xcodegen-version + xcodegen_actual="$(xcodegen --version 2>/dev/null | awk '{print $NF}')" + if [[ -n "${XCODEGEN_VERSION:-}" && "$xcodegen_actual" != "$XCODEGEN_VERSION" ]]; then + err "xcodegen ${xcodegen_actual:-} is installed, but this repo pins ${XCODEGEN_VERSION}." + err "Regenerating would produce a pbxproj that CI's drift check rejects." + err "Either:" + err " - install the pinned release (copy-paste block in README -> Building ->" + err " 'Xcode project is generated (XcodeGen)'), or" + err " - re-run with --no-xcodegen to build the committed project as-is" + err " (added/removed source files will NOT be picked up)." + exit 1 + fi + fi log "Regenerating ${PROJECT_NAME}.xcodeproj from project.yml..." xcodegen generate --quiet || { err "xcodegen generate failed"; exit 1; } fi From 7538dba7ac60ccf892e83a22194e1d402d889e15 Mon Sep 17 00:00:00 2001 From: gly11 Date: Mon, 27 Jul 2026 02:23:39 +0800 Subject: [PATCH 05/13] ci: harden XcodeGen pin enforcement --- CLAUDE.md | 10 ++++++---- README.md | 21 +++++++++++++++------ build.sh | 35 ++++++++++++++++++++++------------- 3 files changed, 43 insertions(+), 23 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 3b0cbb464..054d891c9 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -14,10 +14,12 @@ Two components: **`ASFW.xcodeproj` is GENERATED from the root `project.yml` (XcodeGen).** Never edit the pbxproj or hand-tune settings in Xcode — change `project.yml` and run -`xcodegen generate` (`./build.sh` does it automatically when xcodegen is -installed). After adding/removing/renaming source files, regenerate and commit -the updated `ASFW.xcodeproj` together with `project.yml`. Output is -deterministic; the generated project stays committed so CI builds without +`xcodegen generate` with the version pinned in `.xcodegen-version` +(`./build.sh` checks an installed xcodegen automatically; use +`--no-xcodegen` only to build the committed project as-is). After +adding/removing/renaming source files, regenerate and commit the updated +`ASFW.xcodeproj` together with `project.yml`. Output is deterministic for the +pinned version; the generated project stays committed so CI builds without xcodegen. (`ADKVirtualAudioLab/` has its own separate `project.yml`.) **Primary build (Xcode — required for signing and producing `.dext`):** diff --git a/README.md b/README.md index f168981c4..50c9e5578 100644 --- a/README.md +++ b/README.md @@ -441,15 +441,24 @@ and checksum CI uses: ```bash . ./.xcodegen-version -curl --fail --location --retry 3 --output /tmp/xcodegen.zip \ +XCODEGEN_TMP="$(mktemp -d)" +XCODEGEN_PREFIX="$HOME/.local" +curl --fail --location --retry 3 \ + --output "$XCODEGEN_TMP/xcodegen.zip" \ "https://github.com/yonaskolb/XcodeGen/releases/download/${XCODEGEN_VERSION}/xcodegen.zip" -echo "${XCODEGEN_SHA256} /tmp/xcodegen.zip" | shasum -a 256 -c - -ditto -x -k /tmp/xcodegen.zip /tmp/xcodegen-release -sudo /tmp/xcodegen-release/xcodegen/install.sh # -> /usr/local/bin/xcodegen -# no-sudo variant: install.sh takes a prefix, e.g. -# /tmp/xcodegen-release/xcodegen/install.sh "$HOME/.local" +echo "${XCODEGEN_SHA256} ${XCODEGEN_TMP}/xcodegen.zip" | shasum -a 256 -c - +ditto -x -k "$XCODEGEN_TMP/xcodegen.zip" "$XCODEGEN_TMP/release" +mkdir -p "$XCODEGEN_PREFIX" +"$XCODEGEN_TMP/release/xcodegen/install.sh" "$XCODEGEN_PREFIX" +export PATH="$XCODEGEN_PREFIX/bin:$PATH" +hash -r +xcodegen --version ``` +The `PATH` update deliberately places the pinned binary before a Homebrew +installation. Add the same `export` command to your shell startup file if you +want future terminal sessions to use the pinned version. + If `xcodegen` isn't on your `PATH` at all, nothing above applies — `build.sh` skips regeneration entirely and builds the committed project. The pin only matters once you have *some* `xcodegen` installed, because `build.sh` diff --git a/build.sh b/build.sh index cef264804..fb98c1f91 100755 --- a/build.sh +++ b/build.sh @@ -136,19 +136,28 @@ preflight() { # unpinned version silently produces a project that CI will reject, so # stop here instead — a build missing a newly added source file is much # harder to diagnose than this message. - if [[ -f ".xcodegen-version" ]]; then - . ./.xcodegen-version - xcodegen_actual="$(xcodegen --version 2>/dev/null | awk '{print $NF}')" - if [[ -n "${XCODEGEN_VERSION:-}" && "$xcodegen_actual" != "$XCODEGEN_VERSION" ]]; then - err "xcodegen ${xcodegen_actual:-} is installed, but this repo pins ${XCODEGEN_VERSION}." - err "Regenerating would produce a pbxproj that CI's drift check rejects." - err "Either:" - err " - install the pinned release (copy-paste block in README -> Building ->" - err " 'Xcode project is generated (XcodeGen)'), or" - err " - re-run with --no-xcodegen to build the committed project as-is" - err " (added/removed source files will NOT be picked up)." - exit 1 - fi + if [[ ! -r ".xcodegen-version" ]]; then + err "xcodegen is installed, but the required .xcodegen-version pin is missing or unreadable." + err "Refusing to regenerate ${PROJECT_NAME}.xcodeproj with an unpinned version." + exit 1 + fi + unset XCODEGEN_VERSION + . ./.xcodegen-version + if [[ -z "${XCODEGEN_VERSION:-}" ]]; then + err ".xcodegen-version does not define XCODEGEN_VERSION." + err "Refusing to regenerate ${PROJECT_NAME}.xcodeproj with an unpinned version." + exit 1 + fi + xcodegen_actual="$(xcodegen --version 2>/dev/null | awk '{print $NF}')" + if [[ "$xcodegen_actual" != "$XCODEGEN_VERSION" ]]; then + err "xcodegen ${xcodegen_actual:-} is installed, but this repo pins ${XCODEGEN_VERSION}." + err "Regenerating would produce a pbxproj that CI's drift check rejects." + err "Either:" + err " - install the pinned release (copy-paste block in README -> Building ->" + err " 'Xcode project is generated (XcodeGen)'), or" + err " - re-run with --no-xcodegen to build the committed project as-is" + err " (added/removed source files will NOT be picked up)." + exit 1 fi log "Regenerating ${PROJECT_NAME}.xcodeproj from project.yml..." xcodegen generate --quiet || { err "xcodegen generate failed"; exit 1; } From 21ea8e27102c01448ef532f5aa4b8085536bc572 Mon Sep 17 00:00:00 2001 From: gly11 Date: Tue, 28 Jul 2026 18:28:00 +0800 Subject: [PATCH 06/13] ci: regenerate dev scheme with pinned XcodeGen --- .../xcschemes/ASFWDriver.xcscheme | 22 +++++++++++++------ 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/ASFW.xcodeproj/xcshareddata/xcschemes/ASFWDriver.xcscheme b/ASFW.xcodeproj/xcshareddata/xcschemes/ASFWDriver.xcscheme index 09c8b9bed..0d79cf893 100644 --- a/ASFW.xcodeproj/xcshareddata/xcschemes/ASFWDriver.xcscheme +++ b/ASFW.xcodeproj/xcshareddata/xcschemes/ASFWDriver.xcscheme @@ -1,10 +1,11 @@ + version = "1.7"> + buildImplicitDependencies = "YES" + runPostActionsOnFailure = "NO"> @@ -26,18 +27,21 @@ buildConfiguration = "Debug" selectedDebuggerIdentifier = "Xcode.DebuggerFoundation.Debugger.LLDB" selectedLauncherIdentifier = "Xcode.DebuggerFoundation.Launcher.LLDB" - shouldUseLaunchSchemeArgsEnv = "YES"> + shouldUseLaunchSchemeArgsEnv = "YES" + onlyGenerateCoverageForSpecifiedTargets = "NO"> + + + + + + From f277a87ac232c09d79eeef6479977226cea3e472 Mon Sep 17 00:00:00 2001 From: gly11 Date: Sun, 26 Jul 2026 03:34:09 +0800 Subject: [PATCH 07/13] fix(async): complete block writes from OUTPUT_LAST status --- ASFWDriver/Async/Contexts/ATContextBase.hpp | 73 ++++++++++++-- tests/async/ATContextCompletionTests.cpp | 106 ++++++++++++++++++++ tests/async/CMakeLists.txt | 8 ++ 3 files changed, 178 insertions(+), 9 deletions(-) create mode 100644 tests/async/ATContextCompletionTests.cpp diff --git a/ASFWDriver/Async/Contexts/ATContextBase.hpp b/ASFWDriver/Async/Contexts/ATContextBase.hpp index f394d9987..fc78bec6c 100644 --- a/ASFWDriver/Async/Contexts/ATContextBase.hpp +++ b/ASFWDriver/Async/Contexts/ATContextBase.hpp @@ -230,9 +230,9 @@ class ATContextBase : public ContextBase { /** * \brief Scan for completed descriptors and extract completion status. * - * Walks the descriptor ring from head index, checking xferStatus field - * for hardware completion. Extracts event code, timestamp, and tLabel - * on first completed descriptor found. + * Walks the descriptor ring from head index, checking the packet chain's + * OUTPUT_LAST xferStatus for hardware completion. Extracts event code, + * timestamp, and tLabel on the first completed packet found. * * \return TxCompletion if descriptor completed, std::nullopt if none ready * @@ -245,12 +245,13 @@ class ATContextBase : public ContextBase { * 2. Load head index (atomic acquire) * 3. If head == tail, ring is empty → return nullopt * 4. Read descriptor at head index - * 5. If xferStatus == 0, descriptor not yet completed → return nullopt - * 6. Extract event code from xferStatus[4:0] - * 7. Extract timestamp from timeStamp field - * 8. If OUTPUT_LAST_Immediate, extract tLabel from packet header - * 9. Advance head index: (head + N) % capacity, where N = descriptor block count - * 10. Unlock context, return TxCompletion + * 5. For OUTPUT_MORE, check the following OUTPUT_LAST completion status + * 6. If the packet's terminal xferStatus == 0, return nullopt + * 7. Extract event code from xferStatus[4:0] + * 8. Extract timestamp from timeStamp field + * 9. Extract tLabel from the immediate packet header + * 10. Advance head beyond the completed packet chain + * 11. Unlock context, return TxCompletion * * **Apple Pattern** * ChannelBundle::ScanNextATReqCompletion(): @@ -341,6 +342,7 @@ class ATContextBase : public ContextBase { size_t capacity) noexcept; [[nodiscard]] bool LoadScanState(ScanState& state) noexcept; void FetchScanDescriptor(const ScanState& state) noexcept; + [[nodiscard]] bool AdvanceToCompletedChainTail(const ScanState& state) noexcept; void HandlePendingDescriptor(const ScanState& state) noexcept; [[nodiscard]] bool IsOrphanedDescriptor(const ScanState& state, uint32_t& commandPtrAddr, @@ -605,6 +607,10 @@ std::optional ATContextBase::ScanCompletion() noexce return std::nullopt; } + if (state.xferStatus == 0 && AdvanceToCompletedChainTail(state)) { + continue; + } + if (state.xferStatus == 0) { HandlePendingDescriptor(state); unlock(); @@ -826,6 +832,55 @@ void ATContextBase::FetchScanDescriptor(const ScanState& state) no } } +template +bool ATContextBase::AdvanceToCompletedChainTail( + const ScanState& state) noexcept { + const uint16_t controlHi = static_cast( + state.desc->control >> HW::OHCIDescriptor::kControlHighShift); + const uint8_t command = static_cast( + (controlHi >> HW::OHCIDescriptor::kCmdShift) & 0xF); + if (command != HW::OHCIDescriptor::kCmdOutputMore) { + return false; + } + + const uint8_t key = static_cast( + (controlHi >> HW::OHCIDescriptor::kKeyShift) & 0x7); + const uint8_t precursorBlocks = + (key == HW::OHCIDescriptor::kKeyImmediate) ? 2 : 1; + const size_t tailIndex = + (state.headIndex + precursorBlocks) % state.capacity; + if (tailIndex == state.tailIndex) { + return false; + } + + ScanState tailState; + tailState.capacity = state.capacity; + tailState.headIndex = tailIndex; + tailState.desc = ring_->At(tailIndex); + if (!tailState.desc) { + return false; + } + tailState.isImmediate = HW::IsImmediate(*tailState.desc); + FetchScanDescriptor(tailState); + const uint16_t tailControlHi = static_cast( + tailState.desc->control >> HW::OHCIDescriptor::kControlHighShift); + const uint8_t tailCommand = static_cast( + (tailControlHi >> HW::OHCIDescriptor::kCmdShift) & 0xF); + if (tailCommand != HW::OHCIDescriptor::kCmdOutputLast || + HW::AT_xferStatus(*tailState.desc) == 0) { + return false; + } + + // Linux records completion on the packet's OUTPUT_LAST descriptor: + // references/linux-ohci-firewire-low-level-stack/drivers/firewire/ohci.c:1298-1310,1354-1366. + ClearDescriptorBlocks(state.headIndex, precursorBlocks, state.capacity); + ring_->SetHead(tailIndex); + ASFW_LOG_V2(Async, + "ScanCompletion: head %zu→%zu (completed OUTPUT_LAST after OUTPUT_MORE)", + state.headIndex, tailIndex); + return true; +} + template void ATContextBase::HandlePendingDescriptor(const ScanState& state) noexcept { uint32_t commandPtrAddr = 0; diff --git a/tests/async/ATContextCompletionTests.cpp b/tests/async/ATContextCompletionTests.cpp new file mode 100644 index 000000000..98bb46d7d --- /dev/null +++ b/tests/async/ATContextCompletionTests.cpp @@ -0,0 +1,106 @@ +#include +#include + +#include + +#include "ASFWDriver/Async/Contexts/ATRequestContext.hpp" +#include "ASFWDriver/Hardware/OHCIConstants.hpp" +#include "ASFWDriver/Hardware/OHCIDescriptors.hpp" +#include "ASFWDriver/Hardware/RegisterMap.hpp" +#include "ASFWDriver/Shared/Memory/DMAMemoryManager.hpp" +#include "ASFWDriver/Shared/Rings/DescriptorRing.hpp" + +namespace ASFW::Testing { +namespace { + +constexpr size_t kDescriptorCount = 8; +constexpr uint8_t kTLabel = 45; +constexpr uint16_t kTimestamp = 0x1234; + +class ATContextCompletionTest : public ::testing::Test { +protected: + Driver::HardwareInterface hardware_; + Shared::DMAMemoryManager dma_; + Shared::DescriptorRing ring_; + Async::ATRequestContext context_; + + void SetUp() override { + ASSERT_TRUE(dma_.Initialize( + hardware_, kDescriptorCount * sizeof(Async::HW::OHCIDescriptor))); + + auto region = dma_.AllocateRegion( + kDescriptorCount * sizeof(Async::HW::OHCIDescriptor)); + ASSERT_TRUE(region.has_value()); + + auto* descriptors = + reinterpret_cast(region->virtualBase); + ASSERT_TRUE(ring_.Initialize( + std::span{descriptors, kDescriptorCount})); + ASSERT_TRUE(ring_.Finalize(region->deviceBase)); + ASSERT_EQ(context_.Initialize(hardware_, ring_, dma_), kIOReturnSuccess); + } + + void PrepareBlockWriteChain( + uint16_t payloadStatus = + static_cast(Async::OHCIEventCode::kAckComplete)) { + auto* header = reinterpret_cast(ring_.At(0)); + ASSERT_NE(header, nullptr); + header->common.control = Async::HW::OHCIDescriptor::BuildControl({ + .reqCount = 16, + .command = Async::HW::OHCIDescriptor::kCmdOutputMore, + .key = Async::HW::OHCIDescriptor::kKeyImmediate, + .interruptBits = Async::HW::OHCIDescriptor::kIntNever, + .branchBits = Async::HW::OHCIDescriptor::kBranchAlways, + }); + header->immediateData[0] = static_cast(kTLabel) << 10; + + auto* payload = ring_.At(2); + ASSERT_NE(payload, nullptr); + payload->control = Async::HW::OHCIDescriptor::BuildControl({ + .reqCount = 8, + .command = Async::HW::OHCIDescriptor::kCmdOutputLast, + .key = Async::HW::OHCIDescriptor::kKeyStandard, + .interruptBits = Async::HW::OHCIDescriptor::kIntAlways, + .branchBits = Async::HW::OHCIDescriptor::kBranchNever, + }); + payload->timeStamp = kTimestamp; + payload->xferStatus = payloadStatus; + + ring_.SetTail(3); + hardware_.SetTestRegister( + Async::ATRequestTag::kControlSetReg, + Driver::kContextControlRunBit); + hardware_.SetTestRegister( + Async::ATRequestTag::kCommandPtrReg, + ring_.CommandPtrWordTo(ring_.At(0), 3)); + } +}; + +TEST_F(ATContextCompletionTest, + UsesOutputLastStatusWhenOutputMorePrecursorHasNoStatus) { + PrepareBlockWriteChain(); + + const auto completion = context_.ScanCompletion(); + + ASSERT_TRUE(completion.has_value()); + EXPECT_EQ(completion->eventCode, Async::OHCIEventCode::kAckComplete); + EXPECT_EQ(completion->timeStamp, kTimestamp); + EXPECT_EQ(completion->tLabel, kTLabel); + EXPECT_EQ(completion->descriptor, ring_.At(2)); + EXPECT_EQ(ring_.Head(), 3u); + EXPECT_TRUE(ring_.IsEmpty()); +} + +TEST_F(ATContextCompletionTest, + WaitsWhenOutputMoreAndOutputLastBothHaveNoStatus) { + PrepareBlockWriteChain(0); + + const auto completion = context_.ScanCompletion(); + + EXPECT_FALSE(completion.has_value()); + EXPECT_EQ(ring_.Head(), 0u); + EXPECT_FALSE(ring_.IsEmpty()); +} + +} // namespace +} // namespace ASFW::Testing diff --git a/tests/async/CMakeLists.txt b/tests/async/CMakeLists.txt index 19bd8ae20..54f5f2dad 100644 --- a/tests/async/CMakeLists.txt +++ b/tests/async/CMakeLists.txt @@ -68,6 +68,14 @@ add_async_test(ATDescriptorTests ATDescriptorTests.cpp ) +# AT Context Completion Tests +add_async_test(ATContextCompletionTests + ATContextCompletionTests.cpp + "${ASFW_DRIVER_DIR}/Shared/Memory/DMAMemoryManager.cpp" + "${ASFW_DRIVER_DIR}/Shared/Rings/DescriptorRing.cpp" + "${ASFW_DRIVER_DIR}/Common/BarrierUtils.cpp" +) + # Buffer Ring DMA Tests add_async_test(BufferRingDMATests BufferRingDMATests.cpp From 6594e52ec681a2d1da02040309ca47d41e661459 Mon Sep 17 00:00:00 2001 From: gly11 Date: Mon, 27 Jul 2026 22:02:56 +0800 Subject: [PATCH 08/13] fix(async): preserve pending OUTPUT_LAST chains --- ASFWDriver/Async/Contexts/ATContextBase.hpp | 60 ++++++++++++++------- tests/async/ATContextCompletionTests.cpp | 53 ++++++++++++++++++ 2 files changed, 94 insertions(+), 19 deletions(-) diff --git a/ASFWDriver/Async/Contexts/ATContextBase.hpp b/ASFWDriver/Async/Contexts/ATContextBase.hpp index fc78bec6c..5b0917a03 100644 --- a/ASFWDriver/Async/Contexts/ATContextBase.hpp +++ b/ASFWDriver/Async/Contexts/ATContextBase.hpp @@ -254,11 +254,10 @@ class ATContextBase : public ContextBase { * 11. Unlock context, return TxCompletion * * **Apple Pattern** - * ChannelBundle::ScanNextATReqCompletion(): - * - Checks xferStatus != 0 for completion - * - Extracts ack code and event code from status word - * - Extracts tLabel from packet header for response matching - * - Advances completion cursor + * AppleFWOHCI_AsyncTransmit::checkForCompletedElements(): + * - Checks the terminal descriptor status of each pending ATxElement + * - Leaves the element queued while the terminal status is zero + * - Dispatches completion only after the terminal status becomes non-zero * * **Thread Safety** * Serialized via IOLock. Safe to call concurrently with SubmitChain(). @@ -315,6 +314,12 @@ class ATContextBase : public ContextBase { uint16_t timeStamp{0}; }; + enum class ChainTailScanResult : uint8_t { + NotRecognized, + Pending, + Advanced, + }; + /// Descriptor ring for tracking in-flight chains DescriptorRing* ring_{nullptr}; @@ -342,7 +347,8 @@ class ATContextBase : public ContextBase { size_t capacity) noexcept; [[nodiscard]] bool LoadScanState(ScanState& state) noexcept; void FetchScanDescriptor(const ScanState& state) noexcept; - [[nodiscard]] bool AdvanceToCompletedChainTail(const ScanState& state) noexcept; + [[nodiscard]] ChainTailScanResult InspectChainTail( + const ScanState& state) noexcept; void HandlePendingDescriptor(const ScanState& state) noexcept; [[nodiscard]] bool IsOrphanedDescriptor(const ScanState& state, uint32_t& commandPtrAddr, @@ -607,11 +613,17 @@ std::optional ATContextBase::ScanCompletion() noexce return std::nullopt; } - if (state.xferStatus == 0 && AdvanceToCompletedChainTail(state)) { - continue; - } - if (state.xferStatus == 0) { + switch (InspectChainTail(state)) { + case ChainTailScanResult::Advanced: + continue; + case ChainTailScanResult::Pending: + unlock(); + return std::nullopt; + case ChainTailScanResult::NotRecognized: + break; + } + HandlePendingDescriptor(state); unlock(); return std::nullopt; @@ -833,14 +845,14 @@ void ATContextBase::FetchScanDescriptor(const ScanState& state) no } template -bool ATContextBase::AdvanceToCompletedChainTail( - const ScanState& state) noexcept { +typename ATContextBase::ChainTailScanResult +ATContextBase::InspectChainTail(const ScanState& state) noexcept { const uint16_t controlHi = static_cast( state.desc->control >> HW::OHCIDescriptor::kControlHighShift); const uint8_t command = static_cast( (controlHi >> HW::OHCIDescriptor::kCmdShift) & 0xF); if (command != HW::OHCIDescriptor::kCmdOutputMore) { - return false; + return ChainTailScanResult::NotRecognized; } const uint8_t key = static_cast( @@ -850,7 +862,7 @@ bool ATContextBase::AdvanceToCompletedChainTail( const size_t tailIndex = (state.headIndex + precursorBlocks) % state.capacity; if (tailIndex == state.tailIndex) { - return false; + return ChainTailScanResult::NotRecognized; } ScanState tailState; @@ -858,7 +870,7 @@ bool ATContextBase::AdvanceToCompletedChainTail( tailState.headIndex = tailIndex; tailState.desc = ring_->At(tailIndex); if (!tailState.desc) { - return false; + return ChainTailScanResult::NotRecognized; } tailState.isImmediate = HW::IsImmediate(*tailState.desc); FetchScanDescriptor(tailState); @@ -866,9 +878,19 @@ bool ATContextBase::AdvanceToCompletedChainTail( tailState.desc->control >> HW::OHCIDescriptor::kControlHighShift); const uint8_t tailCommand = static_cast( (tailControlHi >> HW::OHCIDescriptor::kCmdShift) & 0xF); - if (tailCommand != HW::OHCIDescriptor::kCmdOutputLast || - HW::AT_xferStatus(*tailState.desc) == 0) { - return false; + if (tailCommand != HW::OHCIDescriptor::kCmdOutputLast) { + return ChainTailScanResult::NotRecognized; + } + + if (HW::AT_xferStatus(*tailState.desc) == 0) { + // AppleFWOHCI 5.5.9 checkForCompletedElements() reads each pending + // ATxElement's terminal descriptor status and leaves the element queued + // while that status is zero (symbol offsets 0xf0e8-0xf10a). + // Linux leaves the packet queued until its OUTPUT_LAST transfer status becomes + // non-zero: references/linux-ohci-firewire-low-level-stack/drivers/firewire/ohci.c:1354-1366. + // The command pointer may already reference this pending tail, so the + // OUTPUT_MORE precursor is not orphaned. + return ChainTailScanResult::Pending; } // Linux records completion on the packet's OUTPUT_LAST descriptor: @@ -878,7 +900,7 @@ bool ATContextBase::AdvanceToCompletedChainTail( ASFW_LOG_V2(Async, "ScanCompletion: head %zu→%zu (completed OUTPUT_LAST after OUTPUT_MORE)", state.headIndex, tailIndex); - return true; + return ChainTailScanResult::Advanced; } template diff --git a/tests/async/ATContextCompletionTests.cpp b/tests/async/ATContextCompletionTests.cpp index 98bb46d7d..8e5b4af02 100644 --- a/tests/async/ATContextCompletionTests.cpp +++ b/tests/async/ATContextCompletionTests.cpp @@ -102,5 +102,58 @@ TEST_F(ATContextCompletionTest, EXPECT_FALSE(ring_.IsEmpty()); } +TEST_F(ATContextCompletionTest, + PreservesRecognizedChainWhenCommandPtrAdvancesToPendingOutputLast) { + PrepareBlockWriteChain(0); + hardware_.SetTestRegister( + Async::ATRequestTag::kCommandPtrReg, + ring_.CommandPtrWordTo(ring_.At(2), 1)); + + const auto pendingCompletion = context_.ScanCompletion(); + + EXPECT_FALSE(pendingCompletion.has_value()); + EXPECT_EQ(ring_.Head(), 0u); + EXPECT_FALSE(ring_.IsEmpty()); + + auto* payload = ring_.At(2); + ASSERT_NE(payload, nullptr); + payload->xferStatus = + static_cast(Async::OHCIEventCode::kAckComplete); + + const auto completion = context_.ScanCompletion(); + + ASSERT_TRUE(completion.has_value()); + EXPECT_EQ(completion->eventCode, Async::OHCIEventCode::kAckComplete); + EXPECT_EQ(completion->timeStamp, kTimestamp); + EXPECT_EQ(completion->tLabel, kTLabel); + EXPECT_EQ(completion->descriptor, payload); + EXPECT_EQ(ring_.Head(), 3u); + EXPECT_TRUE(ring_.IsEmpty()); +} + +TEST_F(ATContextCompletionTest, AdvancesTrulyOrphanedPendingDescriptor) { + auto* descriptor = ring_.At(0); + ASSERT_NE(descriptor, nullptr); + descriptor->control = Async::HW::OHCIDescriptor::BuildControl({ + .reqCount = 8, + .command = Async::HW::OHCIDescriptor::kCmdOutputLast, + .key = Async::HW::OHCIDescriptor::kKeyStandard, + .interruptBits = Async::HW::OHCIDescriptor::kIntAlways, + .branchBits = Async::HW::OHCIDescriptor::kBranchNever, + }); + descriptor->xferStatus = 0; + ring_.SetTail(1); + hardware_.SetTestRegister(Async::ATRequestTag::kControlSetReg, 0); + hardware_.SetTestRegister( + Async::ATRequestTag::kCommandPtrReg, + ring_.CommandPtrWordTo(descriptor, 1)); + + const auto completion = context_.ScanCompletion(); + + EXPECT_FALSE(completion.has_value()); + EXPECT_EQ(ring_.Head(), 1u); + EXPECT_TRUE(ring_.IsEmpty()); +} + } // namespace } // namespace ASFW::Testing From ca8604326dd1eed2f958e4468193bfcfe3da96e8 Mon Sep 17 00:00:00 2001 From: gly11 Date: Wed, 29 Jul 2026 01:08:54 +0800 Subject: [PATCH 09/13] fix(async): drain recognized chains when the AT context is quiesced MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- ASFWDriver/Async/Contexts/ATContextBase.hpp | 48 ++++++++++++++++++--- tests/async/ATContextCompletionTests.cpp | 23 ++++++++++ 2 files changed, 64 insertions(+), 7 deletions(-) diff --git a/ASFWDriver/Async/Contexts/ATContextBase.hpp b/ASFWDriver/Async/Contexts/ATContextBase.hpp index 5b0917a03..b0d532c95 100644 --- a/ASFWDriver/Async/Contexts/ATContextBase.hpp +++ b/ASFWDriver/Async/Contexts/ATContextBase.hpp @@ -259,6 +259,13 @@ class ATContextBase : public ContextBase { * - Leaves the element queued while the terminal status is zero * - Dispatches completion only after the terminal status becomes non-zero * + * Apple walks a linked list of ATxElements, so a zero-status element only + * clears its "reap the predecessor" flag and iteration continues past it + * (symbol offsets 0xf0e8-0xf10a, 0xf188-0xf195). This ring has a single + * head cursor instead, so a zero-status chain at head necessarily blocks + * everything behind it — hence the quiesced-context escape in + * InspectChainTail(), which Apple gets from resetDMA() instead. + * * **Thread Safety** * Serialized via IOLock. Safe to call concurrently with SubmitChain(). * @@ -349,6 +356,7 @@ class ATContextBase : public ContextBase { void FetchScanDescriptor(const ScanState& state) noexcept; [[nodiscard]] ChainTailScanResult InspectChainTail( const ScanState& state) noexcept; + [[nodiscard]] bool IsContextQuiesced() noexcept; void HandlePendingDescriptor(const ScanState& state) noexcept; [[nodiscard]] bool IsOrphanedDescriptor(const ScanState& state, uint32_t& commandPtrAddr, @@ -883,13 +891,28 @@ ATContextBase::InspectChainTail(const ScanState& state) noexcept { } if (HW::AT_xferStatus(*tailState.desc) == 0) { - // AppleFWOHCI 5.5.9 checkForCompletedElements() reads each pending - // ATxElement's terminal descriptor status and leaves the element queued - // while that status is zero (symbol offsets 0xf0e8-0xf10a). - // Linux leaves the packet queued until its OUTPUT_LAST transfer status becomes - // non-zero: references/linux-ohci-firewire-low-level-stack/drivers/firewire/ohci.c:1354-1366. - // The command pointer may already reference this pending tail, so the - // OUTPUT_MORE precursor is not orphaned. + // A running context has simply not finished this packet yet. The command + // pointer may already reference the pending tail, so the OUTPUT_MORE + // precursor is NOT orphaned and must stay queued. + // AppleFWOHCI 5.5.9 checkForCompletedElements() likewise leaves an + // ATxElement queued while its terminal descriptor status is zero + // (symbol offsets 0xf0e8-0xf10a), and Linux returns 0 from + // handle_at_packet() to stop iteration: + // references/linux-ohci-firewire-low-level-stack/drivers/firewire/ohci.c:1354-1366. + // + // A quiesced context is different: hardware will never write a status, + // so treating the chain as merely pending would wedge the ring head + // forever. Linux gates the same early return on !ctx->flushing + // (ohci.c:1364) and drains via at_context_flush() after context_stop() + // (ohci.c:2000-2010); AppleFWOHCI calls resetDMA() after stopDMA() in + // handleBusResetInt() (symbol offsets 0x5fae-0x5fd2), which frees every + // pending element regardless of status. OHCI 1.2 draft clause 7.2.3.3: + // hardware may leave unsent packets in the AT queues for software to + // drain. ASFW has no equivalent flush entry point yet, so fall through + // to the orphan path, which is what releases the ring today. + if (IsContextQuiesced()) { + return ChainTailScanResult::NotRecognized; + } return ChainTailScanResult::Pending; } @@ -903,6 +926,17 @@ ATContextBase::InspectChainTail(const ScanState& state) noexcept { return ChainTailScanResult::Advanced; } +template +bool ATContextBase::IsContextQuiesced() noexcept { + // Deliberately narrower than IsOrphanedDescriptor(): only run==0 && active==0 + // proves hardware will never write another status. That function's second + // clause (commandPtr != headIOVA) also fires for a *live* chain whose command + // pointer has already advanced to a pending OUTPUT_LAST, which is exactly the + // false positive this scan path must not reintroduce. + const uint32_t controlReg = this->ReadControl(); + return (controlReg & (kContextControlRunBit | kContextControlActiveBit)) == 0; +} + template void ATContextBase::HandlePendingDescriptor(const ScanState& state) noexcept { uint32_t commandPtrAddr = 0; diff --git a/tests/async/ATContextCompletionTests.cpp b/tests/async/ATContextCompletionTests.cpp index 8e5b4af02..5868be2d8 100644 --- a/tests/async/ATContextCompletionTests.cpp +++ b/tests/async/ATContextCompletionTests.cpp @@ -155,5 +155,28 @@ TEST_F(ATContextCompletionTest, AdvancesTrulyOrphanedPendingDescriptor) { EXPECT_TRUE(ring_.IsEmpty()); } +// A quiesced context will never write a status into the OUTPUT_LAST, so the +// recognized-chain path must not hold the head hostage waiting for one. Both +// references drain unconditionally once the context is stopped: Linux gates +// handle_at_packet()'s early return on !ctx->flushing and calls +// at_context_flush() after context_stop() (ohci.c:1364, 2000-2010); AppleFWOHCI +// calls resetDMA() after stopDMA() in handleBusResetInt(). OHCI 1.2 draft +// clause 7.2.3.3 requires software to drain what hardware left unsent. +TEST_F(ATContextCompletionTest, DrainsRecognizedChainWhenContextIsQuiesced) { + PrepareBlockWriteChain(0); + hardware_.SetTestRegister(Async::ATRequestTag::kControlSetReg, 0); + + while (!ring_.IsEmpty()) { + const auto completion = context_.ScanCompletion(); + ASSERT_FALSE(completion.has_value()); + if (ring_.Head() == 0u) { + FAIL() << "quiesced context wedged the ring head at 0"; + } + } + + EXPECT_EQ(ring_.Head(), 3u); + EXPECT_TRUE(ring_.IsEmpty()); +} + } // namespace } // namespace ASFW::Testing From f916716dd75f5a65c25bbca5c8eca624991cbf10 Mon Sep 17 00:00:00 2001 From: gly11 Date: Wed, 29 Jul 2026 01:18:03 +0800 Subject: [PATCH 10/13] fix(async): report unsent AT packets as evt_flushed after a bus reset MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- ASFWDriver/Async/AsyncSubsystemBusReset.cpp | 28 ++++++- ASFWDriver/Async/Contexts/ATContextBase.hpp | 85 +++++++++++++++------ tests/async/ATContextCompletionTests.cpp | 81 ++++++++++++++++---- 3 files changed, 155 insertions(+), 39 deletions(-) diff --git a/ASFWDriver/Async/AsyncSubsystemBusReset.cpp b/ASFWDriver/Async/AsyncSubsystemBusReset.cpp index a60248724..14ac46d06 100644 --- a/ASFWDriver/Async/AsyncSubsystemBusReset.cpp +++ b/ASFWDriver/Async/AsyncSubsystemBusReset.cpp @@ -1,12 +1,16 @@ // SPDX-License-Identifier: Apache-2.0 #include "AsyncSubsystem.hpp" +#include "Contexts/ATRequestContext.hpp" +#include "Contexts/ATResponseContext.hpp" #include "Tx/Submitter.hpp" #include "../Logging/Logging.hpp" #include "Track/LabelAllocator.hpp" #include "Track/PayloadRegistry.hpp" +#include + #include namespace ASFW::Async { @@ -167,7 +171,29 @@ void AsyncSubsystem::FlushATContexts() { if (!txnMgr_) { return; } - (void)DrainTxCompletions(nullptr); + + // Callers reach here only after StopATContextsOnly(), so hardware can no + // longer write descriptor status. Anything still queued was left unsent by + // the bus reset (OHCI 1.2 draft clause 7.2.3.3) and must be reported as + // evt_flushed rather than waited on — the same ordering Linux uses + // (context_stop() then at_context_flush(), ohci.c:2000-2010) and Apple uses + // (stopDMA() then resetDMA() in handleBusResetInt()). + auto* atRequest = ResolveAtRequestContext(); + auto* atResponse = ResolveAtResponseContext(); + + std::optional requestFlush; + if (atRequest) { + requestFlush.emplace(*atRequest); + } + std::optional responseFlush; + if (atResponse) { + responseFlush.emplace(*atResponse); + } + + const uint32_t flushed = DrainTxCompletions("bus-reset-flush"); + if (flushed > 0) { + ASFW_LOG(Async, "FlushATContexts: flushed %u unsent AT packet(s)", flushed); + } } void AsyncSubsystem::ConfirmBusGeneration(uint8_t confirmedGeneration) { diff --git a/ASFWDriver/Async/Contexts/ATContextBase.hpp b/ASFWDriver/Async/Contexts/ATContextBase.hpp index b0d532c95..26fb2feea 100644 --- a/ASFWDriver/Async/Contexts/ATContextBase.hpp +++ b/ASFWDriver/Async/Contexts/ATContextBase.hpp @@ -275,6 +275,46 @@ class ATContextBase : public ContextBase { */ [[nodiscard]] std::optional ScanCompletion() noexcept; + /** + * \brief RAII scope that makes ScanCompletion() report unsent packets. + * + * While held, a descriptor whose xferStatus is still zero is reported as + * evt_flushed instead of being treated as pending. OHCI 1.2 draft clause + * 7.2.3.3 leaves such packets in the AT queue once the context stops, and + * the transaction owning them would otherwise wait for a status hardware + * will never write. + * + * \warning The context must already be stopped. Entering this scope while + * hardware can still write a status would report packets that are + * merely in flight as flushed. + * + * **Reference Pattern** + * Linux at_context_flush() sets ctx->flushing around ohci_at_context_work() + * so the ordinary handle_at_packet() body drains the queue instead of + * stopping at the first zero status (ohci.c:1331-1343, 1364). AppleFWOHCI + * reaches the same end state through resetDMA(), which frees every pending + * ATxElement after stopDMA() (symbol offsets 0x5fae-0x5fd2). + */ + class [[nodiscard]] FlushScope { + public: + explicit FlushScope(ATContextBase& context) noexcept : context_(&context) { + context_->flushing_ = true; + } + ~FlushScope() { + if (context_) { + context_->flushing_ = false; + } + } + + FlushScope(const FlushScope&) = delete; + FlushScope& operator=(const FlushScope&) = delete; + FlushScope(FlushScope&&) = delete; + FlushScope& operator=(FlushScope&&) = delete; + + private: + ATContextBase* context_{nullptr}; + }; + /** * \brief Get descriptor ring for diagnostics. * @@ -327,6 +367,10 @@ class ATContextBase : public ContextBase { Advanced, }; + /// Set by FlushScope: report zero-status descriptors as evt_flushed. + /// Touched only under submitLock_ (set/cleared while the context is stopped). + bool flushing_{false}; + /// Descriptor ring for tracking in-flight chains DescriptorRing* ring_{nullptr}; @@ -356,7 +400,6 @@ class ATContextBase : public ContextBase { void FetchScanDescriptor(const ScanState& state) noexcept; [[nodiscard]] ChainTailScanResult InspectChainTail( const ScanState& state) noexcept; - [[nodiscard]] bool IsContextQuiesced() noexcept; void HandlePendingDescriptor(const ScanState& state) noexcept; [[nodiscard]] bool IsOrphanedDescriptor(const ScanState& state, uint32_t& commandPtrAddr, @@ -621,6 +664,18 @@ std::optional ATContextBase::ScanCompletion() noexce return std::nullopt; } + // OHCI 1.2 draft clause 7.2.3.3: after the context stops, hardware may + // leave packets in the AT queue that it never transmitted and will never + // write a status for. Reporting them as evt_flushed is what releases the + // ring and fails their transactions instead of letting them time out. + // Linux does exactly this by re-running handle_at_packet() with + // ctx->flushing set (ohci.c:1364, at_context_flush() at 1331-1343); + // AppleFWOHCI's resetDMA() frees every pending ATxElement after stopDMA() + // (symbol offsets 0x5fae-0x5fd2). + if (state.xferStatus == 0 && flushing_) { + state.xferStatus = static_cast(OHCIEventCode::kEvtFlushed); + } + if (state.xferStatus == 0) { switch (InspectChainTail(state)) { case ChainTailScanResult::Advanced: @@ -900,19 +955,10 @@ ATContextBase::InspectChainTail(const ScanState& state) noexcept { // handle_at_packet() to stop iteration: // references/linux-ohci-firewire-low-level-stack/drivers/firewire/ohci.c:1354-1366. // - // A quiesced context is different: hardware will never write a status, - // so treating the chain as merely pending would wedge the ring head - // forever. Linux gates the same early return on !ctx->flushing - // (ohci.c:1364) and drains via at_context_flush() after context_stop() - // (ohci.c:2000-2010); AppleFWOHCI calls resetDMA() after stopDMA() in - // handleBusResetInt() (symbol offsets 0x5fae-0x5fd2), which frees every - // pending element regardless of status. OHCI 1.2 draft clause 7.2.3.3: - // hardware may leave unsent packets in the AT queues for software to - // drain. ASFW has no equivalent flush entry point yet, so fall through - // to the orphan path, which is what releases the ring today. - if (IsContextQuiesced()) { - return ChainTailScanResult::NotRecognized; - } + // A stopped context never writes a status either, but the chain must + // still be held here: FlushScope is what reports it as evt_flushed and + // fails the owning transaction. Releasing it from this path instead + // would consume the descriptors before the flush could see them. return ChainTailScanResult::Pending; } @@ -926,17 +972,6 @@ ATContextBase::InspectChainTail(const ScanState& state) noexcept { return ChainTailScanResult::Advanced; } -template -bool ATContextBase::IsContextQuiesced() noexcept { - // Deliberately narrower than IsOrphanedDescriptor(): only run==0 && active==0 - // proves hardware will never write another status. That function's second - // clause (commandPtr != headIOVA) also fires for a *live* chain whose command - // pointer has already advanced to a pending OUTPUT_LAST, which is exactly the - // false positive this scan path must not reintroduce. - const uint32_t controlReg = this->ReadControl(); - return (controlReg & (kContextControlRunBit | kContextControlActiveBit)) == 0; -} - template void ATContextBase::HandlePendingDescriptor(const ScanState& state) noexcept { uint32_t commandPtrAddr = 0; diff --git a/tests/async/ATContextCompletionTests.cpp b/tests/async/ATContextCompletionTests.cpp index 5868be2d8..e93de682c 100644 --- a/tests/async/ATContextCompletionTests.cpp +++ b/tests/async/ATContextCompletionTests.cpp @@ -155,28 +155,83 @@ TEST_F(ATContextCompletionTest, AdvancesTrulyOrphanedPendingDescriptor) { EXPECT_TRUE(ring_.IsEmpty()); } -// A quiesced context will never write a status into the OUTPUT_LAST, so the -// recognized-chain path must not hold the head hostage waiting for one. Both -// references drain unconditionally once the context is stopped: Linux gates -// handle_at_packet()'s early return on !ctx->flushing and calls -// at_context_flush() after context_stop() (ohci.c:1364, 2000-2010); AppleFWOHCI -// calls resetDMA() after stopDMA() in handleBusResetInt(). OHCI 1.2 draft -// clause 7.2.3.3 requires software to drain what hardware left unsent. -TEST_F(ATContextCompletionTest, DrainsRecognizedChainWhenContextIsQuiesced) { +// A stopped context will never write a status, but an ordinary scan must still +// leave the chain alone: FlushScope is what reports it and fails the owning +// transaction. A scan that landed between StopATContextsOnly() and +// FlushATContexts() (the watchdog drain runs on a timer) would otherwise consume +// the descriptors first, leaving the transaction to time out instead. +TEST_F(ATContextCompletionTest, HoldsQuiescedChainSoTheFlushCanReportIt) { PrepareBlockWriteChain(0); hardware_.SetTestRegister(Async::ATRequestTag::kControlSetReg, 0); - while (!ring_.IsEmpty()) { + for (int i = 0; i < 4; ++i) { const auto completion = context_.ScanCompletion(); - ASSERT_FALSE(completion.has_value()); - if (ring_.Head() == 0u) { - FAIL() << "quiesced context wedged the ring head at 0"; - } + EXPECT_FALSE(completion.has_value()); } + EXPECT_EQ(ring_.Head(), 0u); + EXPECT_FALSE(ring_.IsEmpty()); + + Async::ATRequestContext::FlushScope flush(context_); + const auto flushed = context_.ScanCompletion(); + + ASSERT_TRUE(flushed.has_value()); + EXPECT_EQ(flushed->eventCode, Async::OHCIEventCode::kEvtFlushed); + EXPECT_EQ(flushed->tLabel, kTLabel); + EXPECT_TRUE(ring_.IsEmpty()); +} + +// The quiesced-context drain above releases the ring but reports nothing, so the +// owning transaction only fails once it times out. Inside a FlushScope the same +// chain is reported as evt_flushed, which TransactionCompletionHandler maps to +// kIOReturnAborted — Linux calls the equivalent RCODE_GENERATION "the same error +// as when we try to use a stale generation count" (ohci.c:1387-1393). +TEST_F(ATContextCompletionTest, FlushScopeReportsUnsentChainAsFlushed) { + PrepareBlockWriteChain(0); + hardware_.SetTestRegister(Async::ATRequestTag::kControlSetReg, 0); + + Async::ATRequestContext::FlushScope flush(context_); + + const auto completion = context_.ScanCompletion(); + + ASSERT_TRUE(completion.has_value()); + EXPECT_EQ(completion->eventCode, Async::OHCIEventCode::kEvtFlushed); + EXPECT_EQ(completion->tLabel, kTLabel); + EXPECT_EQ(completion->descriptor, ring_.At(2)); EXPECT_EQ(ring_.Head(), 3u); EXPECT_TRUE(ring_.IsEmpty()); } +// A genuinely completed packet keeps its real status while flushing; only the +// zero-status descriptors are synthesized. +TEST_F(ATContextCompletionTest, FlushScopePreservesRealCompletionStatus) { + PrepareBlockWriteChain(); + + Async::ATRequestContext::FlushScope flush(context_); + + const auto completion = context_.ScanCompletion(); + + ASSERT_TRUE(completion.has_value()); + EXPECT_EQ(completion->eventCode, Async::OHCIEventCode::kAckComplete); + EXPECT_EQ(completion->timeStamp, kTimestamp); + EXPECT_EQ(completion->tLabel, kTLabel); + EXPECT_TRUE(ring_.IsEmpty()); +} + +// Leaving the scope must restore normal pending semantics, otherwise a live +// context would start reporting in-flight packets as flushed. +TEST_F(ATContextCompletionTest, FlushScopeRestoresPendingSemanticsOnExit) { + PrepareBlockWriteChain(0); + { + Async::ATRequestContext::FlushScope flush(context_); + } + + const auto completion = context_.ScanCompletion(); + + EXPECT_FALSE(completion.has_value()); + EXPECT_EQ(ring_.Head(), 0u); + EXPECT_FALSE(ring_.IsEmpty()); +} + } // namespace } // namespace ASFW::Testing From be045e6ff7003b2b102b29debffb5ff6827f968b Mon Sep 17 00:00:00 2001 From: gly11 Date: Wed, 29 Jul 2026 02:03:26 +0800 Subject: [PATCH 11/13] fix(async): read ContextControl ACTIVE from the shared constant MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- ASFWDriver/Async/Contexts/ContextBase.hpp | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/ASFWDriver/Async/Contexts/ContextBase.hpp b/ASFWDriver/Async/Contexts/ContextBase.hpp index ad5170bd2..b3049d670 100644 --- a/ASFWDriver/Async/Contexts/ContextBase.hpp +++ b/ASFWDriver/Async/Contexts/ContextBase.hpp @@ -5,6 +5,7 @@ #include #include "../../Hardware/HardwareInterface.hpp" +#include "../../Hardware/OHCIConstants.hpp" #include "../../Hardware/RegisterMap.hpp" namespace ASFW::Async { @@ -254,8 +255,7 @@ class ContextBase { * Used for polling during context stop sequence. */ [[nodiscard]] bool IsActive() const noexcept { - constexpr uint32_t kActiveBit = 1u << 13; - return (ReadControl() & kActiveBit) != 0; + return (ReadControl() & Driver::kContextControlActiveBit) != 0; } /** @@ -264,8 +264,7 @@ class ContextBase { * \return true if ContextControl.run bit is set */ [[nodiscard]] bool IsRunning() const noexcept { - constexpr uint32_t kRunBit = 1u << 15; - return (ReadControl() & kRunBit) != 0; + return (ReadControl() & Driver::kContextControlRunBit) != 0; } /** From 359ddd2967855db8b5b184245f05faeb99e4c72e Mon Sep 17 00:00:00 2001 From: gly11 Date: Wed, 29 Jul 2026 02:03:26 +0800 Subject: [PATCH 12/13] fix(async): stop both AT contexts and flush only quiesced ones MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the P1 raised on #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. --- ASFWDriver/Async/AsyncSubsystemBusReset.cpp | 28 +++++++++++++++++++-- ASFWDriver/Async/Engine/ContextManager.cpp | 22 +++++++++++++--- tests/async/ATContextCompletionTests.cpp | 21 ++++++++++++++++ 3 files changed, 65 insertions(+), 6 deletions(-) diff --git a/ASFWDriver/Async/AsyncSubsystemBusReset.cpp b/ASFWDriver/Async/AsyncSubsystemBusReset.cpp index 14ac46d06..bdad1db33 100644 --- a/ASFWDriver/Async/AsyncSubsystemBusReset.cpp +++ b/ASFWDriver/Async/AsyncSubsystemBusReset.cpp @@ -178,17 +178,41 @@ void AsyncSubsystem::FlushATContexts() { // evt_flushed rather than waited on — the same ordering Linux uses // (context_stop() then at_context_flush(), ohci.c:2000-2010) and Apple uses // (stopDMA() then resetDMA() in handleBusResetInt()). + // + // Confirm ACTIVE is clear per context before flushing. Both references stop + // both contexts unconditionally and then flush unconditionally, but neither + // can prove the stop succeeded: OHCI §7.2.3 lets a context keep the ACTIVE + // bit set past the stop timeout. Flushing such a context would report packets + // that are merely in flight and zero descriptor words the controller is still + // traversing, so skip it and leave the ring for the next scan instead. auto* atRequest = ResolveAtRequestContext(); auto* atResponse = ResolveAtResponseContext(); + const auto flushable = [](auto* context, const char* name) { + if (!context) { + return false; + } + if (context->IsActive()) { + ASFW_LOG_ERROR(Async, + "FlushATContexts: %{public}s still ACTIVE after stop; " + "skipping flush to avoid rewriting live descriptors", + name); + return false; + } + return true; + }; + std::optional requestFlush; - if (atRequest) { + if (flushable(atRequest, "AT request")) { requestFlush.emplace(*atRequest); } std::optional responseFlush; - if (atResponse) { + if (flushable(atResponse, "AT response")) { responseFlush.emplace(*atResponse); } + if (!requestFlush && !responseFlush) { + return; + } const uint32_t flushed = DrainTxCompletions("bus-reset-flush"); if (flushed > 0) { diff --git a/ASFWDriver/Async/Engine/ContextManager.cpp b/ASFWDriver/Async/Engine/ContextManager.cpp index e240922de..169a682f9 100644 --- a/ASFWDriver/Async/Engine/ContextManager.cpp +++ b/ASFWDriver/Async/Engine/ContextManager.cpp @@ -350,11 +350,25 @@ kern_return_t ContextManager::stopAT() noexcept { ASFW_LOG(Async, "ContextManager::stopAT - stopping AT contexts"); - kern_return_t kr = state_->atReqCtx.Stop(); - if (kr != kIOReturnSuccess) return kr; + // Stop both contexts unconditionally, then report the first failure. An + // early return would leave the response context running while callers treat + // AT as quiesced — AsyncSubsystem::FlushATContexts() would then rewrite + // descriptors the controller still owns. Linux issues both context_stop() + // calls back to back for the same reason (ohci.c:1999-2000), as does + // AppleFWOHCI's handleBusResetInt() (stopDMA at symbol offsets + // 0x5fae and 0x5fba). + const kern_return_t reqKr = state_->atReqCtx.Stop(); + if (reqKr != kIOReturnSuccess) { + ASFW_LOG(Async, "ContextManager::stopAT - AT req stop failed (kr=0x%08x)", reqKr); + } - kr = state_->atRspCtx.Stop(); - if (kr != kIOReturnSuccess) return kr; + const kern_return_t rspKr = state_->atRspCtx.Stop(); + if (rspKr != kIOReturnSuccess) { + ASFW_LOG(Async, "ContextManager::stopAT - AT rsp stop failed (kr=0x%08x)", rspKr); + } + + if (reqKr != kIOReturnSuccess) return reqKr; + if (rspKr != kIOReturnSuccess) return rspKr; ASFW_LOG(Async, "ContextManager::stopAT - SUCCESS"); return kIOReturnSuccess; diff --git a/tests/async/ATContextCompletionTests.cpp b/tests/async/ATContextCompletionTests.cpp index e93de682c..307345593 100644 --- a/tests/async/ATContextCompletionTests.cpp +++ b/tests/async/ATContextCompletionTests.cpp @@ -218,6 +218,27 @@ TEST_F(ATContextCompletionTest, FlushScopePreservesRealCompletionStatus) { EXPECT_TRUE(ring_.IsEmpty()); } +// FlushATContexts() gates FlushScope on IsActive(). This pins the property that +// gate depends on: a context whose ACTIVE bit is still set must not be flushed, +// because its queued packets are in flight rather than stranded, and clearing +// their descriptor words would cut the chain the controller is traversing. +TEST_F(ATContextCompletionTest, ActiveContextIsNotEligibleForFlush) { + PrepareBlockWriteChain(0); + hardware_.SetTestRegister( + Async::ATRequestTag::kControlSetReg, + Driver::kContextControlRunBit | Driver::kContextControlActiveBit); + + ASSERT_TRUE(context_.IsActive()); + + // What FlushATContexts() does when IsActive() reports true: no FlushScope. + const auto completion = context_.ScanCompletion(); + + EXPECT_FALSE(completion.has_value()); + EXPECT_EQ(ring_.Head(), 0u); + EXPECT_FALSE(ring_.IsEmpty()); + EXPECT_EQ(ring_.At(2)->xferStatus, 0); +} + // Leaving the scope must restore normal pending semantics, otherwise a live // context would start reporting in-flight packets as flushed. TEST_F(ATContextCompletionTest, FlushScopeRestoresPendingSemanticsOnExit) { From edcb757ab7bdbbc1f5d8f142c7583d0791bcfdf7 Mon Sep 17 00:00:00 2001 From: gly11 Date: Wed, 29 Jul 2026 16:10:13 +0800 Subject: [PATCH 13/13] fix(async): clarify statusless reset completions --- ASFWDriver/Async/AsyncSubsystemBusReset.cpp | 23 +++++++++------- ASFWDriver/Async/Contexts/ATContextBase.hpp | 29 ++++++++++++--------- tests/async/ATContextCompletionTests.cpp | 19 +++++++------- 3 files changed, 40 insertions(+), 31 deletions(-) diff --git a/ASFWDriver/Async/AsyncSubsystemBusReset.cpp b/ASFWDriver/Async/AsyncSubsystemBusReset.cpp index bdad1db33..3ada2e5a5 100644 --- a/ASFWDriver/Async/AsyncSubsystemBusReset.cpp +++ b/ASFWDriver/Async/AsyncSubsystemBusReset.cpp @@ -172,12 +172,15 @@ void AsyncSubsystem::FlushATContexts() { return; } - // Callers reach here only after StopATContextsOnly(), so hardware can no - // longer write descriptor status. Anything still queued was left unsent by - // the bus reset (OHCI 1.2 draft clause 7.2.3.3) and must be reported as - // evt_flushed rather than waited on — the same ordering Linux uses - // (context_stop() then at_context_flush(), ohci.c:2000-2010) and Apple uses - // (stopDMA() then resetDMA() in handleBusResetInt()). + // Callers reach here only after StopATContextsOnly(). Once ACTIVE is clear, + // hardware can no longer write a final descriptor status. OHCI 1.2 draft + // §7.2.3.3 (p. 7-14) permits optional controller behavior that leaves + // outstanding AT descriptors without a final status after a bus reset. + // Complete those descriptors as evt_flushed rather than waiting forever. + // Linux uses the same stop-then-flush ordering and maps zero/no-status + // entries to RCODE_GENERATION (context_stop() then at_context_flush(), + // ohci.c:2000-2010); Apple uses stopDMA() then resetDMA() in + // handleBusResetInt(). // // Confirm ACTIVE is clear per context before flushing. Both references stop // both contexts unconditionally and then flush unconditionally, but neither @@ -214,9 +217,11 @@ void AsyncSubsystem::FlushATContexts() { return; } - const uint32_t flushed = DrainTxCompletions("bus-reset-flush"); - if (flushed > 0) { - ASFW_LOG(Async, "FlushATContexts: flushed %u unsent AT packet(s)", flushed); + const uint32_t drained = DrainTxCompletions("bus-reset-flush"); + if (drained > 0) { + ASFW_LOG(Async, + "FlushATContexts: drained %u AT completion(s) during bus-reset flush", + drained); } } diff --git a/ASFWDriver/Async/Contexts/ATContextBase.hpp b/ASFWDriver/Async/Contexts/ATContextBase.hpp index 26fb2feea..b6dcf0d13 100644 --- a/ASFWDriver/Async/Contexts/ATContextBase.hpp +++ b/ASFWDriver/Async/Contexts/ATContextBase.hpp @@ -276,13 +276,15 @@ class ATContextBase : public ContextBase { [[nodiscard]] std::optional ScanCompletion() noexcept; /** - * \brief RAII scope that makes ScanCompletion() report unsent packets. + * \brief RAII scope that makes ScanCompletion() complete statusless descriptors. * * While held, a descriptor whose xferStatus is still zero is reported as - * evt_flushed instead of being treated as pending. OHCI 1.2 draft clause - * 7.2.3.3 leaves such packets in the AT queue once the context stops, and - * the transaction owning them would otherwise wait for a status hardware - * will never write. + * evt_flushed instead of being treated as pending. OHCI 1.2 draft + * §7.2.3.3 (p. 7-14) permits a controller to leave outstanding AT + * descriptors without a final status after a bus reset. This does not prove + * that the corresponding packet was never transmitted, but the transaction + * must still be completed because hardware can no longer update it once the + * context is stopped. * * \warning The context must already be stopped. Entering this scope while * hardware can still write a status would report packets that are @@ -664,14 +666,15 @@ std::optional ATContextBase::ScanCompletion() noexce return std::nullopt; } - // OHCI 1.2 draft clause 7.2.3.3: after the context stops, hardware may - // leave packets in the AT queue that it never transmitted and will never - // write a status for. Reporting them as evt_flushed is what releases the - // ring and fails their transactions instead of letting them time out. - // Linux does exactly this by re-running handle_at_packet() with - // ctx->flushing set (ohci.c:1364, at_context_flush() at 1331-1343); - // AppleFWOHCI's resetDMA() frees every pending ATxElement after stopDMA() - // (symbol offsets 0x5fae-0x5fd2). + // OHCI 1.2 draft §7.2.3.3 (p. 7-14): after a bus reset, optional + // controller behavior may leave outstanding AT descriptors without a + // final status. Reporting zero-status descriptors as evt_flushed + // releases the ring and fails their transactions instead of letting + // them time out; it does not imply that the packets were never sent. + // Linux uses the same flush-mode scan shape, mapping zero/no-status + // entries to RCODE_GENERATION (handle_at_packet() at ohci.c:1364, + // at_context_flush() at 1331-1343). AppleFWOHCI's resetDMA() frees every + // pending ATxElement after stopDMA() (symbol offsets 0x5fae-0x5fd2). if (state.xferStatus == 0 && flushing_) { state.xferStatus = static_cast(OHCIEventCode::kEvtFlushed); } diff --git a/tests/async/ATContextCompletionTests.cpp b/tests/async/ATContextCompletionTests.cpp index 307345593..7cfee63a5 100644 --- a/tests/async/ATContextCompletionTests.cpp +++ b/tests/async/ATContextCompletionTests.cpp @@ -155,11 +155,12 @@ TEST_F(ATContextCompletionTest, AdvancesTrulyOrphanedPendingDescriptor) { EXPECT_TRUE(ring_.IsEmpty()); } -// A stopped context will never write a status, but an ordinary scan must still -// leave the chain alone: FlushScope is what reports it and fails the owning -// transaction. A scan that landed between StopATContextsOnly() and -// FlushATContexts() (the watchdog drain runs on a timer) would otherwise consume -// the descriptors first, leaving the transaction to time out instead. +// Once a context has stopped, hardware can no longer write a final status, but +// an ordinary scan must still leave the chain alone: FlushScope is what reports +// it and fails the owning transaction. A scan that landed between +// StopATContextsOnly() and FlushATContexts() (the watchdog drain runs on a timer) +// would otherwise consume the descriptors first, leaving the transaction to +// time out instead. TEST_F(ATContextCompletionTest, HoldsQuiescedChainSoTheFlushCanReportIt) { PrepareBlockWriteChain(0); hardware_.SetTestRegister(Async::ATRequestTag::kControlSetReg, 0); @@ -186,7 +187,7 @@ TEST_F(ATContextCompletionTest, HoldsQuiescedChainSoTheFlushCanReportIt) { // chain is reported as evt_flushed, which TransactionCompletionHandler maps to // kIOReturnAborted — Linux calls the equivalent RCODE_GENERATION "the same error // as when we try to use a stale generation count" (ohci.c:1387-1393). -TEST_F(ATContextCompletionTest, FlushScopeReportsUnsentChainAsFlushed) { +TEST_F(ATContextCompletionTest, FlushScopeReportsStatuslessChainAsFlushed) { PrepareBlockWriteChain(0); hardware_.SetTestRegister(Async::ATRequestTag::kControlSetReg, 0); @@ -202,7 +203,7 @@ TEST_F(ATContextCompletionTest, FlushScopeReportsUnsentChainAsFlushed) { EXPECT_TRUE(ring_.IsEmpty()); } -// A genuinely completed packet keeps its real status while flushing; only the +// A descriptor with a real completion keeps that status while flushing; only // zero-status descriptors are synthesized. TEST_F(ATContextCompletionTest, FlushScopePreservesRealCompletionStatus) { PrepareBlockWriteChain(); @@ -220,8 +221,8 @@ TEST_F(ATContextCompletionTest, FlushScopePreservesRealCompletionStatus) { // FlushATContexts() gates FlushScope on IsActive(). This pins the property that // gate depends on: a context whose ACTIVE bit is still set must not be flushed, -// because its queued packets are in flight rather than stranded, and clearing -// their descriptor words would cut the chain the controller is traversing. +// because its queued packets may still be in flight, and clearing their +// descriptor words would cut the chain the controller is traversing. TEST_F(ATContextCompletionTest, ActiveContextIsNotEligibleForFlush) { PrepareBlockWriteChain(0); hardware_.SetTestRegister(