fix: advertise cPhyEnhance only when 1394a enhancements are enabled - #7
Merged
Merged
Conversation
gly11
marked this pull request as ready for review
April 15, 2026 11:01
2 tasks done
Owner
|
Looks good for me. I've didn't even bothered with phyEnhanceEnabled (for mid 2000 devices is always enabled I guess). Just curious what kind of bizarre hardware do you working with :) |
Contributor
Author
Hi mrmidi, I’ve actually been working with some vintage film scanners that use FireWire. Not exactly the original use case for this project, but the low-level stuff maps surprisingly well. Still figuring out some quirks on my side 😄 |
mrmidi
added a commit
that referenced
this pull request
Jun 12, 2026
fix: advertise cPhyEnhance only when 1394a enhancements are enabled
forkt69
pushed a commit
to forkt69/ASFireWire
that referenced
this pull request
Aug 28, 2026
fix: advertise cPhyEnhance only when 1394a enhancements are enabled
mrmidi
pushed a commit
that referenced
this pull request
Sep 15, 2026
…eardown
FCPTransportTests.RejectsResponseForInvalidatedRouteAfterRebind and
RejectsWriteCompletionFromInvalidatedRoute crashed with SEGFAULT.
Diagnosed with AddressSanitizer as stack-use-after-return, not the stack
overflow the fault address and unwind failure first suggested:
ERROR: AddressSanitizer: stack-use-after-return
#0 ...TestBody()::$_0::operator() FCPTransportTests.cpp:129
#7 FCPTransport::Shutdown() FCPTransport.cpp:301
#8 FCPTransportTests::TearDown() FCPTransportTests.cpp:74
Both tests invalidate the route on purpose, so their command never completes and
is still pending when TearDown() runs Shutdown(). Shutdown() then completes
every pending and queued command with kTransportError -- correct behaviour, it
must not leak outstanding work -- which invokes a completion that captured
`&completionCount`, a TestBody() local whose frame is already gone.
The driver is not at fault and is unchanged. The other tests in this file are
safe only incidentally: their commands complete inside the test body, so
Shutdown() finds nothing pending.
Fix: own the counter on the fixture, so its lifetime spans TearDown.
Verified: both tests pass under ASan with no sanitizer findings, and the full
host suite is 1653/1653 (previously 1651/1653 with these two failing on
unmodified origin/main).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
(cherry picked from commit 171e4afe510114d4b8e152fc5687513f042969c2)
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
2 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.
Summary
cPhyEnhanceonly when 1394a PHY/link enhancements were actually enabled successfullyWhy
The driver currently serializes a fixed node capability value into the local Config ROM even when 1394a enhancements are skipped or fail to initialize.
Peers see
cPhyEnhanceadvertised even though the controller did not actually enable the corresponding enhancement path. In that fallback case, the Config ROM no longer matches the driver's effective runtime state.This change keeps the advertised local node capabilities aligned with the hardware state that was successfully brought up.
What changed
kDefaultNodeCapabilitiesconstant withMakeNodeCapabilities(bool phyEnhanceEnabled)inOHCIConstants.hppStageConfigROM()to passphyConfigOk_into the helpercPhyEnhancecasesScope
Only changes local Config ROM capability advertisement.
Does not change:
Test Plan
cmake -S tests -B build/tests_build && cmake --build build/tests_build --target ASFWConfigROMTests./build/tests_build/ASFWConfigROMTests --gtest_filter="ConfigROMBuilderTests.NodeCapabilities*"— 2/2 passed./build.sh— full Xcode build succeeded (0 errors, no new warnings)🤖 Generated with Claude Code