install: improve NOR install reliability and rootfs_data handling - #140
Conversation
Issue `sf lock 0` after a successful NOR probe and before any selected persistent NOR stage. This matches the established OpenIPC flashing sequence and handles boards where erase/write is rejected with a locked DMA/write area. Keep older U-Boot variants compatible by warning and continuing when the lock subcommand is unavailable; explicit unlock failures still stop the install before destructive writes. Add regression coverage for successful unlock, unsupported legacy `sf` syntax, and hard unlock failure ordering. Hardware verified on a GARUS Hi3516EV200 board with 8 MiB SPI NOR: both a kernel-only install and a full install run executed the unlock successfully; U-Boot, kernel, and rootfs writes passed CRC readback and the factory MAC was preserved.
PR Summary by QodoUnlock SPI NOR before persistent install writes
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Hardware testing exposed two independent reliability failures around persistent installs: incomplete U-Boot command responses could advance the state machine, and a completed TFTP transfer could occasionally leave corrupt RAM contents. Require prompt completion for ordinary shell commands, preserve the explicit download-command result, reject a failed sf probe, and keep reset on its own promptless control-flow path. Verify every TFTP-staged payload before persistent writes by checking the reported transfer size and RAM CRC. Keep retry policy in the named TFTP_RAM_VERIFY_RETRIES constant, defaulting to one retry. When verification fails and another attempt is available, emit an operator-facing warning naming the next attempt and the exact TFTP file being fetched again. Host TFTP switches future RFC2348 negotiation to 512-byte blocks after the first failed verification; pod TFTP retries without host-side blocksize control. Apply the same verified staging helper to U-Boot, kernel, NOR rootfs, and the extracted UBIFS payload on NAND/UBI installs. Phase-1 boot-ROM/SPL/U-Boot upload is a separate transport and is intentionally outside this TFTP policy. Regression coverage deterministically corrupts the first RAM CRC and proves that the same file is fetched a second time, the warning reports Attempt 2, no flash erase happens before the second CRC succeeds, and the host fallback caps TFTP blocks at 512 bytes. Persistent corruption still fails closed before any flash write. Hardware verification remains on this test branch before folding the result into the existing PR.
Generic NOR installs currently preserve rootfs_data, while registered stock U-Boot migrations erase it as part of their migration flow. Keep both existing defaults unchanged. Add --wipe-rootfs-data as an explicit destructive option for operators who want a clean persistent overlay during an otherwise normal install. The flag is independent of --stage selection: requesting it always performs the NOR-tail erase and the existing readback/CRC verification before the install continues. Reject the option on NAND, where this NOR rootfs_data layout does not apply, and reject the contradictory combination with --skip-stage rootfs-data before any device access. Treat an explicit wipe as a persistent operation in the vendor-chainload partial-install guard and in the SPI NOR unlock gate, so a wipe cannot bypass the protection clearing added by the preceding install hardening. Document the preserved defaults and the opt-in wipe, and add focused coverage for the legacy generic rootfs-data stage behavior, explicit wipe execution, unlock/erase/verification ordering, NAND rejection, and conflicting CLI intent. Whether a full generic install should eventually wipe rootfs_data by default is a separate policy decision because changing that default would destroy existing overlay data. This commit deliberately does not make that behavior change.
ArthurKoba
changed the title
ArthurKoba
changed the title
openipc-ai
left a comment
openipc-ai
left a comment
There was a problem hiding this comment.
Thanks for this — all three changes are worth having, the failure you hit is real, and the hardware evidence plus the deterministic regression tests are exactly the right way to land it. My concern is scope rather than intent: the fail-closed hardening was applied to flashdump.send_command, which is shared with callers that have a different contract, so it fixes install and regresses restore and dump-flash.
I verified each of these by reading the branch; the targeted test files (test_install_nor_unlock, test_install_rootfs_data, test_ds_i203_final_contract, test_uart_command_integrity, test_network, test_flashdump — 81 tests) pass, so none of the regressions below are test-covered.
Blocking
defib restorenow ends in a traceback after a successful restore — see the inline note onflashdump.py.- The
sf probeguard no longer catches U-Boot's actual probe failure, so the install proceeds to erase a guessed 8 MiB layout on an unprobed flash. printenv ethaddrreturning non-zero is now fatal in download mode, which makes the generic rescue-MAC path unreachable and kills the install after U-Boot/kernel/rootfs are already on flash.dump-flash's CRC32 capability probe became a hard abort — the backup path is the one thing that should never fail closed.
Should fix before merge
- Shell-mode
_cmd_resulthardcodesok=Trueand ignoresallow_failure, so the whole fail-closed contract only exists in download mode. - The 512-byte TFTP cap set on a retry is never lifted.
- The hardcoded 10 s
crc32timeout is inherited by the multi-MiB NAND/UBI payload and is fatal outside the retry. - The new vendor-chainload guard for
--wipe-rootfs-datahas no test, and it is the destructive-path guard for the new flag.
Worth a look
saveenvafter the in-session reset is re-probed but never re-unlocked — possibly the original bug again, depending on the part.--wipe-rootfs-datais rejected alongside--skip-stage rootfs-databut silently honoured with--stage uboot.
Details inline. Nothing here is a disagreement with the direction — mostly it is about confining the new strictness to the caller that asked for it.
| response = buf.decode("ascii", errors="replace") | ||
| if wait_for: | ||
| partial = response.strip()[-200:] or "<no response>" | ||
| raise TransportTimeout( |
There was a problem hiding this comment.
Blocking. This raise is right for the installer, but send_command is shared with two caller classes that are broken by it.
1. Commands that are expected never to return a prompt. defib restore's Phase 6 runs await _send("reset", timeout=5) (cli/app.py:2982), and in shell mode _send is send_command(transport, cmd, timeout=timeout, wait_for="# ") (cli/app.py:2613). The board reboots and never reprints # , so this now raises after 5 s. _restore_async has only try/finally and restore() calls it through a bare asyncio.run (cli/app.py:2295), so a successful restore now ends in an unhandled TransportTimeout: transport.close() / power_controller.close() are skipped, and neither Restore complete! nor the {"event":"done"} JSON is emitted. The installer got a dedicated _reset_command(wait_for=None) for exactly this case — restore needs the same treatment. The same change also makes every other _send in the restore write loop (e.g. nand erase at timeout=120, cli/app.py:2963) abort mid-restore with a traceback and an erased-but-unwritten partition, where it previously continued.
2. Lenient capability probes, where a timeout is information rather than failure. _detect_crc32 (flashdump.py:430) sends crc32 0 0 with a 3 s budget purely to learn whether the command exists, and used to degrade gracefully to "CRC32 not available — dumping without verification". It now raises out of dump_flash, and both callers (cli/app.py:822, tui/screens/progress.py:557) turn that into Dump failed with nothing written. Over a laggy rack:// or rfc2217:// bridge that turns a working backup into no backup — the operation CLAUDE.md singles out as the one that must always succeed before a write. The pre-loop sf probe (507) and md.b sanity check (527) pick up the same fragility; only the per-chunk read loop is inside a retry.
Suggest an opt-in require_prompt: bool = False (or a separate strict wrapper) used only by the install path, so the new strictness reaches the caller that asked for it and nothing else.
There was a problem hiding this comment.
Fixed in 1e11c83.
Strict prompt completion is now opt-in via require_prompt=True and is used by the installer path only. Existing send_command(..., wait_for=...) callers retain the historical behavior of returning the partial response when the prompt does not arrive.
This keeps restore reset semantics and dump-flash capability probes backward-compatible while still allowing install to require confirmed command completion before persistent writes.
Regression coverage was added for:
a command such as reset that legitimately does not return the old prompt;
legacy partial-response behavior;
dump-flash CRC32 capability probing degrading to unverified mode rather than aborting.
| resp = await _cmd("sf probe 0", timeout=5.0) | ||
| if "error" in resp.lower() or "fail" in resp.lower(): | ||
| await close_and_fail(f"sf probe failed: {resp.strip()}") | ||
| probe_error = uboot_flash_command_error(resp) |
There was a problem hiding this comment.
Blocking — this one loses a safety check. uboot_flash_command_error() is narrower than the "error" in resp or "fail" in resp it replaces, and U-Boot's actual probe failure falls through the gap.
do_spi_flash_probe in cmd/sf.c prints:
Failed to initialize SPI flash at 0:0 (error -2)
Against uboot_flash_command_error (layout.py:67-101) that line matches nothing: it does not start with error: / unknown command / usage: sf / usage: nand, it is not exactly failed or failure, it is not no spi flash selected / out of range / not block aligned, and it does not start with any of the sf:|spi flash|spi nor|nand|erase|write|read result prefixes.
So probe_error is None, the install continues, detect_nor_size_mb returns None, and select_nor_size_mb(0, None, require_detection=False) falls back to 8 MiB (layout.py:271) — the installer then erases and writes a guessed layout on a flash that was never successfully probed. The old "fail" in resp.lower() caught this.
Either keep a fail fallback on this specific call site, or teach uboot_flash_command_error about failed to initialize (a ^failed\b line prefix would cover it without reintroducing the "any failed substring anywhere in the buffer" problem the docstring is guarding against).
There was a problem hiding this comment.
uboot_flash_command_error() now recognizes result lines beginning with Failed, including the actual U-Boot response:
Failed to initialize SPI flash at 0:0 (error -2)
The installer therefore aborts immediately after the failed probe and cannot fall through to the generic 8 MiB size fallback or perform persistent writes against an unprobed flash device.
A regression test covers this exact HiSilicon failure text.
| await close_and_fail( | ||
| f"U-Boot transport failed while running {cmd!r}: {exc}" | ||
| ) | ||
| if not ok and not allow_failure: |
There was a problem hiding this comment.
Blocking. Treating every non-OK download-mode command as fatal is too strong: in U-Boot a non-zero return is a normal outcome for some commands, not necessarily an error.
The concrete case is printenv ethaddr (orchestrator.py:1364). When the variable is undefined, do_env_print prints ## Error: "ethaddr" not defined and returns non-zero, so DownloadCommandClient.send_command sees [EOT](ERROR) and returns ok=False (protocol/download_cmd.py:128-130) — and this close_and_fail then aborts the install.
Before this PR that path only printed a yellow warning and fell through to select_install_ethaddr(current_eth=None, preserved_eth=None, allow_generate=True), which generated a rescue MAC. That branch is now unreachable in download mode, and the install dies at the env stage — i.e. after U-Boot, kernel and rootfs have already been written. printenv mtdparts at line 1421 has the same shape.
Simplest fix: pass allow_failure=True for the printenv probes (they already handle a missing value via parse_printenv_value returning None), and keep the hard failure for the commands that actually write.
There was a problem hiding this comment.
Optional environment reads now use _optional_printenv(), which executes the command with allow_failure=True and distinguishes the expected not defined result from an actual transport or command failure.
This restores the generic rescue-MAC path when ethaddr is absent in download-command mode while keeping unexpected failures fatal.
The same policy is used for optional environment reads such as mtdparts, and the missing-ethaddr download-mode path is regression-tested.
| return True, out | ||
| await transport.write(b"\x03\r") | ||
| await _aio.sleep(0.05) | ||
| return True, out |
There was a problem hiding this comment.
The two _cmd_result closures do not implement the same contract, which undercuts the goal of the commit.
Shell mode returns True, out on every path (here and at line 631), so ok is a constant. That makes if not ok: raise RuntimeError("TFTP command failed or timed out") (line 949) and elif not unlock_ok (line 752) dead code in shell mode — only download mode is actually fail-closed.
And allow_failure is accepted but never honoured here, in the inverse direction: a command that never returns a prompt raises TransportTimeout inside send_command, which is caught by except TransportError at line 625 and turned into close_and_fail — so sf lock 0, which explicitly opts into failure, still aborts the install.
Both would be fixed by having shell mode return ok=False on a prompt timeout (when allow_failure is set) instead of aborting, so the two modes share one contract and the not ok branches mean the same thing in each.
There was a problem hiding this comment.
Fixed in 1e11c83.
Shell and download command modes now expose the same status contract.
Installer shell commands use strict prompt completion. When allow_failure=True, a prompt or transport failure is returned as (False, detail) so the caller can classify it. Without allow_failure, the installer still fails closed.
Unsupported commands are also no longer redundantly retried when the caller explicitly requested failure classification.
This makes the not ok branches meaningful in both shell and download-command modes.
| ) | ||
| else: | ||
| crc_resp = await _cmd( | ||
| f"crc32 0x{ram_addr:x} 0x{len(orig_data):x}", |
There was a problem hiding this comment.
This 10 s budget is inherited from the NOR path, where payloads are at most 24 MiB, but _verify_tftp_ram is now also used for the extracted UBIFS payload on NAND/UBI installs (line 1169), which can be 60-100 MiB.
U-Boot's crc32 is a software loop; on a slow ARM9-class SoC that can exceed 10 s. When it does, send_command raises, _cmd_result catches TransportError and calls close_and_fail — so the install dies before nand erase / ubi write, and the except RuntimeError retry at line 969 does not cover it. Note the UBI path previously did no CRC at all, so a U-Boot build without crc32 now also fails an install that used to succeed.
Suggest scaling the timeout with len(orig_data) (or routing this failure into the retry path rather than straight to close_and_fail).
There was a problem hiding this comment.
The RAM CRC timeout now scales with payload size instead of using a fixed 10-second budget, so large extracted UBIFS payloads get an appropriate timeout on slower SoCs.
CRC timeout and incomplete/unparseable CRC output are now routed through the RAM-verification retry path.
Compatibility with older NAND U-Boot builds that genuinely do not implement crc32 is also preserved: an explicit Unknown command 'crc32' disables the optional RAM checksum with a warning and retains TFTP completion/size validation.
That exception is deliberately narrow. CRC timeout, malformed checksum output, and checksum mismatch remain fatal.
Regression coverage now includes CRC timeout, incomplete output, mismatch, and NAND without crc32.
| ) | ||
|
|
||
| await _cmd("reset", timeout=1.0) | ||
| await _reset_command(timeout=1.0) |
There was a problem hiding this comment.
Worth checking on the EV200 board: sf lock 0 is issued once at line 736, but this reset reboots into the freshly flashed OpenIPC U-Boot, and the first persistent write afterwards is saveenv at line 1399 — with no sf probe 0 and no sf lock 0 re-issued.
The codebase already treats the reset as invalidating SPI state: verify_spi_environment_crc re-probes for exactly this reason (install/layout.py:186-193). Nothing re-unlocks, so if the HiSilicon block-protection state does not survive the reboot, --stage env --wipe-env on a stock-U-Boot migration hits the same ERROR: The DMA write area was locked this PR set out to fix.
Whether it actually bites depends on whether the part's protection bits are volatile — you have the hardware, so defib install ... --stage env --wipe-env would settle it. If it does, folding the unlock into the same helper as the re-probe would keep the two from drifting apart.
There was a problem hiding this comment.
Fixed in 1e11c83.
The in-session reset is now treated as invalidating SPI controller/protection state.
After the fresh OpenIPC U-Boot prompt is detected, the installer now performs:
sf probe 0
→ verifies the probe result
→ runs the centralized NOR unlock path again
→ only then proceeds toward the later saveenv
The final-contract regression test verifies that the second probe and unlock occur after the reset and before persistent environment saving.
Hardware validation can still confirm the exact protection-bit behavior of a particular flash part, but the installer no longer relies on SPI protection state surviving reset.
| for stage in request.skip_stages | ||
| if stage.strip() | ||
| } | ||
| if wipe_rootfs_data and "rootfs-data" in skipped_stage_set: |
There was a problem hiding this comment.
This rejection only inspects request.skip_stages, so the equivalent contradiction expressed through --stage is silently honoured instead:
defib install --skip-stage rootfs-data --wipe-rootfs-data # exit 2
defib install --stage uboot --wipe-rootfs-data # wipes the overlay
CLAUDE.md describes --stage as "an exact subset", so the second form reads as "touch only uboot" to an operator, and erase_rootfs_data at line 1205 erases anyway.
The commit message says the flag is deliberately independent of stage selection — if that is the intent, then this check is the inconsistent half and should go (with the independence documented in the --wipe-rootfs-data help text). Otherwise validate against the resolved stage set so both spellings behave the same.
There was a problem hiding this comment.
Fixed in 1e11c83.
--wipe-rootfs-data now follows the documented exact-stage semantics.
When explicit --stage arguments are supplied, rootfs-data must be included explicitly for the wipe to occur.
Therefore this is now rejected before device access:
--stage uboot --wipe-rootfs-data
Likewise:
--skip-stage rootfs-data --wipe-rootfs-data
remains an explicit conflict.
CLI help, README/CLAUDE documentation, and regression tests were updated to describe and enforce the same behavior.
| if vendor_chainloaded: | ||
| partial_persistent = stage_set & {"kernel", "rootfs", "rootfs-data", "env"} | ||
| if wipe_rootfs_data: | ||
| partial_persistent.add("rootfs-data") |
There was a problem hiding this comment.
This is the destructive-path guard for the new flag — it is what stops --wipe-rootfs-data erasing the overlay on a board still chainloaded from the factory bootloader without --stage uboot — but nothing exercises it. grep -rn wipe_rootfs_data tests/ matches only the three cases in test_install_rootfs_data.py (erase + verify, NAND rejection, skip-stage conflict), so a regression here would be silent.
Separately, in tests/test_ds_i203_final_contract.py the crc_failure == "tftp" parametrization used to return "CRC32 command timed out", which made parse_uboot_crc32 return None and exercised the "no checksum" branch. It now returns a well-formed but mismatched ==> 00000000, so that branch is only covered for the readback stage.
There was a problem hiding this comment.
Fixed in 1e11c83.
Regression coverage was added for the stock-U-Boot chainload destructive guard.
An explicit rootfs_data wipe while still chainloaded from genuine stock U-Boot is rejected unless the same exact-stage plan also includes the U-Boot stage, and the test verifies that no erase begins.
The DS-I203 final-contract coverage was also split into distinct RAM-verification failure modes:
checksum mismatch;
CRC prompt timeout;
incomplete/unparseable CRC response.
This prevents those branches from being accidentally collapsed into a single mismatch case again.
| ) | ||
| raise typer.Exit(1) | ||
|
|
||
| raise AssertionError("unreachable TFTP verification state") |
There was a problem hiding this comment.
Minor: this is unreachable for any non-negative TFTP_RAM_VERIFY_RETRIES — the loop always either returns or raises — so no test can cover it.
While here, expected_crc is zlib.crc32(orig_data) & 0xFFFFFFFF at both call sites (1046 and 1168) and orig_data is already a parameter, so it can be computed inside the helper and dropped from the signature.
There was a problem hiding this comment.
The unreachable post-loop assertion was removed.
_verify_tftp_ram() now computes the expected CRC internally from orig_data, so callers no longer calculate and pass the same value separately.
The verifier now has explicit reachable success, retry, and failure paths, with regression tests covering the relevant outcomes.
| allow_failure=True, | ||
| ) | ||
| unlock_text = unlock_resp.lower() | ||
| unlock_unsupported = ( |
There was a problem hiding this comment.
Minor, but these two matchers disagree while sitting ten lines apart. uboot_flash_command_error (used at line 757) already classifies unknown command and usage: sf, anchored to the start of a line; this block matches "usage:" and "unknown command" anywhere in the whole response buffer, which includes the command echo and any banner text carried over. A response can therefore be read as "unsupported, continue" by one branch and "failed, abort" by the other, decided by which sees it first. Reusing the shared matcher for the unsupported case would keep one vocabulary.
Same theme for _tftp_to_ram (lines 930-950): flashdump.tftp_to_ram (flashdump.py:336-352) already implements the tftpboot → unknown command → tftp fallback plus the done / bytes transferred check. This PR extends the copy, so the two now behave differently (one returns silently where the other raises) and have to be kept in sync by hand.
There was a problem hiding this comment.
Fixed in 1e11c83.
sf lock compatibility classification is now centralized in uboot_sf_lock_unsupported().
It distinguishes:
genuinely unavailable legacy sf lock syntax, which remains compatible;
a U-Boot that advertises sf lock but rejects the invocation, which remains a hard failure.
The general flash-command error matcher remains the hard-error vocabulary, so the paths no longer depend on unrelated substring matching.
The duplicated tftpboot -> tftp implementation was also removed.
Both install and flashdump now use the shared run_uboot_tftp() command sequencer, while the transport policy remains caller-owned:
flashdump/restore retain lenient behavior;
install supplies its strict status-preserving runner.
This shares the command sequencing without reintroducing the original regression of imposing installer strictness on backup/restore callers.
Confine strict U-Boot prompt synchronization to the install path so restore and dump-flash keep their historical lenient command contract. Preserve shell and download-command completion status inside install, make actual sf probe failures fatal again, and allow expected printenv misses to reach the existing rescue-MAC and verification logic. Keep the TFTP retry fallback scoped to one verification attempt, scale RAM CRC timeouts for large payloads, and preserve NAND compatibility when crc32 is not available. Re-probe and re-unlock NOR after the stock-migration environment reset before saveenv. Align --wipe-rootfs-data with exact stage selection, cover the vendor-chainload destructive guard, and add regressions for prompt timeouts, missing ethaddr, real U-Boot probe failures, temporary TFTP block-size fallback, and post-reset NOR unlock ordering. This commit is developed on an isolated child branch of PR OpenIPC#140; it does not modify the PR head until the fixes pass review and validation.
Follow up on review of the existing three-commit NOR install series without rewriting its history. Keep flashdump.send_command backward-compatible by making strict prompt completion opt-in to install. Restore and dump-flash therefore retain their lenient prompt semantics, while install preserves explicit shell/download completion status and fails closed where persistent writes depend on it. Reject real SPI probe failures such as "Failed to initialize SPI flash", centralize sf lock result classification, and re-probe/re-unlock after the in-session U-Boot reset before saveenv writes. Treat optional printenv misses as semantic absence rather than transport failure, preserving the generic rescue-MAC path and explicit environment verification in download-command mode. Share one U-Boot TFTP command sequencer between lenient and strict callers without sharing transport policy. Scope the 512-byte host fallback to the retry that needs it, restore normal negotiation after success, scale CRC timeout with payload size, and retry CRC timeout/incomplete output. NAND targets whose older U-Boot genuinely lacks crc32 retain compatibility with a single warning and TFTP completion/size checks; timeouts, malformed checksums, and mismatches remain fatal. Make --wipe-rootfs-data consistent with exact --stage plans and cover its stock-U-Boot chainload guard. Remove unreachable verification code and compute the expected RAM CRC inside the verifier. Add regression coverage for the review findings: strict-vs-legacy prompt behavior, promptless reset, dump CRC capability probing, real sf probe failure, shell/download result contracts, optional environment reads, shared TFTP fallback classification and scope, CRC retry modes, NAND without crc32, post-reset SPI reinitialization, and rootfs_data destructive-path guards.
openipc-ai
left a comment
openipc-ai
left a comment
There was a problem hiding this comment.
Re-reviewed at 1e11c83 against master (be1da26). All twelve findings from the previous review are addressed, each with regression coverage. Thanks for taking the follow-up as a fourth commit rather than a force-push — it made the delta easy to read.
Local CI parity is green on this head: 903 passed / 3 skipped (pytest tests/ --ignore=tests/fuzz), 16 fuzz passed at --hypothesis-seed=0, ruff clean, mypy clean on 78 source files.
The four blocking findings
defib restore traceback on the final reset. Fixed the right way round: flashdump.send_command gained require_prompt=False, so strict prompt completion is opt-in and only the installer's shell-mode _cmd_result passes require_prompt=True. cli/app.py is back to master byte-for-byte apart from the new option, so restore's write loop and dump-flash's probes keep their lenient semantics. The installer's own resets go through _reset_command, which uses wait_for=None in shell mode and allow_failure=True in download mode. Covered by test_legacy_wait_for_allows_reset_without_returned_prompt.
sf probe guard stopped matching U-Boot's real message. uboot_flash_command_error now matches a line-initial failed\b, so Failed to initialize SPI flash at 0:0 (error -2) is caught before select_nor_size_mb(..., require_detection=False) can guess a layout. test_nor_probe_failed_to_initialize_stops_before_unlock asserts the install stops with commands == ["sf probe 0"] and the transport closed.
printenv ethaddr made the generic rescue-MAC branch unreachable. _optional_printenv treats a download-mode failure whose output names the key and contains not defined as semantic absence rather than transport failure. I confirmed this has data to match on: DownloadCommandClient.send_command returns the text preceding [EOT](ERROR), not an empty string. test_download_missing_optional_ethaddr_still_generates_rescue_mac covers it.
_detect_crc32 turned a backup into a failure. It now requires positive evidence (==> <8 hex>, crc32 for, or a usage table naming crc32) and returns False on silence, so a laggy rack:// or rfc2217:// link degrades dump-flash to "dumping without verification" instead of aborting.
The other eight
The 512-byte TFTP cap is lifted on every success path — blocksize_caps == [512, MAX_BLOCKSIZE] is asserted, not just the cap. _crc_timeout_for_size scales at 2 s/MiB with a 10 s floor, and CRC timeouts or unparseable output are retried rather than fatal on first sight. sf probe and sf lock are re-issued after the in-session reset before saveenv, with the ordering asserted in test_ds_i203_final_contract. --wipe-rootfs-data with an exact --stage plan omitting rootfs-data now exits 2. The vendor-chainload partial_persistent guard has a test. The dead code in _verify_tftp_ram is gone.
Extracting src/defib/uboot_tftp.py is the part I'd single out: command spelling and the tftpboot→tftp fallback are now shared between flashdump.tftp_to_ram and the installer, while each keeps its own transport policy through the injected runner. That is the correct seam, and it removes the duplicate-matcher drift the previous review flagged.
Three residual observations — not change requests
1. sf lock argument arity (src/defib/install/layout.py:105). uboot_sf_lock_unsupported deliberately treats a usage table that does list sf lock as a hard failure, as test_sf_lock_usage_with_lock_entry_is_not_unsupported makes explicit. A U-Boot fork spelling it sf lock <offset> <len> would therefore print usage for sf lock 0 and abort an install that master would have completed. Exposure is narrow — mainline's sf protect lock/unlock does not match ^sf\s+lock, so it correctly warns and continues — and fail-closed is the defensible default here. Worth a sentence in the commit body noting the hardware evidence is one EV200, and sf protect unlock is the obvious future fallback.
2. NAND without crc32 (src/defib/install/orchestrator.py:1027). On the compatibility path, a TFTP response carrying done but no Bytes transferred = N is accepted with neither the size nor the CRC checked. Theoretical, since U-Boot always prints the byte count alongside done, but it is the one place where the degraded mode has no check left at all.
3. Unreachable code. The trailing return True, out after the shell-mode retry loop in _cmd_result cannot be reached: attempt == 1 already returns on the second pass.
One thing worth a comment rather than a change: shell mode can never return ok=False for a command U-Boot merely rejected, because a console has no exit-status channel. So allow_failure means "timed out" in shell mode and "non-zero result" in download mode. That asymmetry is inherent, not a defect, but a line on _cmd_result saying so would save the next reader the trace.
Approving. Nice work on the follow-up.
2 participants
This PR originally started as a small fix for SPI NOR write protection encountered during
defib install.On the tested Hi3516EV200 board, U-Boot detected the SPI NOR normally, but persistent writes could fail with:
Running
sf lock 0aftersf probecleared the protection and allowed the same erase/write operation to succeed, so the initial version of the PR added that unlock step before persistent NOR operations.While testing and reviewing that change further, two related installer issues became visible. Rather than opening separate PRs for changes that affect the same NOR installation path, I kept them together and extended the original PR with two follow-up commits.
The first follow-up addresses command synchronization and TFTP staging reliability.
The original unlock path depended too much on returned text: an incomplete shell response or a failed download command could be interpreted as success. Command execution now preserves completion/status information, shell commands must return to the expected U-Boot prompt, and probe/unlock failures stop the install before persistent writes.
The same testing also exposed an intermittent case where TFTP reported a completed transfer but the data already present in RAM had an incorrect CRC before anything was written to flash.
installnow verifies TFTP-staged data in RAM before erase/write. If verification fails, it retries the same file once by default. For host TFTP, the retry falls back to 512-byte blocks. The retry count is kept inTFTP_RAM_VERIFY_RETRIESrather than being hardcoded into the control flow.If the retry also fails, the install stops before modifying flash.
This verified staging path is used for the TFTP payloads handled by
install, including U-Boot, kernel, NOR rootfs and the extracted UBIFS payload used by NAND/UBI installs. The initial boot-ROM/SPL/U-Boot upload remains a separate transport path.The second follow-up came from checking clean-install behavior around
rootfs_data.Generic NOR installs previously preserved the existing persistent overlay, while stock-U-Boot migration paths already had their own cleanup behavior. This PR keeps those defaults unchanged, but adds an explicit:
option for cases where the operator wants a clean persistent overlay.
The explicit wipe goes through the same NOR unlock path, erases the
rootfs_dataregion and verifies the erased contents by CRC. It is rejected for NAND and for contradictory use together with--skip-stage rootfs-data.So the final PR keeps the original SPI NOR unlock fix as its base, while incorporating the additional reliability and cleanup changes that were discovered while testing that fix in the complete installation flow.
Verification
The final three-commit series was tested from a clean Linux/WSL checkout:
The NOR installation path was also tested on physical Hi3516EV200 / 8 MiB SPI NOR hardware.
Kernel-only and full NOR installs completed successfully with automatic SPI NOR unlock. U-Boot, kernel and rootfs writes passed CRC verification, and the existing factory MAC address was preserved.
The intermittent TFTP corruption condition was observed on hardware during development. The retry/fallback behavior is covered deterministically by regression tests: the first RAM CRC is forced to fail, the same file is requested again using the conservative block-size fallback, and no flash erase is allowed until RAM verification succeeds.