Post-review cleanup batch: MOTU counters, shared identity tables, stale comments - #125
Merged
Merged
Conversation
…path The SPH-capture block added its own rxReplayEntries increment, but the unconditional increment further down already counts every packet that publishes a replay entry. A MOTU master stream therefore reported twice the replay entries it had really published, in a counter that reaches asfw_get_audio_stream_health and the diagnostics ring. No behavioural change beyond the counter; motuTimingEstablished_ still latches on the same condition.
AudioProfileRegistry::FindProfile defaulted every MOTU device that was not an
828mk2 to the UltraLite profile. Its comment defended this on the grounds that
DeviceProfiles had already matched the unit directory — but that is a different
registry; this one matches the vendor OUI alone and never consults DeviceProfiles.
Protocol v2 covers five models and their fixed chunk counts do not generalise
(Linux motu-protocol-v2.c:274-320). The 896HD and Traveler are {14,14,8}, which
happens to coincide with {14,14,0} below 176.4 kHz. The 8pre does not coincide at
all: tx {10,10,0} against rx {6,6,0}, asymmetric between directions, which a
single pcmChunks value cannot express.
Return nullptr outside the 828mk2 and UltraLite instead. Unreachable today,
because DeviceProfiles resolves kNone for the other three so they never get a
nub — but that is a guard in another layer, not a property of this function.
Also corrects MotuV2Profile's claim that geometry is shared across the v2
fixed-chunk models, which reads as an invitation to enable the 8pre by adding one
case to a name switch.
kOnyxIDefaultStartRateHz was documented as a start default that explicit user
rate selections would override. It is not: EffectiveStartClockForProfile returns
it unconditionally for the Onyx-i, discarding request.desiredClock the same way
the Duet pin does, and AVCDiscovery publishes sampleRates = {44100} so there is
no other rate to select in the first place.
The comment describes the state after the M4 48 kHz work and before c0e5da6
reverted it, so it has been wrong since that revert. Left a pointer to the
revert, since whoever restores rate switching has to undo this pin too.
Comment only.
…arding RemoveObserver's bounded wait exists to stop Publish() being inside an observer while that observer is destroyed. Two things about it were undocumented. A false return is a use-after-free hazard rather than something to log: the EfcTransport destructor reports it and then frees its lock and returns anyway, with a Publish() potentially still inside OnResponse. And the bound is large because the window is large. OnResponse ends in Complete(), which calls the caller-supplied completion function, so the inflight region spans arbitrary client work -- not a short critical section. That makes the wait an IODelay spin of up to 100 ms on the teardown queue, and it also means shrinking the timeout would convert waits that currently succeed into use-after-frees. The fix is to shrink the window, not the bound: run Complete() outside the observer callback, leaving only the decode and sequence match inside it. Recorded here so the obvious-looking change is not attempted first. Comment only.
The class comment said the per-data-block SPH cache lives with the consumer that fills it. It does not — it lives in AudioTransportControlBlock, because the transmit side drains on the audio queue what the consumer fills on the transport queue, which is precisely why it cannot live on either side. The second was a doc comment left behind when the member it described moved to the control block: three lines describing a field that is not there, immediately above the closing brace. Both are removed by the planned RX format extraction, which deletes this MOTU paragraph entirely. Fixed anyway so the record is right in the meantime and the correction survives in history. Comment only.
DeviceStreamModeQuirks declared its own kLoudMackieVendorId = 0x000ff2 while the same change that added the vendor-wide blocking rule also added kMackieVendorId to AudioDeviceIds. The values agree today; two spellings of one OUI can only drift apart. Alias the shared constant instead, keeping the file's local-name style for the call site.
RemoveObserver's bounded wait is what stops an observer being destroyed while Publish() is still inside it, but nothing tested either outcome. Three cases: that it actually waits for an in-flight publish rather than returning immediately; that it returns false when the observer outlasts the bound, which is the use-after-free condition EfcTransport then proceeds through; and that a removed observer is not entered by a later publish. The failure case takes ~100 ms of real time, since IODelay sleeps under ASFW_HOST_TEST and the bound is 2000 x 50 us. That is the point -- it confirms the wait reaches its limit rather than short-circuiting.
…shared table The file respelled each vendor and model id locally, so every identity it matches on was a second copy of one the rest of the driver already holds in DeviceProfiles::Audio. The values agree today; nothing keeps them agreeing. Alias the shared constants instead, keeping the local names so the rules below still read as rules. Extends the Mackie change in the previous commit to Apogee, Focusrite and Midas, which had the same duplication for longer. No behavioural change: every value is identical (the Apogee pair differed only in hex case).
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.
The cleanup backlog accumulated while reviewing #101 / #107 / #114 / #118. Eight commits, each independent and separately reviewable.
Defects
fix(audio): stop double-counting rxReplayEntries on the MOTU capture path— the counter was incremented in both the shared path and the MOTU block, so the MOTU capture path reported twice the real entry count. Every diagnosis that reads this counter on MOTU hardware has been reading an inflated number.fix(audio): bound the MOTU profile fallback to the two models it fits— the fallback applied to any MOTU device, including models whose geometry it does not describe.fix(audio): take the Loud/Mackie OUI from the shared table— a second, hand-written copy of the OUI.Consolidation
refactor(audio): take every DeviceStreamModeQuirks identity from the shared table— same problem class as the OUI above: vendor/model identities written out again next to the table that already holds them.Comments that were wrong
Three commits correcting stale comments (the MOTU offset cache, the Onyx-i start rate) and one recording what the EFC mailbox teardown wait is actually guarding — the existing comment described a different mechanism than the code implemented.
Test
test(audio): pin the EFC mailbox teardown race, including the UAF path— the race this suite could not previously express.Rebased onto main after #123. No conflicts.