bootscript: rk35xx: simplify armbianEnv.txt corruption detection

[mingzhangqun]

Summary

• Replace armbian_env_loaded flag approach with a simpler rootdev check to detect armbianEnv.txt corruption
• Use part exists mmc to probe the correct mmc device in the final fallback instead of hardcoding mmcblk0

Details

Power loss can fill armbianEnv.txt with 0xFF (eMMC erased block) which passes env import -t without error but imports zero variables. The new approach clears rootdev before import and verifies it was actually set afterward. If not, falls back to .bak, and finally uses part exists to detect which mmc device is present.

Test plan

• Boot RK3576 DevKit with valid armbianEnv.txt — verify normal boot
• Corrupt armbianEnv.txt (fill with 0xFF) — verify .bak fallback works
• Remove both armbianEnv.txt and .bak — verify part exists fallback boots correctly
• Test on both SD card (mmcblk1) and eMMC (mmcblk0) boot scenarios

Summary by CodeRabbit

• Bug Fixes
  • Improved boot resilience: detects and recovers from corrupted environment imports after power loss.
  • Automatically falls back to a backup environment when the primary import doesn't populate required boot settings.
  • Derives the fallback root device from detected boot hardware instead of using a fixed device path, increasing recovery reliability.

[coderabbitai]

Important

Review skipped

Auto reviews are limited based on label configuration.

🏷️ Required labels (at least one) (1)

• Needs review

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: d8946c70-a9f7-4a0b-8f78-74279f8d2e5d

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

The boot script clears rootdev before importing armbianEnv.txt, uses a non-empty post-import rootdev as success, falls back to importing armbianEnv.txt.bak if empty, and finally derives rootdev as /dev/mmcblk${devnum}p${distro_bootpart} if still unset.

Changes

Boot Environment Loading and Device Selection

Layer / File(s)	Summary
Environment loading with power-loss fallback config/bootscripts/boot-seeed-rk35xx.cmd	Clears rootdev before env import -t; a non-empty rootdev after import signals success. If empty, the script attempts armbianEnv.txt.bak. The previous armbian_env_loaded flag and explicit warnings were removed.
Root device selection fallback config/bootscripts/boot-seeed-rk35xx.cmd	If both imports leave rootdev empty, the script sets rootdev to /dev/mmcblk${devnum}p${distro_bootpart} derived from the boot devnum and distro_bootpart instead of using a fixed device path.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Suggested reviewers

• igorpecovnik
• rpardini
• paolosabatino

Poem

🐰 I nudged the env to wake from naps,
Cleared old roots and checked its maps.
If the file was hollow, I fetched the .bak stack,
If still unsure, I read devnum to pick the track.
Seeed boots humbly — a small rabbit hack.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title accurately describes the main change: simplifying corruption detection for armbianEnv.txt by replacing the flag-based approach with a rootdev-based check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@config/bootscripts/boot-seeed-rk35xx.cmd`:
- Around line 38-43: The current fallback that sets rootdev by probing MMC order
can pick mmcblk1p1 even when the system booted from mmc 0; instead, if rootdev
is empty derive it from the actual boot source: first prefer a configured
PARTUUID (use ${PARTUUID} if present), otherwise construct rootdev from the boot
source variables ${devtype} and ${devnum} (e.g., the mmc device the script was
loaded from) rather than the probe order; update the block that currently checks
"part exists mmc 1 1" / "part exists mmc 0 1" to check/choose based on PARTUUID
or build "/dev/<devtype><devnum>p1" when devtype/devnum indicate an MMC and only
fall back to probing if those variables are unset.
- Around line 24-37: Don't use rootdev itself as the corruption sentinel; save
the current value into a temp variable (e.g. saved_rootdev) before you clear
rootdev, perform the load/import as today, then if rootdev is still empty
restore saved_rootdev and only then attempt the .bak fallback; also unset the
temp saved_rootdev afterwards so you don't leak state. This keeps the corruption
check independent of an optional config key while preserving any
already-initialized rootdev.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: acc8a70a-a4f5-4972-9bb9-7dfc4d2a9ec4

📥 Commits

Reviewing files that changed from the base of the PR and between 66e68ea and 3410aac.

📒 Files selected for processing (1)

• config/bootscripts/boot-seeed-rk35xx.cmd

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@config/bootscripts/boot-seeed-rk35xx.cmd`:
- Around line 33-43: The script restores rootdev from saved_rootdev before
attempting to import armbianEnv.txt.bak, which prevents the final fallback
(/dev/mmcblk${devnum}p1) if the .bak import fails or imports nothing; relocate
or conditionalize the setenv rootdev "${saved_rootdev}" so it only runs after
the load/import attempt (or only if rootdev is still empty after a successful
import), i.e., attempt load ${devtype} ${devnum}:${distro_bootpart} ${load_addr}
${prefix}armbianEnv.txt.bak and env import -t ${load_addr} ${filesize} first,
then restore saved_rootdev (or set saved_rootdev cleared) only if rootdev
remains unset—affecting the symbols rootdev, saved_rootdev, armbianEnv.txt.bak,
load ${devtype}..., env import -t, and setenv rootdev "/dev/mmcblk${devnum}p1".

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 1cd6d613-8042-4cf1-b670-6da130f74f71

📥 Commits

Reviewing files that changed from the base of the PR and between 3410aac and e7a3a02.

📒 Files selected for processing (1)

• config/bootscripts/boot-seeed-rk35xx.cmd

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@config/bootscripts/boot-seeed-rk35xx.cmd`:
- Around line 39-41: The fallback sets rootdev with a hardcoded partition "p1";
update the setenv call so it derives the partition from ${distro_bootpart}
instead of "1" (i.e., build rootdev using ${devnum} and ${distro_bootpart});
modify the setenv rootdev line (the line that currently reads setenv rootdev
"/dev/mmcblk${devnum}p1") to construct "/dev/mmcblk${devnum}p${distro_bootpart}"
so the script honors non-1 boot partitions.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 2c083362-32ed-429c-8aea-60841f01aa0c

📥 Commits

Reviewing files that changed from the base of the PR and between e7a3a02 and 6a14fe2.

📒 Files selected for processing (1)

• config/bootscripts/boot-seeed-rk35xx.cmd

[djurny]

I'm wondering if this check on file content can be extended to the other files that are to be loaded. Now it's mainly focussed on armbianEnv.txt, but this might as well be a DT overlay file just created before a cold reboot or fixup script. Perhaps something that checks if the first few bytes on ${load_addr} are either 0xFF or 0x00 ?

[djurny]

I will search a bit more, but out of my loose wrist i cannot recall where an armbianEnv.txt.bak file is being created? All the other changes make an armbianEnv.txt.tmp file that will be renamed to armbianEnv.txt. Is the fallback for a corrupted armbianEnv.txt falling back on the right file here?

[mingzhangqun]

Thanks @djurny for the thorough review and suggestions!

I'm wondering if this check on file content can be extended to the other files that are to be loaded.

That's a good idea for a future improvement. A generic mw.b ${load_addr} 0x00 1 before load + checksum comparison could catch 0xFF/0x00 files regardless of format. For now I've kept the scope to armbianEnv.txt since that's the most critical file (wrong rootdev = unbootable), and the .bak + rootdev-derivation fallback chain already handles it well.

I cannot recall where an armbianEnv.txt.bak file is being created?

The .bak file is created at image build time by a Seeed board config hook (recomputer-rk35xx-common.inc:514-524). It copies the initial armbianEnv.txt as a known-good fallback before the device ever reaches the user. This is separate from the upstream runtime pattern of .tmp → rename, which protects against write interruption but doesn't help with post-write corruption from power loss.

[djurny]

I cannot recall where an armbianEnv.txt.bak file is being created?

The .bak file is created at image build time by a Seeed board config hook (recomputer-rk35xx-common.inc:514-524). It copies the initial armbianEnv.txt as a known-good fallback before the device ever reaches the user. This is separate from the upstream runtime pattern of .tmp → rename, which protects against write interruption but doesn't help with post-write corruption from power loss.

Hi @mingzhangqun,
Propose to have the armbianEnv.txt.bak renamed to armbianEnv.txt.dist. It is too common to make a backup of a file before editing it, and when peoples edit armbianEnv.txt, there is a big chance the backup file will be armbianEnv.txt.bak. Some people (like myself) also like to clean up every now and then, which might involve removing backups of files that look obsolete.

Naming the "fallback" configuration .dist would make more sense, as the file is generated during image build. Another option would be to put the fallback file into /usr/share/armbian/ as for the standard installations, /usr lives on the same partition as /boot. There is already an armbianEnv.txt put in /usr/share/armbian so please use .dist to distinguish them.

Groetjes,

[mingzhangqun]

Hi @djurny,

Thanks for the thorough review and the suggestion!

I've renamed the fallback to armbianEnv.txt.dist as proposed — it now reads as a build-shipped default rather than a user backup, so it won't collide with the .bak files users tend to create (or clean up) themselves. Done in d78a18238, both the build hook and the bootscript consumer are updated.

Regarding placing the file under /usr/share/armbian/: I'd prefer to keep it on /boot. The fallback is consumed by the U-Boot bootscript (load ${devtype} ${devnum}:${distro_bootpart} ...), which only sees the boot partition at that stage. On these boards /boot is a separate partition from the rootfs, and on encrypted-root builds (LUKS) the rootfs is unreadable from U-Boot entirely — so /usr/share/armbian/ wouldn't be reachable precisely when the fallback is needed. Keeping it on /boot with the .dist suffix works uniformly across the standard, encrypted, and A/B partition configurations.

The .dist naming should still address your concern about the file being mistaken for a user backup.

Groetjes,
mingzhangqun

[github-actions]

✅ This PR has been reviewed and approved — all set for merge!