akiko: keep CD32 NVRAM dirty when the backing-file write fails#266
Merged
Conversation
Clearing the dirty bit before the write meant a transient host error (full disk, missing directory, permissions) silently dropped the EEPROM contents: no later I2C STOP would retry, so nothing persisted until the guest wrote the NVRAM again. Clear it only after the write lands; a failure leaves it armed and the flush retries at the next STOP. Retries stay naturally rate-limited because only a STOP with dirty contents attempts a write. Same fix as the RP5C01 battmem flush in #265, which inherited the pattern from here.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes CD32 Akiko NVRAM durability by ensuring the dirty bit is only cleared after a successful backing-file write, so transient host filesystem errors don’t silently drop EEPROM contents (e.g., save games) and a later I2C STOP will retry the flush.
Changes:
- Update
Nvram::stopto keepdirtyset whenstd::fs::writefails, clearing it only after a successful write. - Add a unit test that simulates a failed write (missing parent directory) and verifies a later STOP retries and persists the data once the directory exists.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Match the temp-naming convention the other filesystem-touching tests in this module use (pid + atomic counter). The pid-only name could not collide today -- only this one test uses it and the harness runs it once per process -- but the suffix keeps the name safe if another test ever reuses the pattern.
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
The CD32 NVRAM flush (
Nvram::stop) cleared its dirty bit before attempting the backing-file write, so a transient host error -- full disk, missing directory, permissions -- silently dropped the EEPROM contents: no later I2C STOP would retry, and nothing persisted until the guest happened to write the NVRAM again. Save games are exactly what lives here, so a one-off failure could quietly lose them.The dirty bit is now cleared only after
std::fs::writesucceeds; on failure it stays armed and the flush retries at the next STOP. Retries are naturally rate-limited because only a STOP with dirty contents attempts a write.This is the same durability fix Copilot flagged on the RP5C01 battmem flush in #265 -- that code had inherited the clear-before-write pattern from here, so this brings the original in line.
Testing
nvram_retries_the_backing_file_write_after_a_failure: points the backing file into a not-yet-created directory, drives a page write over the bit-banged I2C lines, and checks the failed STOP leaves the state dirty, a later STOP lands the file once the directory exists, and the contents match the written byte.cargo clippyandcargo fmt --checkclean.