bus/irm/isoch/ohci: four device-independent fixes from #107 and #114 - #117
Merged
Merged
Conversation
Fourth hardware contact (v18, Mackie Onyx 400F, 2026-09-13): the whole AV/C+CMP start now succeeds — EFC transport mode and clock, IRM channel and bandwidth, iPCR/oPCR connected and verified, IR/IT contexts running, coordinator in Running — and then the ADK times out waiting for the first hardware zero-timestamp while the IT context reports its 48 primed packets and zero interrupts. Nothing on the bus generates cycle starts: the Fireworks unit is IRM-capable and won IRM, the host (ClientOnly / ObserveOnly) is root, and CyclePolicyCoordinator suppressed the local cycle master because the host is neither BM nor fallback IRM. The OHCI confirmed it: LinkControlSet 0x00100600 (cycleTimerEnable set, cycleMaster clear) with NodeID reporting root. Both reference stacks enable the local cycle master on the local-root transition with no bus-management precondition (Linux ohci.c:1907-1910 bus_reset_work; Apple IOFireWireController.cpp:3366-3367 — AssignCycleMaster only chooses who is root). It is a local LinkControl bit, not a bus transaction, so it is now planned ahead of the BM/IRM activity ladder, which keeps guarding elections, PHY config packets and remote STATE_SET writes. The previously unreachable duplicate local-root block below the ladder is removed; not-root clearing is unchanged. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> (cherry picked from commit 48d7e4044f80e82f4c15db844cfd2837e14d131f) (cherry picked from commit 3550238)
…ways try the channel release IRMClient treated an async timeout like a definitive failure: the first read or lock the IRM never answered aborted the whole allocate/release, and a failed bandwidth release skipped the channel release entirely. Apple re-issues every timed-out async command kFWCmdDefaultRetries (3) times (IOFWAsyncCommand.cpp:425-461, IOFWCommand.h:37) and Linux loops its IRM compare-swap up to 5 times on any non-generation failure (core-iso.c:296-320); Apple's releaseChannelComplete also carries on to the channel "error or not" (IOFWIsochChannel.cpp:1312-1320). Field-found on the Mackie Onyx 400F (2026-09-13): the device drops async requests issued in the same millisecond an isoch context starts or stops, which is exactly when the stop path releases its reservation. With the watchdog now ticking, that cost a 500 ms timeout and leaked 884 bandwidth units plus the channel on the IRM until the next bus reset. - RetryPolicy gains maxTimeoutRetries (default 3, None() = 0). A timed-out read or lock restarts the read+lock step within that budget; a generation change is never retried. - A bandwidth release lock whose response was lost may already have been applied. The fresh read detects that (register already holds the value we tried to write) and completes without crediting the IRM twice. Allocations get no such shortcut: a duplicate allocation only under-subscribes until the next reset, whereas a false claim would not. A channel release re-applies as a no-op; an allocation whose lock was applied but unanswered reads as taken and reports NoResources. - ReleaseResources proceeds to the channel after a non-generation bandwidth failure and reports the first failure, so a leaked reservation stays visible to the caller. Tests: new tests/irm/IRMClientRemoteRetryTests.cpp (13 cases) drives the client against a fake remote IRM whose reads and locks can be told to time out, apply-then-lose the response, or conflict once. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> (cherry picked from commit df728c10c7886070470f8d49b2f6db9dbd6e1c17) (cherry picked from commit b672030)
…nd realign instead of slipping IsochTxDmaRing measured consumption from the IT context's command-pointer index modulo the 48-descriptor hardware ring. A completion handler that runs a full ring (6 ms) or more late therefore saw a delta aliased modulo 48 while the hardware had already wrapped and re-sent every descriptor it passed; the refill under-filled by a ring, the software fill index kept its old value, and every later packet went out 48 cycles later than its index said. The content layer's exposure timeline lost 48 cycles of frames per lap, permanently and silently: no fatal, no underrun counter (the descriptors were all committed), and the completion-delta high-water is capped below 48 by the same aliasing. Field-found 2026-09-13 (Mackie Onyx 400F, M3): three host stalls of 2-3 ms each cost the TX exposure lead 261, 267 and 259 frames (48 x 5.51 = 264.6) in one session and three rings' worth in another; the lead reached zero and playback turned choppy; a stream restart cured it. Linux never meets this case: it queues packets rather than running a self-linked ring, and aborts the PCM (cancel_stream -> amdtp_stream_pcm_abort) when it falls behind. The OUTPUT_LAST status word of a completed descriptor carries the 13-bit cycleCount of the cycle it went out in (OHCI timeStamp; Linux ohci.c handle_it_packet reads the same field) and is rewritten on every transmission, so two readings measure true elapsed cycles between two completed packets. One packet goes out per cycle: lag beyond the packets the aliased cursor counted in between is lost cycles, and every kNumPackets of it is a lap. Three earlier cuts failed on the same hardware. One trusted every word and a single stale one produced a 1066-lap correction that ran the fill index past the producer and stopped the stream with an uncommitted slot. The next folded the stamp's bits 15:13 in as seconds and refused nearly every reading, so three real laps went through unseen. The third worked in the 13-bit domain but with an unsigned age, and its refusal log showed why both had been blind: on the Apple Thunderbolt adapter the stamp of the last completed descriptor is consistently two cycles AHEAD of a CycleTimer read made in the same refill, so an unsigned age wraps to 7998. DetectRingLaps therefore works in the 13-bit cycleCount domain only, and accepts a reading only when its signed distance from the cycle timer read in the same refill lies in -16..+24 cycles (falling back to the previous descriptor when the last one is not fresh), no older than the packets the aliased cursor counted since the baseline packet, and plausible in size (laps <= shared ring / hardware ring - 1); a pointer back at the same place with a rewritten stamp counts as a sighting too. A lap is acted on only when the next accepted reading still shows it; refused readings never touch the baseline; a confirmed lap is realigned only if the fill index stays within committedEnd, otherwise it is logged and the old slip is kept. Every refusal and every first sighting is recorded ring-only with its raw numbers, rate-limited. - Realigning advances the completion cursor over the lapped packets, skips them in the software fill index, and pushes stamps only for the packets that were really sent. - Counters on the ring (RTCounters) and the shared IsochTxQueueControl: ringLaps, ringLapPacketsSkipped, lostCycles; refused-reading and fallback counters on the ring. The IT stop line prints them and a ring-only warning names each confirmed lap with its numbers. Tests: fourteen IsochTxDmaRingLapTest cases (no lap, confirmed lap plus a second lap from the realigned baseline, exact lap with an unchanged pointer, lost cycles short of a ring, cycle wrap, stale stamp refused, inconsistent stamp refused, contradicted sighting dropped, implausible lap count refused, upper stamp bits ignored, in-flight fallback, stamp leading the cycle timer by two, previous-ring word with that lead, lap beyond committedEnd not realigned), fixture in production order (reset, seed, prime). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> (cherry picked from commit 57bb3237e49dfadb9834855b2a333c901ddabb4b) (cherry picked from commit b521705)
Becoming the IRM exercised the local CSR path for the first time, and it corrupts the registers it reads: [IRM] CHANNELS_AVAILABLE_31_0 selector=2 addr=0xf0000224 value=0xffffffff [IRM] Allocating channel 0 (addr=0xf0000224 bit=0x80000000) [IRM] CHANNELS_AVAILABLE_31_0 selector=2 addr=0xf0000224 value=0x00001332 [IRM] Channel 0 not available (current=0x00001332 mask=0x80000000) 0x00001332 is a bandwidth-shaped value (BANDWIDTH_AVAILABLE is 0x00001333); no channel allocation can produce it, since those only clear single bits from 0xfffffffe. The value also persists into the next StartIO attempt, so every subsequent channel reservation fails with NoResources. OHCI 1.1 §5.5.1: the CSR resource mechanism has no read-only mode. Writing kCSRControl always performs a compare-swap using whatever kCSRData and kCSRCompareData currently hold. ReadLocalIRMResource wrote only kCSRControl, so each read ran a swap against values left by an earlier operation; whenever that stale compare matched the target register, the stale data was silently written into it. Linux writes all three registers unconditionally and zeroes both data registers for a plain quadlet read (ohci.c:1504-1517, handle_local_lock). Comparing against zero either mismatches -- no swap -- or matches a register already holding zero and writes zero back, so it is a no-op for every value. Do the same. HardwareInterfaceOrderTests pins the MMIO sequence and correctly caught this; update both read phases to the corrected order. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit 11885ca)
mrmidi
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
3 participants
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.
Three commits from @ottendorfcipher's #107 and one from @deweydb's #114, none of which relate to the Mackie or MOTU hardware they were found on. They fix bugs that are live on other devices, so they're going in ahead of the device support rather than waiting on it. Authorship preserved via
cherry-pick -x; both PRs remain open for their device work.9af4c12b— a local root always enables its own cycle master.CyclePolicyCoordinatorsuppressed the local cycle master unless the host was BM or fallback IRM, and thelocalIsRootblock below that ladder was unreachable. A host that is root with no BM on the bus generated no cycle starts at all, and every isochronous start timed out. Root duty is a localLinkControlbit, not a bus transaction, so it now precedes the activity ladder — which still guards elections, PHY config packets and remoteSTATE_SETwrites. Matches AppleIOFireWireController.cpp:3366-3367and Linuxohci.c:2052-2056.3ad56c1b— IRM re-issues reads and locks the IRM never answered. A single timed-out async command aborted the whole allocate/release, and a failed bandwidth release skipped the channel release entirely, leaking both until the next bus reset. Apple retries 3×, Linux loops its compare-swap 5×. A release whose response was lost may already have applied, so the fresh read detects that; allocations deliberately get no such shortcut.ac120258— zero the CSR data registers before a local IRM resource read. The OHCI CSR resource mechanism has no read-only mode: writingkCSRControlalways performs a compare-swap using whateverkCSRDataandkCSRCompareDatacurrently hold. Triggering it without setting both runs the swap against stale values from the previous operation, and whenever that stale compare happens to match the target register, the stale data is silently written into it. That is how a bandwidth-shaped0x00001332ended up inCHANNELS_AVAILABLE_31_0— a value no channel allocation can produce, since those only clear single bits — after which every channel reservation failed. Linux writes all three registers unconditionally and zeroes both data registers for a plain quadlet read (handle_local_lock). This is a different bug from3ad56c1b: that one is about retrying transactions the IRM never answered, this one is about corrupting the register before the transaction even starts.60c036bf— detect IT hardware-ring laps and realign instead of slipping.IsochTxDmaRingmeasured consumption as the command-pointer index modulo the 48-descriptor hardware ring. A completion handler running ≥6 ms late saw an aliased delta while the hardware had already wrapped; the refill under-filled by a ring and every later packet went out 48 cycles late. The exposure timeline lost 48 cycles of frames per lap, permanently and silently — no fatal, no underrun counter, and the completion-delta high-water capped below 48 by the same aliasing.Reproduced independently on a Focusrite Saffire Pro 24 DSP, a different device and protocol family from where it was found:
skipped=144is exactly 3 × 48, one full hardware ring per lap. Same session:maxLatUs=3215,late1500=6— the 2–3 ms host-stall class that triggers it. Without this commit those three laps would have cost ~790 frames of TX exposure lead with nothing registering it.refusedStamps=0/0/0andunrealignable=0mean the detector never rejected a reading or bailed on a realign — no false positives.Supersedes the cycle-master changes in #114 (
kDefaultLinkControlonly — misses Linux's per-self-ID re-arm) and #116 (written from the audio backend, never cleared). Three independent hardware contacts hit that bug: MOTU UltraLite twice, Onyx 400F once.Verification: all four cherry-pick clean onto main,
1621/1621host tests pass, build clean. 30 new tests from the first three (2 cycle-policy, 13 IRM, 14 ring-lap);ac120258extendsHardwareInterfaceOrderTestsrather than adding a case, so the total is unchanged. Saffire Pro 24 DSP and Apogee Duet both run clean on this build, cold boot included. The cycle-master change is inert on that topology — the Saffire wins root (local=0 root=1 irm=1 bm=63 cycleSeen=1), so the new branch correctly doesn't fire — meaning this validates no-regression, not that path's efficacy.