helios64: u-boot: bump to v2026.04 + diagnostics + CMD_FILEENV; v2026.04: generic btrfs zstd fix#9675
Open
iav wants to merge 3 commits intoarmbian:mainfrom
Open
helios64: u-boot: bump to v2026.04 + diagnostics + CMD_FILEENV; v2026.04: generic btrfs zstd fix#9675iav wants to merge 3 commits intoarmbian:mainfrom
iav wants to merge 3 commits intoarmbian:mainfrom
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (18)
✅ Files skipped from review due to trivial changes (7)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughAdds Kobol Helios64 support to U-Boot v2026.04: updates build inputs, supplies board patch overlays, adds Kconfig/Makefile/defconfig/include, implements board code and OTP SPI reader, introduces Changes
Sequence DiagramsequenceDiagram
participant SPL
participant UBoot as U-Boot
participant SPI
participant OTP
participant GPIO
participant Env
SPL->>GPIO: board_early_init_f() (status LED)
UBoot->>OTP: ensure_otp_data_ready()
OTP->>SPI: spi_write_then_read(cmd)
SPI-->>OTP: OTP bytes
OTP->>OTP: validate magic / CRC32
OTP-->>UBoot: cached OTP data
UBoot->>Env: set_board_info() (board_rev, serial#)
UBoot->>OTP: mac_read_from_otp()
OTP-->>Env: env_set ethaddr / eth%daddr
UBoot->>GPIO: setup_iodomain() / init_vdd_center()
UBoot->>GPIO: setup_boottargets() / setup_leds() / last_stage_init()
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~65 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
github-actions
Bot
added
size/large
iav
force-pushed
the
feat/helios64-uboot-v2026.04
branch
from
788c65c to
feafefa
Compare
iav
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch (1)
131-131: Drop the temporary#warningbefore merge.This will make every Helios64 U-Boot build noisy and can hide real compiler warnings once the MAC debugging work is done.
Suggested cleanup
-#warning "Building image-fdt.c: fdt fixup call chain instrumented for MAC debugging."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch` at line 131, Remove the temporary preprocessor warning line that was added for MAC debugging (the line containing `#warning` "Building image-fdt.c: fdt fixup call chain instrumented for MAC debugging.") from the patch/board code (image-fdt.c / fdt fixup code) before merging so regular builds are not noisy and real compiler warnings aren't obscured; locate the literal `#warning` string in the diff and delete that line (or revert the change) and ensure no other debug-only preprocessor warnings remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@patch/u-boot/v2026.04/board_helios64/board/Kconfig`:
- Around line 21-22: The Kconfig default for ENV_OFFSET (symbol ENV_OFFSET) is
set to 0x460000 which conflicts with the board defconfig's CONFIG_ENV_OFFSET
value (0x3F8000); update the Kconfig default for ENV_OFFSET (the "default ... if
ENV_IS_IN_SPI_FLASH" line) to 0x3F8000 so SPI-flash fallback paths place the
environment at the same address as the helios64 defconfig (ensure you modify the
ENV_OFFSET default and keep the ENV_IS_IN_SPI_FLASH condition intact).
In `@patch/u-boot/v2026.04/board_helios64/board/sys_otp.c`:
- Around line 45-54: is_valid_header currently returns true if any single byte
of otp.magic matches, making the check useless; change it to require the full
magic sequence by checking all bytes match the expected sequence (e.g.,
'H','6','4','N','P','V','1','\0') using logical AND for each comparison or a
single memcmp against the expected string; update the is_valid_header function
to only return 1 when the entire otp.magic equals the expected magic, otherwise
return 0.
- Around line 103-104: The printf currently performs an unaligned 8-byte read by
casting otp.serial_num to uint64_t* (seen where Serial Number is printed and the
second occurrence later), which is undefined and will fault; fix by decoding the
6 bytes explicitly into a 64-bit value (e.g., combine otp.serial_num[0..5] into
a uint64_t with shifts/masks or use an unaligned-read helper) and then
mask/print that 48-bit value instead of dereferencing
*(uint64_t*)otp.serial_num; apply the same change at both places referencing
otp.serial_num to avoid unaligned accesses.
In
`@patch/u-boot/v2026.04/board_helios64/general-fix-btrfs-zstd-decompression.patch`:
- Around line 103-116: zstd_decompress_dctx can return a non-error size smaller
than the expected dlen, so after calling zstd_decompress_dctx (and freeing
workspace) check the returned ret and treat any short decode as an error: if
zstd_is_error(ret) || ret != dlen (or ret < dlen) then free tmp and return -1;
only on exact match ret == dlen proceed to memcpy(tmp -> dbuf), free tmp and
return dlen. Reference zstd_decompress_dctx, ret, dlen, dbuf, tmp, workspace,
out_buf/out_len and cbuf/clen when making the change.
---
Nitpick comments:
In `@patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch`:
- Line 131: Remove the temporary preprocessor warning line that was added for
MAC debugging (the line containing `#warning` "Building image-fdt.c: fdt fixup
call chain instrumented for MAC debugging.") from the patch/board code
(image-fdt.c / fdt fixup code) before merging so regular builds are not noisy
and real compiler warnings aren't obscured; locate the literal `#warning` string
in the diff and delete that line (or revert the change) and ensure no other
debug-only preprocessor warnings remain.
🪄 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: 61483722-d597-4673-908b-e04b1ef3ca58
📒 Files selected for processing (17)
config/boards/helios64.confpatch/u-boot/v2026.04/board_helios64/0000.patching_config.yamlpatch/u-boot/v2026.04/board_helios64/1000-exception-include-string.patchpatch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patchpatch/u-boot/v2026.04/board_helios64/1001-helios64-enable-target.patchpatch/u-boot/v2026.04/board_helios64/board/Kconfigpatch/u-boot/v2026.04/board_helios64/board/MAINTAINERSpatch/u-boot/v2026.04/board_helios64/board/Makefilepatch/u-boot/v2026.04/board_helios64/board/helios64.cpatch/u-boot/v2026.04/board_helios64/board/sys_otp.cpatch/u-boot/v2026.04/board_helios64/board/sys_otp.hpatch/u-boot/v2026.04/board_helios64/cmd-fileenv-read-string-from-file-into-env.patchpatch/u-boot/v2026.04/board_helios64/defconfig/helios64-rk3399_defconfigpatch/u-boot/v2026.04/board_helios64/dt_uboot/rk3399-kobol-helios64-u-boot.dtsipatch/u-boot/v2026.04/board_helios64/dt_upstream_rockchip/rk3399-kobol-helios64.dtspatch/u-boot/v2026.04/board_helios64/general-fix-btrfs-zstd-decompression.patchpatch/u-boot/v2026.04/board_helios64/include_configs/helios64.h
iav
added a commit
to iav/armbian
that referenced
this pull request
Apr 15, 2026
Per CodeRabbit review on PR armbian#9675: board/Kconfig ENV_OFFSET default 0x460000 conflicts with defconfig CONFIG_ENV_OFFSET=0x3F8000. Authoritative source - upstream U-Boot itself: 1. configs/helios64-rk3399_defconfig:7 sets CONFIG_ENV_OFFSET=0x3F8000 - this is what actually ends up in the build; the defconfig overrides the Kconfig default. 2. All 20+ rk3399 boards in upstream configs/*rk3399*_defconfig use 0x3F8000 (evb, firefly, nanopi-m4/r4s, pinebook-pro, rock-4c, khadas-edge, leez, orangepi, ...). 3. The 0x460000 in board/Kconfig is dead code: it is always shadowed by the defconfig, never applied in builds. It is a legacy artefact of the Kobol vendor patch series, not matching any standard SPI layout for rk3399. 4. Physically: on Helios64's 16 MiB W25Q128JV, 0x3F8000 = 4 MiB - 32 KiB reserves the last 32 KiB slot inside the lower 4 MiB window for env - the common rk3399 convention. Aligning the Kconfig default with the defconfig removes the inconsistency and makes the fallback path land at the same address the defconfig expects.
iav
force-pushed
the
feat/helios64-uboot-v2026.04
branch
from
feafefa to
8a8fe50
Compare
iav
added a commit
to iav/armbian
that referenced
this pull request
Apr 15, 2026
Per CodeRabbit review on PR armbian#9675: board/Kconfig ENV_OFFSET default 0x460000 conflicts with defconfig CONFIG_ENV_OFFSET=0x3F8000. Authoritative source - upstream U-Boot itself: 1. configs/helios64-rk3399_defconfig:7 sets CONFIG_ENV_OFFSET=0x3F8000 - this is what actually ends up in the build; the defconfig overrides the Kconfig default. 2. All 20+ rk3399 boards in upstream configs/*rk3399*_defconfig use 0x3F8000 (evb, firefly, nanopi-m4/r4s, pinebook-pro, rock-4c, khadas-edge, leez, orangepi, ...). 3. The 0x460000 in board/Kconfig is dead code: it is always shadowed by the defconfig, never applied in builds. It is a legacy artefact of the Kobol vendor patch series, not matching any standard SPI layout for rk3399. 4. Physically: on Helios64's 16 MiB W25Q128JV, 0x3F8000 = 4 MiB - 32 KiB reserves the last 32 KiB slot inside the lower 4 MiB window for env - the common rk3399 convention. Aligning the Kconfig default with the defconfig removes the inconsistency and makes the fallback path land at the same address the defconfig expects.
iav
added a commit
to iav/armbian
that referenced
this pull request
Apr 15, 2026
Per CodeRabbit review on PR armbian#9675: zstd_decompress_dctx() returns a size_t — the actual number of bytes written, not just an error indicator. The previous code unconditionally returned dlen even if ret < dlen, leaving the tail of dbuf as uninitialised garbage. In practice this path is unreachable for well-formed BTRFS extents: after our fix out_len = max(dlen, fcs), and on success zstd produces exactly fcs bytes, so ret == fcs >= dlen. But the defensive check is trivial and guards against: - a frame header with a falsified content-size that still passes the integrity check; - truncated/corrupted frames that zstd does not always flag as an error. Apply the same fix to the duplicate patch under board_helios64/. This issue was not flagged in PR armbian#9651 (v2026.01) — different review pass surfaced different findings.
iav
force-pushed
the
feat/helios64-uboot-v2026.04
branch
from
8a8fe50 to
f37c9a6
Compare
iav
added a commit
to iav/armbian
that referenced
this pull request
Apr 15, 2026
Per CodeRabbit review on PR armbian#9675: zstd_decompress_dctx() returns a size_t — the actual number of bytes written, not just an error indicator. The previous code unconditionally returned dlen even if ret < dlen, leaving the tail of dbuf as uninitialised garbage. In practice this path is unreachable for well-formed BTRFS extents: after our fix out_len = max(dlen, fcs), and on success zstd produces exactly fcs bytes, so ret == fcs >= dlen. But the defensive check is trivial and guards against: - a frame header with a falsified content-size that still passes the integrity check; - truncated/corrupted frames that zstd does not always flag as an error. Apply the same fix to the duplicate patch under board_helios64/. This issue was not flagged in PR armbian#9651 (v2026.01) — different review pass surfaced different findings.
iav
added a commit
to iav/armbian
that referenced
this pull request
Apr 15, 2026
Per CodeRabbit review on PR armbian#9675: is_valid_header() in board/sys_otp.c uses '||' between byte comparisons, returning 1 if *any* one byte of otp.magic matches the expected sequence. Practically every random byte pattern will trip at least one comparison, so the magic check is effectively meaningless — partially corrupt OTP data is treated as a valid Helios64 header. This is a pre-existing bug in the Kobol vendor code, carried forward unchanged through every U-Boot bump. We are migrating these board files to v2026.04, so the right place to fix it. Replace the chain with a single memcmp() against the expected magic: - removes the '||' vs '&&' confusion entirely; - reads as a normal magic-bytes check; - the constant lives in one place.
iav
force-pushed
the
feat/helios64-uboot-v2026.04
branch
from
f37c9a6 to
6426700
Compare
iav
added a commit
to iav/armbian
that referenced
this pull request
Apr 15, 2026
Per CodeRabbit review on PR armbian#9675: is_valid_header() in board/sys_otp.c uses '||' between byte comparisons, returning 1 if *any* one byte of otp.magic matches the expected sequence. Practically every random byte pattern will trip at least one comparison, so the magic check is effectively meaningless — partially corrupt OTP data is treated as a valid Helios64 header. This is a pre-existing bug in the Kobol vendor code, carried forward unchanged through every U-Boot bump. We are migrating these board files to v2026.04, so the right place to fix it. Replace the chain with a single memcmp() against the expected magic: - removes the '||' vs '&&' confusion entirely; - reads as a normal magic-bytes check; - the constant lives in one place.
iav
added a commit
to iav/armbian
that referenced
this pull request
Apr 15, 2026
…_num Per CodeRabbit review on PR armbian#9675: *((uint64_t *) otp.serial_num) & 0xFFFFFFFFFFFF reads 8 bytes from a 6-byte field inside a __packed__ struct (over-reads into mfg_year), and is unaligned. UB per the C standard, and would fault on stricter alignment requirements. Same pattern affects two call sites the bot pointed at (lines 103-104 and 217-218 of the original vendor file). The bot did not flag it but *(u16 *) otp.mfg_year on the next line has the same problem - packed field, unaligned access - so fix it together. Replace the casts with two small inline helpers that decode the bytes explicitly little-endian (RK3399 is LE, matches the vendor's original intent): otp_serial() - assemble 6 bytes into a u64 otp_mfg_year() - assemble 2 bytes into a u16 This is pre-existing Kobol vendor code carried forward through every U-Boot bump; this PR migrates the board files to v2026.04, so it is the right place to fix it.
iav
force-pushed
the
feat/helios64-uboot-v2026.04
branch
from
6426700 to
023a5c6
Compare
8 tasks
iav
requested review from
ColorfulRhino,
HeyMeco,
NicoD-SBC,
SuperKali,
Tonymac32,
amazingfate,
catalinii and
efectn
as code owners
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
patch/u-boot/v2026.04/board_helios64/board/sys_otp.c (1)
219-222:⚠️ Potential issue | 🟡 MinorMake
get_revision()use the same OTP-ready path as the other accessors.Line 221 still bypasses
ensure_otp_data_ready(), so a transient early SPI failure can be retried byget_variant(),set_board_info(), andmac_read_from_otp(), but not here.🔧 Minimal fix
int get_revision(int *major, int *minor) { - if (!is_data_valid()) + if (ensure_otp_data_ready()) return -1; *major = (otp.revision & 0xf0) >> 4;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/u-boot/v2026.04/board_helios64/board/sys_otp.c` around lines 219 - 222, get_revision() currently bypasses the OTP-ready path by directly calling is_data_valid(); change it to mirror other accessors by calling ensure_otp_data_ready() first (and handling its error), then call is_data_valid() and return -1 if still invalid. Update get_revision to invoke ensure_otp_data_ready() before using is_data_valid() so transient SPI/OTP init failures are retried (refer to function names get_revision, ensure_otp_data_ready, is_data_valid, and callers like get_variant/set_board_info/mac_read_from_otp to match behavior).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@patch/u-boot/v2026.04/board_helios64/board/sys_otp.c`:
- Around line 219-222: get_revision() currently bypasses the OTP-ready path by
directly calling is_data_valid(); change it to mirror other accessors by calling
ensure_otp_data_ready() first (and handling its error), then call
is_data_valid() and return -1 if still invalid. Update get_revision to invoke
ensure_otp_data_ready() before using is_data_valid() so transient SPI/OTP init
failures are retried (refer to function names get_revision,
ensure_otp_data_ready, is_data_valid, and callers like
get_variant/set_board_info/mac_read_from_otp to match behavior).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 67eeae8d-850a-4eb7-a875-39c1a5db5247
📒 Files selected for processing (2)
patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patchpatch/u-boot/v2026.04/board_helios64/board/sys_otp.c
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch (3)
133-133:⚠️ Potential issue | 🟡 MinorDrop the compile-time
#warning.The runtime
log_info()tracing already makes this patch visible. Keeping#warningin a carried patch will spam every build and can become fatal under-Werrortoolchains.Minimal fix
-#warning "Building image-fdt.c: fdt fixup call chain instrumented for MAC debugging."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch` at line 133, Remove the compile-time directive that emits a build warning: delete the added line '#warning "Building image-fdt.c: fdt fixup call chain instrumented for MAC debugging."' from the patch so it no longer spams builds; rely on the existing runtime tracing via log_info() in image-fdt.c (the fdt fixup call chain instrumentation) for visibility instead.
100-104:⚠️ Potential issue | 🟠 MajorKeep the new
statusprobe diagnostic-only.This turns a logging patch into a behavior change. The original flow only skipped explicitly
"disabled"nodes; this now skips any present status other than exact"okay", which can suppress valid aliases that still mean enabled.Minimal fix
- const struct fdt_property *status_prop = fdt_get_property(fdt, nodeoff, "status", NULL); - if (status_prop && strcmp((const char *)status_prop->data, "okay")) { - log_info("[fdt_fixup_ethernet] Node '%s' status is '%s', skipping\n", path, (const char *)status_prop->data); - total_skipped++; - continue; - } + const struct fdt_property *status_prop = fdt_get_property(fdt, nodeoff, "status", NULL); + if (status_prop) + log_info("[fdt_fixup_ethernet] Node '%s' status is '%s'\n", + path, (const char *)status_prop->data);Based on learnings: For the Armbian build project, maintaining consistency with existing patches across U-Boot versions (such as between 2025.01 and 2025.04) is prioritized over refactoring individual patches for code improvements.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch` around lines 100 - 104, The new check using fdt_get_property/status_prop and strcmp changes behavior by skipping any status not equal to "okay" instead of only skipping "disabled"; revert it to be diagnostic-only: keep retrieving status_prop via fdt_get_property and log its value with log_info (referencing status_prop, fdt_get_property, log_info, path, nodeoff), but do not alter control flow—remove or avoid using strcmp to continue/total_skipped so the code only logs the status and preserves the original skipping logic (which should only skip when status equals "disabled").
33-46:⚠️ Potential issue | 🟠 MajorRecompute
/aliasesinside the loop.
do_fixup_by_path()mutates the FDT during iteration, so cachingaliases_offonce can leave laterfdt_first_property_offset()calls walking a stale node offset. It also contradicts the comment on Line 43.Minimal fix
- int aliases_off = fdt_path_offset(fdt, "/aliases"); - if (aliases_off < 0) { + if (fdt_path_offset(fdt, "/aliases") < 0) { log_info("[fdt_fixup_ethernet] /aliases node not found\n"); return; } @@ - offset = fdt_first_property_offset(fdt, aliases_off); + offset = fdt_first_property_offset(fdt, + fdt_path_offset(fdt, "/aliases"));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch` around lines 33 - 46, The cached aliases_off can become stale because do_fixup_by_path() may mutate the FDT while iterating; change the loop so you recompute the /aliases offset each iteration instead of reusing aliases_off (i.e., call fdt_path_offset(fdt, "/aliases") when computing offset inside the for loop), keep the early-not-found check but avoid returning a stale offset, and ensure this logic around aliases_off and offset (used with fdt_first_property_offset) is updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch`:
- Line 133: Remove the compile-time directive that emits a build warning: delete
the added line '#warning "Building image-fdt.c: fdt fixup call chain
instrumented for MAC debugging."' from the patch so it no longer spams builds;
rely on the existing runtime tracing via log_info() in image-fdt.c (the fdt
fixup call chain instrumentation) for visibility instead.
- Around line 100-104: The new check using fdt_get_property/status_prop and
strcmp changes behavior by skipping any status not equal to "okay" instead of
only skipping "disabled"; revert it to be diagnostic-only: keep retrieving
status_prop via fdt_get_property and log its value with log_info (referencing
status_prop, fdt_get_property, log_info, path, nodeoff), but do not alter
control flow—remove or avoid using strcmp to continue/total_skipped so the code
only logs the status and preserves the original skipping logic (which should
only skip when status equals "disabled").
- Around line 33-46: The cached aliases_off can become stale because
do_fixup_by_path() may mutate the FDT while iterating; change the loop so you
recompute the /aliases offset each iteration instead of reusing aliases_off
(i.e., call fdt_path_offset(fdt, "/aliases") when computing offset inside the
for loop), keep the early-not-found check but avoid returning a stale offset,
and ensure this logic around aliases_off and offset (used with
fdt_first_property_offset) is updated accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 41f68186-a5b4-440f-8cd8-0b3870cf5517
📒 Files selected for processing (2)
patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patchpatch/u-boot/v2026.04/board_helios64/board/sys_otp.c
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch (3)
100-104:⚠️ Potential issue | 🟠 MajorKeep
statushandling diagnostic-only; skip only explicit"disabled".The current condition changes behavior by rejecting any status not exactly
"okay".Suggested minimal fix
- const struct fdt_property *status_prop = fdt_get_property(fdt, nodeoff, "status", NULL); - if (status_prop && strcmp((const char *)status_prop->data, "okay")) { - log_info("[fdt_fixup_ethernet] Node '%s' status is '%s', skipping\n", path, (const char *)status_prop->data); - total_skipped++; - continue; - } + const struct fdt_property *status_prop = fdt_get_property(fdt, nodeoff, "status", NULL); + if (status_prop) + log_info("[fdt_fixup_ethernet] Node '%s' status is '%s'\n", + path, (const char *)status_prop->data); + if (status_prop && !strcmp((const char *)status_prop->data, "disabled")) { + total_skipped++; + continue; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch` around lines 100 - 104, The code in fdt_fixup_ethernet uses status_prop and rejects any status not exactly "okay", but the review asks to keep status diagnostic-only and only skip when status is explicitly "disabled"; change the condition around fdt_get_property/status_prop so that you log the status (using log_info with path and status_prop->data) but only increment total_skipped and continue when strcmp(status, "disabled") == 0; leave nodes with absent status or other values to be processed normally. Ensure you reference the same symbols: status_prop, fdt_get_property(fdt, nodeoff, "status", NULL), log_info(..., path, status), total_skipped, and the surrounding fdt_fixup_ethernet logic.
33-47:⚠️ Potential issue | 🟠 MajorRecompute
/aliasesoffset inside the loop (cached offset can go stale).This still contradicts the “recompute” intent and can miss later aliases after DT edits in-loop.
Suggested minimal fix
- int aliases_off = fdt_path_offset(fdt, "/aliases"); - if (aliases_off < 0) { + if (fdt_path_offset(fdt, "/aliases") < 0) { log_info("[fdt_fixup_ethernet] /aliases node not found\n"); return; } @@ - /* FDT might have been edited, recompute the offset */ - offset = fdt_first_property_offset(fdt, aliases_off); + /* FDT might have been edited, recompute the offset */ + offset = fdt_first_property_offset(fdt, + fdt_path_offset(fdt, "/aliases"));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch` around lines 33 - 47, The loop currently uses a cached aliases_off and so can miss aliases if the device tree is modified during iteration; instead, recompute the /aliases node offset each iteration by calling fdt_path_offset(fdt, "/aliases") right before fdt_first_property_offset, and handle a negative result (e.g., break or continue) appropriately; update references to aliases_off and the call to fdt_first_property_offset(fdt, aliases_off) inside the for-loop in the fdt_fixup_ethernet code so the offset is fresh on each pass.
133-133:⚠️ Potential issue | 🟡 MinorDrop the hard-coded compile-time
#warning.This is still noisy in carried patches and can fail warning-as-error toolchains.
Suggested minimal fix
-#warning "Building image-fdt.c: fdt fixup call chain instrumented for MAC debugging."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch` at line 133, Remove the hard-coded compile-time '#warning "Building image-fdt.c: fdt fixup call chain instrumented for MAC debugging."' (or wrap it behind a dedicated debug macro) so it doesn't emit noisy warnings or break warning-as-error builds; locate the added '#warning' line in the image-fdt.c patch and either delete it or enclose it in a conditional such as '#ifdef ENABLE_FDT_MAC_DEBUG' / '#endif' so the warning is only produced when explicitly enabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch`:
- Around line 100-104: The code in fdt_fixup_ethernet uses status_prop and
rejects any status not exactly "okay", but the review asks to keep status
diagnostic-only and only skip when status is explicitly "disabled"; change the
condition around fdt_get_property/status_prop so that you log the status (using
log_info with path and status_prop->data) but only increment total_skipped and
continue when strcmp(status, "disabled") == 0; leave nodes with absent status or
other values to be processed normally. Ensure you reference the same symbols:
status_prop, fdt_get_property(fdt, nodeoff, "status", NULL), log_info(..., path,
status), total_skipped, and the surrounding fdt_fixup_ethernet logic.
- Around line 33-47: The loop currently uses a cached aliases_off and so can
miss aliases if the device tree is modified during iteration; instead, recompute
the /aliases node offset each iteration by calling fdt_path_offset(fdt,
"/aliases") right before fdt_first_property_offset, and handle a negative result
(e.g., break or continue) appropriately; update references to aliases_off and
the call to fdt_first_property_offset(fdt, aliases_off) inside the for-loop in
the fdt_fixup_ethernet code so the offset is fresh on each pass.
- Line 133: Remove the hard-coded compile-time '#warning "Building image-fdt.c:
fdt fixup call chain instrumented for MAC debugging."' (or wrap it behind a
dedicated debug macro) so it doesn't emit noisy warnings or break
warning-as-error builds; locate the added '#warning' line in the image-fdt.c
patch and either delete it or enclose it in a conditional such as '#ifdef
ENABLE_FDT_MAC_DEBUG' / '#endif' so the warning is only produced when explicitly
enabled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 01d590bc-fc12-47be-8977-a700663f9804
📒 Files selected for processing (2)
patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patchpatch/u-boot/v2026.04/board_helios64/board/sys_otp.c
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch (3)
133-133:⚠️ Potential issue | 🟡 MinorDrop the hard-coded
#warning.The runtime
log_info()tracing already gives you the diagnostics. The compile-time warning fires on every build and can become fatal with-Werror.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch` at line 133, Remove the hard-coded preprocessor warning that was added: delete the line containing `#warning` "Building image-fdt.c: fdt fixup call chain instrumented for MAC debugging." (the compile-time diagnostic is redundant because runtime log_info() provides the tracing and the `#warning` can cause build failures with -Werror); rely on existing log_info() instrumentation instead of emitting a compile-time `#warning`.
100-104:⚠️ Potential issue | 🟠 MajorKeep the
statusprobe diagnostic-only.This now skips every node whose
statusis not literally"okay". The original flow only skipped explicit"disabled"nodes, so this turns a logging patch into a behavior change.Suggested minimal fix
const struct fdt_property *status_prop = fdt_get_property(fdt, nodeoff, "status", NULL); - if (status_prop && strcmp((const char *)status_prop->data, "okay")) { - log_info("[fdt_fixup_ethernet] Node '%s' status is '%s', skipping\n", path, (const char *)status_prop->data); + if (status_prop) + log_info("[fdt_fixup_ethernet] Node '%s' status is '%s'\n", + path, (const char *)status_prop->data); + if (status_prop && !strcmp((const char *)status_prop->data, "disabled")) { + log_info("[fdt_fixup_ethernet] Node '%s' is disabled, skipping\n", path); total_skipped++; continue; }Based on learnings: For the Armbian build project, maintaining consistency with existing patches across U-Boot versions (such as between 2025.01 and 2025.04) is prioritized over refactoring individual patches for code improvements.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch` around lines 100 - 104, The current code treats any non-"okay" status as a reason to skip nodes; change the probe to be diagnostic-only and only skip when status is explicitly "disabled": fetch the "status" with fdt_get_property into status_prop and if status_prop && strcmp((const char *)status_prop->data, "disabled") == 0 then log and increment total_skipped and continue; otherwise, if status_prop exists and is not "disabled", just log the status with log_info but do not continue. Reference symbols: fdt_get_property, status_prop, strcmp, log_info, total_skipped, continue.
32-46:⚠️ Potential issue | 🟠 MajorRecompute
/aliasesinside the loop.This still leaves
aliases_offstale afterdo_fixup_by_path()mutates the blob, which can make later alias iteration unreliable.Suggested minimal fix
- int aliases_off = fdt_path_offset(fdt, "/aliases"); - if (aliases_off < 0) { + if (fdt_path_offset(fdt, "/aliases") < 0) { log_info("[fdt_fixup_ethernet] /aliases node not found\n"); return; } @@ - offset = fdt_first_property_offset(fdt, aliases_off); + offset = fdt_first_property_offset(fdt, + fdt_path_offset(fdt, "/aliases"));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch` around lines 32 - 46, The current code caches aliases_off once before iterating aliases, but do_fixup_by_path() can modify the device tree and make aliases_off stale; inside the for loop before calling fdt_first_property_offset and any use of aliases_off, recompute aliases_off by calling fdt_path_offset(fdt, "/aliases") and handle a negative return (log and break/return) so each iteration uses a fresh /aliases offset; update references in the loop that use aliases_off (and any subsequent calls like fdt_first_property_offset) to use the recomputed value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch`:
- Line 133: Remove the hard-coded preprocessor warning that was added: delete
the line containing `#warning` "Building image-fdt.c: fdt fixup call chain
instrumented for MAC debugging." (the compile-time diagnostic is redundant
because runtime log_info() provides the tracing and the `#warning` can cause build
failures with -Werror); rely on existing log_info() instrumentation instead of
emitting a compile-time `#warning`.
- Around line 100-104: The current code treats any non-"okay" status as a reason
to skip nodes; change the probe to be diagnostic-only and only skip when status
is explicitly "disabled": fetch the "status" with fdt_get_property into
status_prop and if status_prop && strcmp((const char *)status_prop->data,
"disabled") == 0 then log and increment total_skipped and continue; otherwise,
if status_prop exists and is not "disabled", just log the status with log_info
but do not continue. Reference symbols: fdt_get_property, status_prop, strcmp,
log_info, total_skipped, continue.
- Around line 32-46: The current code caches aliases_off once before iterating
aliases, but do_fixup_by_path() can modify the device tree and make aliases_off
stale; inside the for loop before calling fdt_first_property_offset and any use
of aliases_off, recompute aliases_off by calling fdt_path_offset(fdt,
"/aliases") and handle a negative return (log and break/return) so each
iteration uses a fresh /aliases offset; update references in the loop that use
aliases_off (and any subsequent calls like fdt_first_property_offset) to use the
recomputed value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 95c2e09e-d1df-4efd-ad23-2be938e8c83b
📒 Files selected for processing (2)
patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patchpatch/u-boot/v2026.04/board_helios64/board/sys_otp.c
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch (3)
133-133:⚠️ Potential issue | 🟡 MinorRemove the compile-time
#warningfrom carried patch code.Line 133 emits on every build and can become fatal under warning-as-error policies. Runtime
log_info()already provides sufficient diagnostics.Suggested fix
-#warning "Building image-fdt.c: fdt fixup call chain instrumented for MAC debugging."#!/bin/bash set -euo pipefail FILE="patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch" rg -n '#warning "Building image-fdt.c: fdt fixup call chain instrumented for MAC debugging\."' "$FILE"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch` at line 133, Remove the compile-time directive '#warning "Building image-fdt.c: fdt fixup call chain instrumented for MAC debugging."' from the patch so builds won't emit this warning (which can fail under -Werror); keep the existing runtime diagnostic via log_info() in the fdt fixup/ethernet instrumentation code (search for the '#warning' token or the instrumented image-fdt.c fdt fixup call chain message to locate the exact patch hunk and delete that line).
100-104:⚠️ Potential issue | 🟠 MajorThe
statusgate changes behavior beyond diagnostics.Line 101 now skips any node whose
statusis not exactly"okay". That is stricter than the earlier disabled-only behavior and can suppress valid MAC fixups.Suggested minimal fix
- const struct fdt_property *status_prop = fdt_get_property(fdt, nodeoff, "status", NULL); - if (status_prop && strcmp((const char *)status_prop->data, "okay")) { - log_info("[fdt_fixup_ethernet] Node '%s' status is '%s', skipping\n", path, (const char *)status_prop->data); - total_skipped++; - continue; - } + const struct fdt_property *status_prop = fdt_get_property(fdt, nodeoff, "status", NULL); + if (status_prop && !strcmp((const char *)status_prop->data, "disabled")) { + log_info("[fdt_fixup_ethernet] Node '%s' is disabled, skipping\n", path); + total_skipped++; + continue; + }#!/bin/bash set -euo pipefail FILE="patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch" rg -n -C2 'status_prop|status.*okay|status.*disabled' "$FILE"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch` around lines 100 - 104, The change makes fdt_fixup_ethernet skip any node whose status is not exactly "okay", which is too strict and will skip valid nodes; update the check in the fdt_fixup_ethernet logic so it only skips when the "status" property explicitly equals "disabled" (use strcmp(...) == 0), keep using status_prop and fdt_get_property to read the prop, and leave the log_info, total_skipped, path and nodeoff handling as-is so only disabled nodes are skipped instead of every non-"okay" node.
33-46:⚠️ Potential issue | 🟠 MajorRecompute
/aliasesoffset per iteration (current code contradicts Line 43 comment).Line 43 says to recompute after potential FDT edits, but Line 46 uses cached
aliases_off. Since fixups can mutate the blob, this can desync traversal and skip later aliases.Suggested minimal fix
- int aliases_off = fdt_path_offset(fdt, "/aliases"); - if (aliases_off < 0) { + if (fdt_path_offset(fdt, "/aliases") < 0) { log_info("[fdt_fixup_ethernet] /aliases node not found\n"); return; } @@ - offset = fdt_first_property_offset(fdt, aliases_off); + offset = fdt_first_property_offset(fdt, + fdt_path_offset(fdt, "/aliases"));#!/bin/bash set -euo pipefail FILE="patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch" rg -n -C2 'FDT might have been edited|aliases_off|fdt_first_property_offset\(fdt' "$FILE"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch` around lines 33 - 46, The loop comment says the /aliases offset must be recomputed because the FDT may be edited, but the code uses the cached aliases_off; change the loop to recompute the aliases node offset each iteration by calling fdt_path_offset(fdt, "/aliases") and pass that value into fdt_first_property_offset(fdt, ...); handle a negative return (e.g., break out of the loop) if the recomputed offset becomes invalid, and keep the initial not-found check for the first iteration as-is; update the surrounding comment if needed to reflect per-iteration recomputation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch`:
- Line 133: Remove the compile-time directive '#warning "Building image-fdt.c:
fdt fixup call chain instrumented for MAC debugging."' from the patch so builds
won't emit this warning (which can fail under -Werror); keep the existing
runtime diagnostic via log_info() in the fdt fixup/ethernet instrumentation code
(search for the '#warning' token or the instrumented image-fdt.c fdt fixup call
chain message to locate the exact patch hunk and delete that line).
- Around line 100-104: The change makes fdt_fixup_ethernet skip any node whose
status is not exactly "okay", which is too strict and will skip valid nodes;
update the check in the fdt_fixup_ethernet logic so it only skips when the
"status" property explicitly equals "disabled" (use strcmp(...) == 0), keep
using status_prop and fdt_get_property to read the prop, and leave the log_info,
total_skipped, path and nodeoff handling as-is so only disabled nodes are
skipped instead of every non-"okay" node.
- Around line 33-46: The loop comment says the /aliases offset must be
recomputed because the FDT may be edited, but the code uses the cached
aliases_off; change the loop to recompute the aliases node offset each iteration
by calling fdt_path_offset(fdt, "/aliases") and pass that value into
fdt_first_property_offset(fdt, ...); handle a negative return (e.g., break out
of the loop) if the recomputed offset becomes invalid, and keep the initial
not-found check for the first iteration as-is; update the surrounding comment if
needed to reflect per-iteration recomputation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9f633e4a-1e9f-4145-8942-51b7d4a29a46
📒 Files selected for processing (2)
patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patchpatch/u-boot/v2026.04/board_helios64/board/sys_otp.c
iav
added a commit
to iav/armbian
that referenced
this pull request
Apr 17, 2026
Per CodeRabbit review on PR armbian#9675: board/Kconfig ENV_OFFSET default 0x460000 conflicts with defconfig CONFIG_ENV_OFFSET=0x3F8000. Authoritative source - upstream U-Boot itself: 1. configs/helios64-rk3399_defconfig:7 sets CONFIG_ENV_OFFSET=0x3F8000 - this is what actually ends up in the build; the defconfig overrides the Kconfig default. 2. All 20+ rk3399 boards in upstream configs/*rk3399*_defconfig use 0x3F8000 (evb, firefly, nanopi-m4/r4s, pinebook-pro, rock-4c, khadas-edge, leez, orangepi, ...). 3. The 0x460000 in board/Kconfig is dead code: it is always shadowed by the defconfig, never applied in builds. It is a legacy artefact of the Kobol vendor patch series, not matching any standard SPI layout for rk3399. 4. Physically: on Helios64's 16 MiB W25Q128JV, 0x3F8000 = 4 MiB - 32 KiB reserves the last 32 KiB slot inside the lower 4 MiB window for env - the common rk3399 convention. Aligning the Kconfig default with the defconfig removes the inconsistency and makes the fallback path land at the same address the defconfig expects.
iav
added a commit
to iav/armbian
that referenced
this pull request
Apr 17, 2026
Per CodeRabbit review on PR armbian#9675: zstd_decompress_dctx() returns a size_t — the actual number of bytes written, not just an error indicator. The previous code unconditionally returned dlen even if ret < dlen, leaving the tail of dbuf as uninitialised garbage. In practice this path is unreachable for well-formed BTRFS extents: after our fix out_len = max(dlen, fcs), and on success zstd produces exactly fcs bytes, so ret == fcs >= dlen. But the defensive check is trivial and guards against: - a frame header with a falsified content-size that still passes the integrity check; - truncated/corrupted frames that zstd does not always flag as an error. Apply the same fix to the duplicate patch under board_helios64/. This issue was not flagged in PR armbian#9651 (v2026.01) — different review pass surfaced different findings.
iav
added a commit
to iav/armbian
that referenced
this pull request
Apr 17, 2026
Per CodeRabbit review on PR armbian#9675: is_valid_header() in board/sys_otp.c uses '||' between byte comparisons, returning 1 if *any* one byte of otp.magic matches the expected sequence. Practically every random byte pattern will trip at least one comparison, so the magic check is effectively meaningless — partially corrupt OTP data is treated as a valid Helios64 header. This is a pre-existing bug in the Kobol vendor code, carried forward unchanged through every U-Boot bump. We are migrating these board files to v2026.04, so the right place to fix it. Replace the chain with a single memcmp() against the expected magic: - removes the '||' vs '&&' confusion entirely; - reads as a normal magic-bytes check; - the constant lives in one place.
iav
added a commit
to iav/armbian
that referenced
this pull request
Apr 17, 2026
…_num Per CodeRabbit review on PR armbian#9675: *((uint64_t *) otp.serial_num) & 0xFFFFFFFFFFFF reads 8 bytes from a 6-byte field inside a __packed__ struct (over-reads into mfg_year), and is unaligned. UB per the C standard, and would fault on stricter alignment requirements. Same pattern affects two call sites the bot pointed at (lines 103-104 and 217-218 of the original vendor file). The bot did not flag it but *(u16 *) otp.mfg_year on the next line has the same problem - packed field, unaligned access - so fix it together. Replace the casts with two small inline helpers that decode the bytes explicitly little-endian (RK3399 is LE, matches the vendor's original intent): otp_serial() - assemble 6 bytes into a u64 otp_mfg_year() - assemble 2 bytes into a u16 This is pre-existing Kobol vendor code carried forward through every U-Boot bump; this PR migrates the board files to v2026.04, so it is the right place to fix it.
iav
added a commit
to iav/armbian
that referenced
this pull request
Apr 17, 2026
When FDT_SEQ_MACADDR_FROM_ENV is defined, nodeoff is already declared and assigned earlier in the function. The later 'int nodeoff = ...' shadowed that variable and performed a redundant fdt_path_offset() lookup. Guard the inner declaration with #ifndef so it only exists when the outer one does not. No behavior change — the same nodeoff value is checked either way. Addresses a CodeRabbit review comment on PR armbian#9675.
iav
added a commit
to iav/armbian
that referenced
this pull request
Apr 17, 2026
show_otp_data() directly indexed var_str[otp.variant] without validation.
If the OTP blob is corrupted (e.g. variant byte out of 0..BOARD_VARIANT_MAX-1
range), this read is out of bounds.
Mirror the check already used in get_variant(): fall back to var_str[0]
("Invalid variant") when otp.variant is out of range.
This is debug-only (wrapped in #ifdef DEBUG) and never reached in
production builds, but keeps behavior consistent with get_variant() and
avoids a latent OOB read if DEBUG is enabled.
Addresses a CodeRabbit review comment on PR armbian#9675.
iav
added a commit
to iav/armbian
that referenced
this pull request
Apr 17, 2026
spi_write_then_read() returned value was only used to set has_been_read, then header/CRC checks ran against whatever ended up in the otp struct. On a real SPI I/O failure, the struct could contain partial garbage, header/CRC would fail, and the caller got a generic -1 — losing the original errno and conflating I/O errors with invalid OTP data. Handle the I/O error path explicitly: - On ret != 0: log, keep has_been_read = 0 (so the next call can retry), and return the original ret. - On ret == 0: set has_been_read = 1 and proceed to header/CRC validation as before. Addresses a CodeRabbit review comment on PR armbian#9675.
iav
added a commit
to iav/armbian
that referenced
this pull request
Apr 17, 2026
The previous commit preserved has_been_read = 0 on SPI I/O failure so a later call could retry. But in the actual boot flow no retry happens: board_early_init_r() ignores the return of read_otp_data(), and the later consumers (set_board_info, mac_read_from_otp) bail out early with 'if (!is_data_valid()) return;' — so a single transient SPI failure leaves board_rev / serial# / ethaddr unset for the whole boot. Add an ensure_otp_data_ready() helper that re-invokes read_otp_data() when the cached data is not yet valid, and use it in both consumers. Valid/invalid-data behavior is unchanged; only the retry path is now actually exercised. Addresses a CodeRabbit review comment on PR armbian#9675.
iav
force-pushed
the
feat/helios64-uboot-v2026.04
branch
2 times, most recently
from
4165426 to
c5eeb8e
Compare
Contributor
Author
|
Fixups squashed + rebase. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@patch/u-boot/v2026.04/board_helios64/board/helios64.c`:
- Around line 180-188: The setup_leds function calls
led_get_by_label("helios64::status", &dev) but ignores its int return value,
risking use of an uninitialized dev; update setup_leds to capture and check the
return (e.g., int ret = led_get_by_label(...)) and only call led_set_state(dev,
...) when ret == 0, otherwise handle the error (log via printf/dev_err or simply
return) to avoid dereferencing an invalid dev pointer; ensure both LEDST_OFF and
LEDST_ON calls are guarded by this check.
- Around line 80-88: The current calls use rk_setreg(..., 0 << SHIFT) which is a
no-op; replace those with rk_clrreg so the low-selected IO-domain bits are
actually cleared. Specifically, change the rk_setreg calls that target
GRF_IO_VSEL_SDMMC_SHIFT and GRF_IO_VSEL_GPIO1830_SHIFT to
rk_clrreg(&grf->io_vsel, 1 << GRF_IO_VSEL_SDMMC_SHIFT) and
rk_clrreg(&grf->io_vsel, 1 << GRF_IO_VSEL_GPIO1830_SHIFT), and change the PMUGRF
calls using PMUGRF_CON0_VSEL_SHIFT and PMUGRF_CON0_PMU1830_VOL_SHIFT to
rk_clrreg(&pmugrf->soc_con0, 1 << PMUGRF_CON0_VSEL_SHIFT) and
rk_clrreg(&pmugrf->soc_con0, 1 << PMUGRF_CON0_PMU1830_VOL_SHIFT) so the bits are
explicitly cleared.
In `@patch/u-boot/v2026.04/board_helios64/board/sys_otp.c`:
- Around line 270-287: Update mac_read_from_otp to return a failure code when it
fails to apply a valid primary MAC (ethaddr): within the existing loop that
checks is_valid_ethaddr and calls eth_env_set_enetaddr, track whether the
primary index (i == 0) resulted in setting ethaddr (or check env_get("ethaddr")
after the loop). If the primary MAC was invalid and ethaddr remains unset,
return a non-zero error (instead of 0); otherwise return 0 as before. Use the
existing symbols mac_read_from_otp, is_valid_ethaddr, env_get, and
eth_env_set_enetaddr to locate where to add the boolean/condition and change the
return value.
In
`@patch/u-boot/v2026.04/board_helios64/cmd-fileenv-read-string-from-file-into-env.patch`:
- Around line 25-28: The Kconfig entry for CMD_FILEENV lacks a dependency on
CMD_FAT even though fileenv calls do_fat_fsload(); update the CMD_FILEENV stanza
to include "depends on CMD_FAT" so CONFIG_CMD_FAT is enabled whenever
CONFIG_CMD_FILEENV is selected, ensuring do_fat_fsload() (fatload) is available
at build and link time.
🪄 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: d6b9a3ec-0013-4a23-8c7b-e663a34cb8ce
📒 Files selected for processing (18)
config/boards/helios64.confpatch/u-boot/v2026.04/board_helios64/0000.patching_config.yamlpatch/u-boot/v2026.04/board_helios64/1000-exception-include-string.patchpatch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch.disabledpatch/u-boot/v2026.04/board_helios64/1001-helios64-enable-target.patchpatch/u-boot/v2026.04/board_helios64/board/Kconfigpatch/u-boot/v2026.04/board_helios64/board/MAINTAINERSpatch/u-boot/v2026.04/board_helios64/board/Makefilepatch/u-boot/v2026.04/board_helios64/board/helios64.cpatch/u-boot/v2026.04/board_helios64/board/sys_otp.cpatch/u-boot/v2026.04/board_helios64/board/sys_otp.hpatch/u-boot/v2026.04/board_helios64/cmd-fileenv-read-string-from-file-into-env.patchpatch/u-boot/v2026.04/board_helios64/defconfig/helios64-rk3399_defconfigpatch/u-boot/v2026.04/board_helios64/dt_uboot/rk3399-kobol-helios64-u-boot.dtsipatch/u-boot/v2026.04/board_helios64/dt_upstream_rockchip/rk3399-kobol-helios64.dtspatch/u-boot/v2026.04/board_helios64/general-fix-btrfs-zstd-decompression.patchpatch/u-boot/v2026.04/board_helios64/include_configs/helios64.hpatch/u-boot/v2026.04/general-fix-btrfs-zstd-decompression.patch
✅ Files skipped from review due to trivial changes (6)
- patch/u-boot/v2026.04/board_helios64/board/MAINTAINERS
- patch/u-boot/v2026.04/board_helios64/1000-exception-include-string.patch
- patch/u-boot/v2026.04/board_helios64/board/sys_otp.h
- patch/u-boot/v2026.04/board_helios64/0000.patching_config.yaml
- patch/u-boot/v2026.04/board_helios64/defconfig/helios64-rk3399_defconfig
- patch/u-boot/v2026.04/board_helios64/board/Kconfig
🚧 Files skipped from review as they are similar to previous changes (4)
- config/boards/helios64.conf
- patch/u-boot/v2026.04/general-fix-btrfs-zstd-decompression.patch
- patch/u-boot/v2026.04/board_helios64/general-fix-btrfs-zstd-decompression.patch
- patch/u-boot/v2026.04/board_helios64/dt_uboot/rk3399-kobol-helios64-u-boot.dtsi
Contributor
Author
|
Rabbit, you're kidding me |
Self-contained board_helios64/ overlay bumped to v2026.04 from v2026.01. The overlay is a pristine copy of v2026.01 plus the minimum set of v2026.04-mandated changes and new cherry-picks: - board/Kconfig: ENV_OFFSET 0x460000 → 0x3F8000 Required by the v2026.04 SPI NOR environment layout. - 1001-fdt_fixup_ethernet-add-logs.patch.disabled Cherry-pick of Ricardo Pardini's fdt_fixup_ethernet logging patch. Shipped disabled; preserved for debugging MAC propagation issues. - cmd-fileenv-read-string-from-file-into-env.patch Cherry-pick of the Home Assistant 2018 vendor patch (rebased by rpardini) adapting 'fileenv' u-boot command for v2026.04. config/boards/helios64.conf: BOOTBRANCH / BOOTPATCHDIR → v2026.04. No behaviour changes beyond what the u-boot version bump itself requires; pre-existing code paths (sys_otp.c, helios64.c) are left exactly as they were in v2026.01. Defensive code-style fixes on top of this baseline are a separate follow-up commit. Assisted-by: Claude:claude-opus-4-7
iav
force-pushed
the
feat/helios64-uboot-v2026.04
branch
from
c5eeb8e to
5fe2707
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
patch/u-boot/v2026.04/board_helios64/board/sys_otp.c (1)
263-288:⚠️ Potential issue | 🟠 MajorStill outstanding: return non-zero when primary
ethaddrwasn't applied.In
helios64.cline 215 the caller usesif (mac_read_from_otp()) ret = rockchip_setup_macaddr();— so returning0when slot 0 has an invalid OTP MAC andethaddrisn't already set suppresses the fallback and leaves the board without a primary MAC. Previous review on this was not marked as addressed; please revisit.🔧 Suggested minimal fix
int mac_read_from_otp(void) { unsigned int i; if (ensure_otp_data_ready()) return -1; for (i = 0; i < MAX_NUM_PORTS; i++) { char enetvar[9]; sprintf(enetvar, i ? "eth%daddr" : "ethaddr", i); if (!is_valid_ethaddr(otp.mac_addr[i])) { debug("Not valid %s!\n", enetvar); continue; } /* Only initialize environment variables that are blank * (i.e. have not yet been set) */ if (!env_get(enetvar)) eth_env_set_enetaddr(enetvar, otp.mac_addr[i]); } - return 0; + return env_get("ethaddr") ? 0 : -1; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/u-boot/v2026.04/board_helios64/board/sys_otp.c` around lines 263 - 288, mac_read_from_otp currently always returns 0 even when the primary MAC (slot 0 / "ethaddr") was not applied, which suppresses the caller fallback; update mac_read_from_otp to detect if the primary ethaddr was neither valid in otp.mac_addr[0] nor already present in the environment (env_get("ethaddr") returns NULL) and in that case return non-zero (e.g. -1) so the caller (helios64.c) will call rockchip_setup_macaddr(); keep the existing loop behavior for other ports (use is_valid_ethaddr, otp.mac_addr[i], eth_env_set_enetaddr) but track/apply a boolean flag indicating whether the primary ethaddr was successfully applied or already present and base the function’s return value on that flag while still returning -1 on ensure_otp_data_ready() failure.patch/u-boot/v2026.04/board_helios64/board/helios64.c (1)
80-88:⚠️ Potential issue | 🟠 MajorStill outstanding:
rk_setreg(..., 0 << SHIFT)is a no-op — userk_clrreg().These three calls were flagged previously and the comment wasn't marked addressed.
rk_setregORs the value into the register; ORing zero never clears a bit. To actually select the 3.0V domain for SDMMC/GPIO1830 and drivePMU1830_VOLlow, the bits must be cleared explicitly viark_clrreg. If the board happens to work today, it's because the bits were already cleared at reset — not because this code did it.🔧 Suggested fix
/* SDMMC is in 3.0v domain */ - rk_setreg(&grf->io_vsel, 0 << GRF_IO_VSEL_SDMMC_SHIFT); + rk_clrreg(&grf->io_vsel, 1 << GRF_IO_VSEL_SDMMC_SHIFT); /* GPIO1830 is in 3.0v domain */ - rk_setreg(&grf->io_vsel, 0 << GRF_IO_VSEL_GPIO1830_SHIFT); + rk_clrreg(&grf->io_vsel, 1 << GRF_IO_VSEL_GPIO1830_SHIFT); /* Set GPIO1 1.8v/3.0v source select to PMU1830_VOL */ rk_setreg(&pmugrf->soc_con0, 1 << PMUGRF_CON0_VSEL_SHIFT); - rk_setreg(&pmugrf->soc_con0, 0 << PMUGRF_CON0_PMU1830_VOL_SHIFT); + rk_clrreg(&pmugrf->soc_con0, 1 << PMUGRF_CON0_PMU1830_VOL_SHIFT);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/u-boot/v2026.04/board_helios64/board/helios64.c` around lines 80 - 88, The three rk_setreg calls that pass 0 << SHIFT are no-ops and must clear the bits instead: replace rk_setreg(&grf->io_vsel, 0 << GRF_IO_VSEL_SDMMC_SHIFT) with rk_clrreg(&grf->io_vsel, 1 << GRF_IO_VSEL_SDMMC_SHIFT), replace rk_setreg(&grf->io_vsel, 0 << GRF_IO_VSEL_GPIO1830_SHIFT) with rk_clrreg(&grf->io_vsel, 1 << GRF_IO_VSEL_GPIO1830_SHIFT), and replace rk_setreg(&pmugrf->soc_con0, 0 << PMUGRF_CON0_PMU1830_VOL_SHIFT) with rk_clrreg(&pmugrf->soc_con0, 1 << PMUGRF_CON0_PMU1830_VOL_SHIFT); keep the rk_setreg(&pmugrf->soc_con0, 1 << PMUGRF_CON0_VSEL_SHIFT) call as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@patch/u-boot/v2026.04/board_helios64/board/helios64.c`:
- Around line 80-88: The three rk_setreg calls that pass 0 << SHIFT are no-ops
and must clear the bits instead: replace rk_setreg(&grf->io_vsel, 0 <<
GRF_IO_VSEL_SDMMC_SHIFT) with rk_clrreg(&grf->io_vsel, 1 <<
GRF_IO_VSEL_SDMMC_SHIFT), replace rk_setreg(&grf->io_vsel, 0 <<
GRF_IO_VSEL_GPIO1830_SHIFT) with rk_clrreg(&grf->io_vsel, 1 <<
GRF_IO_VSEL_GPIO1830_SHIFT), and replace rk_setreg(&pmugrf->soc_con0, 0 <<
PMUGRF_CON0_PMU1830_VOL_SHIFT) with rk_clrreg(&pmugrf->soc_con0, 1 <<
PMUGRF_CON0_PMU1830_VOL_SHIFT); keep the rk_setreg(&pmugrf->soc_con0, 1 <<
PMUGRF_CON0_VSEL_SHIFT) call as-is.
In `@patch/u-boot/v2026.04/board_helios64/board/sys_otp.c`:
- Around line 263-288: mac_read_from_otp currently always returns 0 even when
the primary MAC (slot 0 / "ethaddr") was not applied, which suppresses the
caller fallback; update mac_read_from_otp to detect if the primary ethaddr was
neither valid in otp.mac_addr[0] nor already present in the environment
(env_get("ethaddr") returns NULL) and in that case return non-zero (e.g. -1) so
the caller (helios64.c) will call rockchip_setup_macaddr(); keep the existing
loop behavior for other ports (use is_valid_ethaddr, otp.mac_addr[i],
eth_env_set_enetaddr) but track/apply a boolean flag indicating whether the
primary ethaddr was successfully applied or already present and base the
function’s return value on that flag while still returning -1 on
ensure_otp_data_ready() failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 092c2aad-5315-4084-ac22-bd97467a99ac
📒 Files selected for processing (18)
config/boards/helios64.confpatch/u-boot/v2026.04/board_helios64/0000.patching_config.yamlpatch/u-boot/v2026.04/board_helios64/1000-exception-include-string.patchpatch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch.disabledpatch/u-boot/v2026.04/board_helios64/1001-helios64-enable-target.patchpatch/u-boot/v2026.04/board_helios64/board/Kconfigpatch/u-boot/v2026.04/board_helios64/board/MAINTAINERSpatch/u-boot/v2026.04/board_helios64/board/Makefilepatch/u-boot/v2026.04/board_helios64/board/helios64.cpatch/u-boot/v2026.04/board_helios64/board/sys_otp.cpatch/u-boot/v2026.04/board_helios64/board/sys_otp.hpatch/u-boot/v2026.04/board_helios64/cmd-fileenv-read-string-from-file-into-env.patchpatch/u-boot/v2026.04/board_helios64/defconfig/helios64-rk3399_defconfigpatch/u-boot/v2026.04/board_helios64/dt_uboot/rk3399-kobol-helios64-u-boot.dtsipatch/u-boot/v2026.04/board_helios64/dt_upstream_rockchip/rk3399-kobol-helios64.dtspatch/u-boot/v2026.04/board_helios64/general-fix-btrfs-zstd-decompression.patchpatch/u-boot/v2026.04/board_helios64/include_configs/helios64.hpatch/u-boot/v2026.04/general-fix-btrfs-zstd-decompression.patch
✅ Files skipped from review due to trivial changes (6)
- patch/u-boot/v2026.04/board_helios64/board/MAINTAINERS
- patch/u-boot/v2026.04/board_helios64/1000-exception-include-string.patch
- patch/u-boot/v2026.04/board_helios64/0000.patching_config.yaml
- patch/u-boot/v2026.04/board_helios64/board/sys_otp.h
- patch/u-boot/v2026.04/board_helios64/board/Kconfig
- patch/u-boot/v2026.04/board_helios64/defconfig/helios64-rk3399_defconfig
🚧 Files skipped from review as they are similar to previous changes (5)
- patch/u-boot/v2026.04/board_helios64/1001-helios64-enable-target.patch
- patch/u-boot/v2026.04/general-fix-btrfs-zstd-decompression.patch
- patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch.disabled
- config/boards/helios64.conf
- patch/u-boot/v2026.04/board_helios64/dt_uboot/rk3399-kobol-helios64-u-boot.dtsi
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@patch/u-boot/v2026.04/board_helios64/board/helios64.c`:
- Around line 269-294: sata_power_enable currently waits 10s only after enabling
hdd_a_power, so hdd_b_power may not be spun up before last_stage_init calls
scsi_scan; modify sata_power_enable (and/or last_stage_init) so that both
regulators returned by regulator_get_by_platname ("hdd_a_power" and
"hdd_b_power") are enabled first via regulator_set_enable, then perform the
spin-up wait (e.g., mdelay(10000)) after both enable calls complete (or
poll/regulator_is_enabled for both) before returning, ensuring last_stage_init
calls scsi_scan(true) only after both bays are ready.
🪄 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: 85ab69a1-3adc-40b8-8fb3-b7450ef32456
📒 Files selected for processing (18)
config/boards/helios64.confpatch/u-boot/v2026.04/board_helios64/0000.patching_config.yamlpatch/u-boot/v2026.04/board_helios64/1000-exception-include-string.patchpatch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch.disabledpatch/u-boot/v2026.04/board_helios64/1001-helios64-enable-target.patchpatch/u-boot/v2026.04/board_helios64/board/Kconfigpatch/u-boot/v2026.04/board_helios64/board/MAINTAINERSpatch/u-boot/v2026.04/board_helios64/board/Makefilepatch/u-boot/v2026.04/board_helios64/board/helios64.cpatch/u-boot/v2026.04/board_helios64/board/sys_otp.cpatch/u-boot/v2026.04/board_helios64/board/sys_otp.hpatch/u-boot/v2026.04/board_helios64/cmd-fileenv-read-string-from-file-into-env.patchpatch/u-boot/v2026.04/board_helios64/defconfig/helios64-rk3399_defconfigpatch/u-boot/v2026.04/board_helios64/dt_uboot/rk3399-kobol-helios64-u-boot.dtsipatch/u-boot/v2026.04/board_helios64/dt_upstream_rockchip/rk3399-kobol-helios64.dtspatch/u-boot/v2026.04/board_helios64/general-fix-btrfs-zstd-decompression.patchpatch/u-boot/v2026.04/board_helios64/include_configs/helios64.hpatch/u-boot/v2026.04/general-fix-btrfs-zstd-decompression.patch
✅ Files skipped from review due to trivial changes (7)
- patch/u-boot/v2026.04/board_helios64/1000-exception-include-string.patch
- patch/u-boot/v2026.04/board_helios64/board/MAINTAINERS
- patch/u-boot/v2026.04/board_helios64/0000.patching_config.yaml
- patch/u-boot/v2026.04/board_helios64/1001-helios64-enable-target.patch
- patch/u-boot/v2026.04/board_helios64/board/sys_otp.h
- patch/u-boot/v2026.04/board_helios64/board/Kconfig
- patch/u-boot/v2026.04/board_helios64/dt_uboot/rk3399-kobol-helios64-u-boot.dtsi
🚧 Files skipped from review as they are similar to previous changes (6)
- patch/u-boot/v2026.04/general-fix-btrfs-zstd-decompression.patch
- patch/u-boot/v2026.04/board_helios64/general-fix-btrfs-zstd-decompression.patch
- config/boards/helios64.conf
- patch/u-boot/v2026.04/board_helios64/defconfig/helios64-rk3399_defconfig
- patch/u-boot/v2026.04/board_helios64/cmd-fileenv-read-string-from-file-into-env.patch
- patch/u-boot/v2026.04/board_helios64/dt_upstream_rockchip/rk3399-kobol-helios64.dts
Code-style and defensive-programming fixes to the helios64 u-boot
overlay, applied on top of the pristine v2026.04 vendor bump. All
changes are additive safety nets — none alter the happy-path
behaviour of the previous Kobol/Armbian code.
board/helios64.c:
- setup_leds(): check led_get_by_label() return value. Without
this, led_set_state() is called on an uninitialised udevice
pointer if LED lookup fails.
- sata_power_enable(): add the matching 10s mdelay after
enabling hdd_b_power, mirroring the one already present after
hdd_a_power. Previously rail B had no spin-up time before
scsi_scan(true) in last_stage_init(), so drives on the second
rail could miss the scan. The staggered spin-up between rails
is preserved (PSU in-rush protection for two 3.5" HDDs).
board/sys_otp.c:
- replace the || chain magic-number check in is_valid_header()
with memcmp() against a const expected_magic[8] (fixes a real
pre-existing bug: the || chain was always true);
- introduce otp_serial() / otp_mfg_year() helpers that assemble
the multi-byte values byte-by-byte, eliminating unaligned
u64/u16 casts into the packed otp struct (UB per the C standard,
also over-read into adjacent fields);
- bounds-check otp.variant against BOARD_VARIANT_MAX before
indexing var_str[], adding BOARD_VARIANT_INVALID slot;
- use strnlen() when printing packed fixed-length fields;
- use snprintf() with target buffer size instead of sprintf();
- retry read_otp_data() in ensure_otp_data_ready() on the first
access after a failed board_early_init_r(), instead of silently
skipping OTP-provided values for the whole boot.
cmd-fileenv-read-string-from-file-into-env.patch:
add 'depends on CMD_FAT' to the cherry-picked CMD_FILEENV Kconfig
entry. do_fat_fsload() is called unconditionally from the
fileenv implementation, so the dependency reflects reality.
general-fix-btrfs-zstd-decompression.patch:
tighten the decompression-success check to also reject short
reads ('|| ret < dlen'). zstd_is_error() alone does not cover
truncated output.
Assisted-by: Claude:claude-opus-4-7
Same fix as armbian#9651 for v2026.01 and v2025.10, applied to the common patch/u-boot/v2026.04/ directory so all boards on U-Boot v2026.04 pick it up (cm3588-nas, nanopct6, orangepi5, rock-3a, nanopi-r3s-lts, mekotronics-*, mixtile-blade3, radxa-rock-4d, helios64 via its own self-contained board_helios64 subdir already has a copy). Without this, booting from a BTRFS rootfs with zstd-compressed extents fails with: zstd_decompress: failed to decompress: 70 BTRFS: An error occurred while reading file /boot/boot.scr See commit 5617ff3 for the full rationale.
iav
force-pushed
the
feat/helios64-uboot-v2026.04
branch
from
5fe2707 to
11ee9e3
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
patch/u-boot/v2026.04/board_helios64/general-fix-btrfs-zstd-decompression.patch (1)
78-87: Optional:fcs > SIZE_MAXguard is dead on 64-bit targets.
fcsisunsigned long longandmalloc's argument issize_t. On aarch64 (Helios64's target)SIZE_MAX == ULLONG_MAX, so thefcs > SIZE_MAXbranch can never trigger, and a pathologically hugefcsfrom a hostile frame header would be passed straight tomalloc.mallocwill fail and the subsequent!tmpcheck handles it correctly, so this is purely a defensive-coding nit — functionally fine today.If you want a stricter guard that's meaningful on all targets, something like a policy cap (e.g., reject
fcsabove a few MiB, since BTRFS compresses per-sector/per-extent and frames beyond that are implausible here) would be more useful than theSIZE_MAXcomparison. Given the maintainer preference for keeping this patch consistent across v2026.01 / v2026.04 / common, deferring is also perfectly reasonable.Based on learnings: "When porting patches between U-Boot versions (like from 2025.01 to 2025.04), rpardini prefers to maintain patches as-is rather than introducing refactoring changes" — feel free to ignore if you'd rather keep the three copies of this patch bit-identical.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/u-boot/v2026.04/board_helios64/general-fix-btrfs-zstd-decompression.patch` around lines 78 - 87, The check "if (fcs > SIZE_MAX)" is ineffective on 64-bit targets because fcs is unsigned long long and SIZE_MAX == ULLONG_MAX; remove this dead comparison and replace it with a meaningful defensive cap (e.g., define a MAX_FCS_CAP like 16*1024*1024) and change the condition to "if (fcs > MAX_FCS_CAP) return -1;" before calling malloc; update references around fcs, malloc, tmp, out_buf and out_len so the code rejects implausibly large frame sizes consistently across targets.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@patch/u-boot/v2026.04/board_helios64/board/helios64.c`:
- Around line 153-173: Current logic only swaps boot_targets if the "mmc0"
substring occurs before "mmc1" (if (mmc0 < mmc1)), but on this platform the
default boot_targets may list "mmc1 mmc0 ..." so we should swap when mmc1
precedes mmc0; change the condition in the SD-boot handling block (where mmc0 =
strstr(env, "mmc0"); mmc1 = strstr(env, "mmc1"); and the if (mmc0 < mmc1)
branch) to if (mmc1 < mmc0) so mmc0 is promoted to first, keep the same
byte-editing (mmc0[3] = '1'; mmc1[3] = '0';) and call env_set("boot_targets",
env) as before.
---
Nitpick comments:
In
`@patch/u-boot/v2026.04/board_helios64/general-fix-btrfs-zstd-decompression.patch`:
- Around line 78-87: The check "if (fcs > SIZE_MAX)" is ineffective on 64-bit
targets because fcs is unsigned long long and SIZE_MAX == ULLONG_MAX; remove
this dead comparison and replace it with a meaningful defensive cap (e.g.,
define a MAX_FCS_CAP like 16*1024*1024) and change the condition to "if (fcs >
MAX_FCS_CAP) return -1;" before calling malloc; update references around fcs,
malloc, tmp, out_buf and out_len so the code rejects implausibly large frame
sizes consistently across targets.
🪄 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: 742fc330-94c2-4ed3-9222-571aff331ce8
📒 Files selected for processing (18)
config/boards/helios64.confpatch/u-boot/v2026.04/board_helios64/0000.patching_config.yamlpatch/u-boot/v2026.04/board_helios64/1000-exception-include-string.patchpatch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch.disabledpatch/u-boot/v2026.04/board_helios64/1001-helios64-enable-target.patchpatch/u-boot/v2026.04/board_helios64/board/Kconfigpatch/u-boot/v2026.04/board_helios64/board/MAINTAINERSpatch/u-boot/v2026.04/board_helios64/board/Makefilepatch/u-boot/v2026.04/board_helios64/board/helios64.cpatch/u-boot/v2026.04/board_helios64/board/sys_otp.cpatch/u-boot/v2026.04/board_helios64/board/sys_otp.hpatch/u-boot/v2026.04/board_helios64/cmd-fileenv-read-string-from-file-into-env.patchpatch/u-boot/v2026.04/board_helios64/defconfig/helios64-rk3399_defconfigpatch/u-boot/v2026.04/board_helios64/dt_uboot/rk3399-kobol-helios64-u-boot.dtsipatch/u-boot/v2026.04/board_helios64/dt_upstream_rockchip/rk3399-kobol-helios64.dtspatch/u-boot/v2026.04/board_helios64/general-fix-btrfs-zstd-decompression.patchpatch/u-boot/v2026.04/board_helios64/include_configs/helios64.hpatch/u-boot/v2026.04/general-fix-btrfs-zstd-decompression.patch
✅ Files skipped from review due to trivial changes (6)
- patch/u-boot/v2026.04/board_helios64/1000-exception-include-string.patch
- patch/u-boot/v2026.04/board_helios64/0000.patching_config.yaml
- patch/u-boot/v2026.04/board_helios64/board/MAINTAINERS
- patch/u-boot/v2026.04/board_helios64/board/sys_otp.h
- patch/u-boot/v2026.04/board_helios64/board/Kconfig
- patch/u-boot/v2026.04/board_helios64/cmd-fileenv-read-string-from-file-into-env.patch
🚧 Files skipped from review as they are similar to previous changes (4)
- patch/u-boot/v2026.04/board_helios64/1001-helios64-enable-target.patch
- patch/u-boot/v2026.04/board_helios64/defconfig/helios64-rk3399_defconfig
- patch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch.disabled
- patch/u-boot/v2026.04/board_helios64/dt_uboot/rk3399-kobol-helios64-u-boot.dtsi
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@patch/u-boot/v2026.04/general-fix-btrfs-zstd-decompression.patch`:
- Around line 77-87: In decompress_zstd(), ZSTD_getFrameContentSize(cbuf, clen)
can return an unbounded fcs which is used directly in malloc; cap fcs to a safe
upper bound before allocation (for example: if fcs > dlen + 4096 then set fcs =
dlen + 4096) while still checking fcs != ZSTD_CONTENTSIZE_ERROR and !=
ZSTD_CONTENTSIZE_UNKNOWN and fcs <= SIZE_MAX; then use the capped fcs for malloc
and to set out_buf/out_len (symbols: decompress_zstd, ZSTD_getFrameContentSize,
cbuf, clen, dlen, fcs, malloc, out_buf, out_len).
🪄 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: f4b1a059-f722-4153-9b01-e6823c9f41e5
📒 Files selected for processing (18)
config/boards/helios64.confpatch/u-boot/v2026.04/board_helios64/0000.patching_config.yamlpatch/u-boot/v2026.04/board_helios64/1000-exception-include-string.patchpatch/u-boot/v2026.04/board_helios64/1001-fdt_fixup_ethernet-add-logs.patch.disabledpatch/u-boot/v2026.04/board_helios64/1001-helios64-enable-target.patchpatch/u-boot/v2026.04/board_helios64/board/Kconfigpatch/u-boot/v2026.04/board_helios64/board/MAINTAINERSpatch/u-boot/v2026.04/board_helios64/board/Makefilepatch/u-boot/v2026.04/board_helios64/board/helios64.cpatch/u-boot/v2026.04/board_helios64/board/sys_otp.cpatch/u-boot/v2026.04/board_helios64/board/sys_otp.hpatch/u-boot/v2026.04/board_helios64/cmd-fileenv-read-string-from-file-into-env.patchpatch/u-boot/v2026.04/board_helios64/defconfig/helios64-rk3399_defconfigpatch/u-boot/v2026.04/board_helios64/dt_uboot/rk3399-kobol-helios64-u-boot.dtsipatch/u-boot/v2026.04/board_helios64/dt_upstream_rockchip/rk3399-kobol-helios64.dtspatch/u-boot/v2026.04/board_helios64/general-fix-btrfs-zstd-decompression.patchpatch/u-boot/v2026.04/board_helios64/include_configs/helios64.hpatch/u-boot/v2026.04/general-fix-btrfs-zstd-decompression.patch
✅ Files skipped from review due to trivial changes (5)
- patch/u-boot/v2026.04/board_helios64/board/MAINTAINERS
- patch/u-boot/v2026.04/board_helios64/1000-exception-include-string.patch
- patch/u-boot/v2026.04/board_helios64/0000.patching_config.yaml
- patch/u-boot/v2026.04/board_helios64/board/sys_otp.h
- patch/u-boot/v2026.04/board_helios64/defconfig/helios64-rk3399_defconfig
🚧 Files skipped from review as they are similar to previous changes (5)
- patch/u-boot/v2026.04/board_helios64/general-fix-btrfs-zstd-decompression.patch
- patch/u-boot/v2026.04/board_helios64/1001-helios64-enable-target.patch
- patch/u-boot/v2026.04/board_helios64/cmd-fileenv-read-string-from-file-into-env.patch
- patch/u-boot/v2026.04/board_helios64/board/Kconfig
- patch/u-boot/v2026.04/board_helios64/dt_uboot/rk3399-kobol-helios64-u-boot.dtsi
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
2 participants
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.
Three commits, grouped into one PR since they all touch
patch/u-boot/v2026.04/:1.
helios64: bump u-boot to v2026.04patch/u-boot/v2026.04/board_helios64/— helios64 uses a self-containedBOOTPATCHDIRand does not inherit from the parent dir.2.
helios64: u-boot: cherry-pick fdt_fixup_ethernet logs and fileenv cmdPull two generic v2026.04 patches that helios64 currently misses because
BOOTPATCHDIRpoints at the self-containedboard_helios64subdirectory:1001-fdt_fixup_ethernet-add-logs.patch— diagnosticlog_info()in DT Ethernet fixup.cmd-fileenv-read-string-from-file-into-env.patch— adds thefileenvCLI command.Enables
CONFIG_CMD_FILEENVvia apost_config_uboot_target__helios64_enable_fileenvhook inhelios64.conf, matching the convention of odroidm1 / khadas-vim3 / odroidm1s (patch + enable in the same commit). The command is purely additive — does not affectarmbianEnv.txtor existing env mechanics.Other parent-level v2026.04 patches (rk3288 VOP, rk3588 I2S MCLK, DW-HDMI disable) are SoC-specific or require config options helios64 does not enable, so they are not pulled in.
3.
u-boot: v2026.04: add btrfs zstd decompression fixSame fix as #9651 for v2026.01 and v2025.10, applied to the common
patch/u-boot/v2026.04/directory so all boards on U-Boot v2026.04 pick it up (cm3588-nas, nanopct6, orangepi5, rock-3a, nanopi-r3s-lts, mekotronics-*, mixtile-blade3, radxa-rock-4d — helios64 via its own self-containedboard_helios64subdir already has a copy from commit 1).Without this, booting from a BTRFS rootfs with zstd-compressed extents fails with:
Test plan
./compile.sh iav uboot BOARD=helios64 BRANCH=edge—linux-u-boot-helios64-edge_..._2026.04-S88dc-Pab2e-H1c4d-Vfcb0-B138f-R448a.deb.armbian-install, reboot to login prompt.U-Boot 2026.04_armbian-2026.04-S88dc-Pab2e-....fdt_fixup_ethernetdebug output visible: aliases enumerated, MAC patched intoethernet@fe300000.btrfs+BTRFS_COMPRESSION=zstd), nozstd_decompress: failed to decompress: 70.fileenvavailable in U-Boot CLI (config hook present, command usable when needed).Summary by CodeRabbit
New Features
Bug Fixes
Chores