fix(config-rom): reject suspended generation exports - #87
Draft
gly11 wants to merge 10 commits into
Draft
Conversation
gly11
force-pushed
the
sbp2/config-rom-generation-safety
branch
2 times, most recently
from
July 26, 2026 10:43
808c5a4 to
a0cfbcf
Compare
gly11
marked this pull request as ready for review
July 26, 2026 13:57
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b59542c8f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
gly11
marked this pull request as draft
July 26, 2026 14:31
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.
gly11
force-pushed
the
sbp2/config-rom-generation-safety
branch
from
July 28, 2026 12:14
5814bea to
1847198
Compare
SuspendAll() gets its first production caller in this PR, which makes the rest of the ROM lifecycle worth checking. ValidateROM() is the only transition out of Suspended and has no callers anywhere — not in the driver, not in tests — so ROMState::Validated was unreachable and the `state == Validated` arms of SuspendAll() were dead. Rediscovery already does the work ValidateROM() was written for. Insert() overwrites romsByGuid_[guid] wholesale, state included, and romsByGenNode_ is keyed by (generation, nodeId), so after a reset the new generation gets fresh keys and the suspended entries are simply never looked up again. Wiring ValidateROM() up instead would put a second writer on the same entries and require deciding which one wins when Insert()'s richer-ROM guard declines an update — a second source of truth for no behavior gained. Nothing observes the difference today: FindByGuid() does not filter on state, and FindByNode(gen, nodeId, allowSuspended=false) looks up the current generation, whose key never matches a suspended entry. Remove the function, the enum value, and the dead arms, and say in both the enum comment and ConfigROM/README.md how a device actually comes back. If the store later needs to report which devices did not return, that closes the Suspended → Invalid → PruneInvalid loop and deserves its own change with hardware validation.
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
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
Problem
ExportConfigROMdocuments exact-generation lookup with no stale fallback, but it called theConfigROMStoreoverload that returns suspended entries. A bus reset between a client generation read and selector 14 could therefore return a complete ROM from the previous generation and let the client reuse stale node routing information.The cache also remained export-visible until later rediscovery work updated it, leaving a reset window in which stale bytes could still be returned.
Fix
Use
FindByNode(generation, node, false)in the user-client handler. The existing not-cached path then returns empty data for suspended entries, while internal callers can still opt into suspended cache access.On the controller's bus-reset edge, suspend the ROM cache alongside the existing route and lease invalidation, before protocol producers are notified. The migration preserves
dev's current reset ordering.Verification
ConfigROMStoreConcurrencyTests: 8/8 passedControllerCoreBusResetTests: 1/1 passedgit diff --checkdevheadDependency and merge order
Depends on #88. This PR targets
devand is stacked directly on the current #88 head.This PR is a sibling of #84, not a child of it: #84 and #87 have no code dependency and may be reviewed or merged in either order after #88.
Until #88 merges, GitHub's Files changed view will also include #88's CI/project-generation changes. The #87-specific review scope is the top three Config ROM commits and these five files:
ASFWDriver/Controller/ControllerCoreInterrupts.cppASFWDriver/UserClient/Handlers/ConfigROMHandler.cpptests/core/CMakeLists.txttests/core/ControllerCoreBusResetTests.cpptests/discovery/ConfigROMStoreConcurrencyTests.cppIntended merge order:
dev;devtip if GitHub does not collapse the stacked diff automatically, retaining only the three Config ROM commits;