install: improve NOR install reliability and rootfs_data handling - #140
Merged
Merged
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
ArthurKoba
added a commit
to ArthurKoba/openipc-defib
that referenced
this pull request
Sep 21, 2026
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.
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
1 participant
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.
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.