Repository navigation
fix: reads were silently landing at addr>>8 after any contact flicker - #18
Merged
Merged
Conversation
…d connection A LIBUSB_ERROR_IO on a bulk endpoint is usually a STALL, and a stall is latched: once the endpoint halts, every later transfer fails identically until the host clears it. Nothing cleared it, so a single glitch turned every subsequent command into an error and killed the run. That is what kept ending `ratchet monitor` at read 8 of 500. Clear the halt on both endpoints when a transfer returns Io, then hand the error back unchanged. Deliberately no retry at this layer: every caller is mid-CS-frame, so re-issuing would splice a second SPI transaction into a frame the chip is already interpreting and hand back bytes that look valid and are not. The retry belongs to the caller, which re-frames CS properly. Monitor now records a USB error as a reading and carries on instead of dying, and reports how many it saw. Also fixes a bug this exposed in the integrity scorer. A run where EVERY transfer failed is perfectly consistent, so it scored 100 and reported "Stable: connection is solid, safe to proceed with read/write operations". Add SpiPattern::LinkError, checked before the score branches, so a dead link scores 0 and says the probe is not the problem. Same symptom to a human as a silent chip, opposite fix.
…ng bus
Reads wait out contact dropouts per chunk, but the read still had to be STARTED,
and identify failed closed in ~11 ms when the probe was not already touching:
$ ratchet read chip-backup-2.bin
Error: chip not detected
0.011 total
So the user had to press enter inside a contact window they cannot see, because
the pads on a WSON part are underneath the package. That defeats the point of
waiting at all. identify_chip_patient polls for contact on the same budget the
chunks use, so the command can be started first and the probe seated second.
Waiting costs nothing here for the same reason it costs nothing per chunk: RDID
against a silent chip is a successful USB transfer reporting 000000, not an error
path. Paced with an unconditional sleep rather than built on wait_for_contact,
which returns the instant RDID answers and would hot-spin at full USB rate on a
bus flickering between a valid id and a dead one.
Waiting before identify exposed a second and worse bug. chip_answers only rejected
000000. That is fine once identify anchors an id, since a floating bus then fails
the equality check, but identify_chip_patient waits BEFORE any id exists, where
jedec_expect is None and every guard is is_none_or(true). ffffff is MISO floating
high, it is byte-for-byte identical to erased flash, and it is what a lifted probe
reads on this part. A floating probe would have read as a healthy chip and handed
back a dump of 0xff as a successful backup, passing every blank check. is_dead_bus
rejects both values, and both liveness sites now use it.
Both fixes are mutation-checked: reverting either one turns exactly one new test
red, for the right reason.
…ead at addr>>8 A 32 MB read held contact for 12.6 minutes, passed every gate, reported success, and was 128 KB of chip smeared across 32 MB. Measured on the artifact: 99.22% self-similar at chunk_size - 32, zero _FVH, zero "American Megatrends", zero $PSP. The address ratio is exactly 256, which is addr>>8, which is a 4-byte address sent to a chip in 3-byte mode. read_one_chunk hardcoded plain READ (0x03). That opcode is mode DEPENDENT, and the mode is chip state we do not control: a chip that loses power reverts to its 3-byte power-on default while use_4byte_addr stays true in software. It then consumes 3 of our 4 address bytes as the address and the 4th as data. A flickering probe IS a power interruption. Since reads now wait out flickers rather than dying on them, the read resumes against a chip that silently reset, which means the contact-waiting added recently is what turned this latent bug into a corrupt dump. Nothing downstream could catch it: RDID answers identically in both modes so the liveness check is blind, and the wrong bytes are deterministic so the per-chunk double-read agrees with itself. The fix is the one page_program and erase already had. SPI_READ_4B (0x13) was defined and never used in production, while PAGE_PROGRAM_4B and SECTOR_ERASE_4B were wired up properly. 0x13 carries 4 address bytes by definition, so it cannot be misframed by chip state, and it makes reads consistent with the write path rather than betting on anything new. read_range shares read_one_chunk, so verify is fixed too. The fake now models the physics it was missing: contact recovery resets four_byte, because losing contact is losing power. Without that, contact loss looked free. Mutation-checked: restoring 0x03 fails the new test with 33227276 of 33554432 bytes wrong while still reporting success, which is the reported dump exactly. The other 65 tests in the module pass under that mutation, which is why this shipped.
jackulau
changed the title
…size
Two bugs in identify_chip_patient, both reported from a real terminal, both in the
wait added earlier today.
A USB error is a transient LINK fault: a stall just cleared, a re-enumeration. It
says nothing about whether the probe is touching. Propagating it with `?` killed a
read whose entire job was to wait, about 2 seconds into a 600 second budget:
no chip answering yet - waiting up to 600s for the probe to make contact.
Error: usb error: I/O error
chip_answers already treats a failed RDID as "not answering"; this now agrees.
An identification we cannot SIZE is not an identification. An id read through a
marginal contact window can be self-consistent garbage that misses both the chip DB
and SFDP, leaving size_bytes 0, which the caller turns into ChipNotDetected. So the
wait announced success and then contradicted itself one line later:
chip is answering - starting the read
Error: chip not detected
Keep waiting for an id we can actually act on.
Mutation-checked: restoring either behaviour fails the matching new test with the
exact field error, Usb(Io) and ChipNotDetected respectively.
… 11ms read learned to wait for contact; write and verify did not. They identified the chip once, in about 11ms, and gave up. On a WSON part the pads are underneath the package, so that made "seat the probe, THEN press enter" the only workable order, inside a contact window nobody can see. Same trap the read path had, one command later. Both now look first and wait only on a miss. Looking first matters: an unconditional poll adds an RDID to every write, which the mock write tests caught immediately by having their queued identify response eaten. A seated probe should cost nothing. write deliberately does NOT use identify_chip_patient. That helper keeps waiting for an id it can SIZE, which is right for a read (an id read through a marginal window is self-consistent garbage) but wrong here: a genuinely unsupported chip would burn the whole patience budget and then report "chip not detected" while sitting there answering RDID. The existing unknown-capacity refusal is the honest answer and is now reachable again. Waiting is safe at this point specifically because it is startup, before any erase. The write itself still refuses to ride out a dropout: that is per-block erase/program/verify plus re-running the command to resume. wait_for_contact now takes the line it prints. The two waits are not the same event - mid-read the news is "contact lost, your finished chunks are safe", while at startup nothing is lost and nothing is on disk to reassure anyone about. Verified by mutation: dropping the wait turns the new test red on its expect().
Making write wait for the probe is only half a fix if the wait cannot be tuned. read has had --patience since the backup work, and it is what makes "start the command, then seat the probe" possible on a WSON part; write and verify were stuck on the 120s default with no way to raise it, which is the same 11ms-fail-closed problem in a slower costume. For write the budget covers both the startup identify and each block's retry, so raising it also buys tolerance for a probe that wobbles mid-write. That is safe: a block that has been programmed stays programmed, and the backup read doubles as the baseline that lets a re-run skip it.
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.
The headline bug
A 32 MB read held contact for 12.6 minutes, passed every gate, reported success, and was 128 KB of chip smeared across 32 MB.
Measured on the artifact:
d[x] == d[x+0x1FE0](chunk_size - 32)_FVH(UEFI volume header)American Megatrends$PSP(AMD PSP directory)The address ratio is exactly 256. That is
addr>>8, which is a 4-byte address sent to a chip in 3-byte mode.read_one_chunkhardcoded plainREAD(0x03). That opcode is mode DEPENDENT, and the mode is chip state we do not control: a chip that loses power reverts to its 3-byte power-on default whileuse_4byte_addrstays true in software. It then eats 3 of our 4 address bytes as the address and the 4th as data.A flickering probe IS a power interruption. Reads recently learned to wait out flickers instead of dying on them, so the read now resumes against a chip that silently reset. The contact-waiting is what turned this latent bug into a corrupt dump.
Nothing downstream could catch it:
The fix is the one the write path already had
SPI_READ_4B(0x13) was defined and never used in production, whilePAGE_PROGRAM_4BandSECTOR_ERASE_4Bwere wired up properly. 0x13 carries 4 address bytes by definition, so it cannot be misframed by chip state. This makes reads consistent with writes rather than betting on anything new.read_rangesharesread_one_chunk, so verify is fixed too.The fake now models the physics it was missing: contact recovery resets
four_byte, because losing contact is losing power. Without that, contact loss looked free.Also in this PR
A latched USB stall killed every run.
LIBUSB_ERROR_IOon a bulk endpoint is usually a STALL, and a stall is latched: every later transfer fails identically until the host clears it. Nothing did, so one glitch killed the run. That is what kept endingratchet monitorat read 8 of 500. No retry at that layer on purpose: every caller is mid-CS-frame.The scorer called a dead link a good connection. A run where every transfer failed is perfectly consistent, so it scored 100 and printed "Stable: connection is solid, safe to proceed".
SpiPattern::LinkErroris checked before the score branches.A read could not be started unless the probe was already touching. identify failed closed in ~11 ms, so the user had to press enter inside a contact window they cannot see (WSON pads are under the package).
identify_chip_patientpolls on the same budget the chunks use.A floating bus read as a healthy chip.
chip_answersonly rejected000000. Safe once identify anchors an id, but the new pre-identify wait runs wherejedec_expectis None and every guard isis_none_or(true).ffffffis MISO floating high, byte-identical to erased flash. A floating probe would have handed back 32 MB of0xffas a successful backup passing every blank check.Verification
538 tests,
clippy --workspace --all-targets -D warningsexit 0, fmt clean.Every new test is mutation-checked and each mutation turns exactly one test red for the right reason:
0x03: fails with 33227276 of 33554432 bytes wrong, still reporting success (the reported dump exactly). The other 65 tests in the module pass under this mutation, which is why it shipped.is_dead_busto000000only: the floating-bus test fails at thechip_answersassertion.ChipNotDetected, the exact error from the bug report.