Skip to content

Conversation

tmshlvck
Copy link

@tmshlvck tmshlvck commented Oct 1, 2025

Description

OrangePi RV2 features Ky X1 SoC that seems to be similar to Spacemit K1, however the bananapif3 board is not 100% compatible because of a different boot process (OPi RV2 uses SPI flash, unlike eMMC on BananaPi-F3), different Ethernet PHYs and different WiFi module.

This patch implements board support for OrangePi RV2 including:

  • New KY family configuration for RISC-V platform
  • Custom U-Boot sources from OrangePi (v2022.10-ky branch)
  • Custom kernel sources (orange-pi-6.6-ky branch)
  • SPI NOR flash boot support with MTD partitioning
  • Hardware support for WiFi (AP6256), Bluetooth, CAN, V4L
  • BSP packages and platform-specific configurations
  • Boot scripts and environment files

Board boots from SPI flash with specialized bootloader layout.

Based on OrangePi BSP adapted for current Armbian architecture. https://github.com/orangepi-xunlong/orangepi-build.git

How Has This Been Tested?

[*] Image built with

./compile.sh build BOARD=orangepirv2 BRANCH=edge RELEASE=trixie

[*] Written to the SD card and test-booted

[    0.000000] Linux version 6.6.63-edge-ky (build@armbian) (riscv64-linux-gnu-gcc (Ubuntu 13.3.0-6ubuntu2~24.04) 13.3.0, GNU ld (GNU Binutils for Ubuntu) 2.42) #4 SMP PREEMPT Tue Mar 18 02:29:27 UTC 2025
[    0.000000] Machine model: ky x1 orangepi-rv2 board
....

[] Written to the NVMe SSD and test-booted
[
] Ethernet works

[    6.463699] x1_emac cac80000.ethernet end0: renamed from eth0
[    6.504825] x1_emac cac81000.ethernet end1: renamed from eth1
[   43.377774] x1_emac cac80000.ethernet end0: registered PTP clock
[   46.455296] x1_emac cac80000.ethernet end0: Link is Up - 1Gbps/Full - flow control rx/tx

[*] WiFi works

[    3.292460] ky-wlan rf-pwrseq:wlan-pwrseq: error -ENXIO: IRQ index 0 not found
[    3.296957] ky-wlan rf-pwrseq:wlan-pwrseq: get platform irq failed, try to get gpio irq
[   10.217539] [dhd] STATIC-MSG) dhd_static_buf_init : 101.10.361.36 (wlan=r892223-20231107-1)
[   10.226489] [dhd] STATIC-MSG) dhd_init_wlan_mem : prealloc ok for index 0: 8459264(8261K)
[   10.303821] [dhd] dhd_wlan_init_gpio: WL_HOST_WAKE=-1, oob_irq=72, oob_irq_flags=0x1
[   10.303836] [dhd] dhd_wlan_init_gpio: WL_REG_ON=-1
[   10.975192] [dhd] STATIC-MSG) dhd_wlan_mem_prealloc : section 3, size 139264
[   10.986735] [dhd] STATIC-MSG) dhd_wlan_mem_prealloc : section 4, size 0
[   11.046406] [dhd] STATIC-MSG) dhd_wlan_mem_prealloc : section 7, size 42968
[   11.060940] [dhd] STATIC-MSG) dhd_wlan_mem_prealloc : section 0, size 10320
[   11.100275] [dhd] STATIC-MSG) dhd_wlan_mem_prealloc : section 5, size 65536
[   11.116201] [dhd] STATIC-MSG) dhd_wlan_mem_prealloc : section 19, size 65720
[   11.188396] [dhd] STATIC-MSG) dhd_wlan_mem_prealloc : section 1, size 10300
[   11.195559] [dhd] STATIC-MSG) dhd_wlan_mem_prealloc : section 2, size 65536
[   11.225519] [dhd] Register interface [wlan0]  MAC: 40:d9:5a:0d:b6:b0
[   11.233572] [dhd] [wlan0] wl_android_wifi_off :  g_wifi_on=1 force_off=1
[   11.310743] [dhd] [wlan0] wl_android_wifi_off : out
[54418.421178] [dhd] [wlan0] dhd_open : Enter
[54418.448341] [dhd] [wlan0] wl_android_wifi_on : in g_wifi_on=0
[54419.639214] [dhd] [wlan0] wl_android_wifi_on : Success
[54419.705040] [dhd] [wlan0] dhd_open : Exit ret=0
[54419.709625] [dhd] [wlan0] dhd_pri_open : tx queue started
[54434.119570] [dhd] [wlan0] wl_run_escan : LEGACY_SCAN sync ID: 0, bssidx: 0

Checklist:

Please delete options that are not relevant.

  • [?] My code follows the style guidelines of this project
  • [?] I have performed a self-review of my own code
  • [*] My changes generate no new warnings

Implements complete board support for OrangePi RV2 including:
- New KY family configuration for RISC-V platform
- Custom U-Boot sources from OrangePi (v2022.10-ky branch)
- Custom kernel sources (orange-pi-6.6-ky branch)
- SPI NOR flash boot support with MTD partitioning
- Hardware support for WiFi (AP6256), Bluetooth, CAN, V4L
- BSP packages and platform-specific configurations
- Boot scripts and environment files

Board boots from SPI flash with specialized bootloader layout.

Based on OrangePi BSP adapted for current Armbian architecture.
https://github.com/orangepi-xunlong/orangepi-build.git

Tested:
- board booted from SPI (Bootloader updated by the OrangePi BSP) -> SD
  (image generated by Armbian)
- board booted from SPI -> NVMe SSD in the back slot
  (image generate by Armbian and dd-transfered to the SSD)
- ethernet and WiFi works
@tmshlvck tmshlvck requested review from a team and igorpecovnik as code owners October 1, 2025 14:42
Copy link
Contributor

coderabbitai bot commented Oct 1, 2025

Walkthrough

Adds Orange Pi RV2 board config with metadata, U-Boot extra configs, and BSP tweaks. Introduces a new “ky” family with kernel/boot settings, U-Boot write helpers, and BSP extras. Adds KY boot environment and boot script. Includes initramfs hook to bundle firmware, camera test JSON configs, and platform setup script. Provides a comprehensive USB gadget management tool, an Access Point setup utility, and an SSH reset script. Adds systemd overrides and an autologin helper for getty/serial-getty. Changes are primarily new files: configs, boot scripts, BSP utilities, and system services.

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Suggested labels

Needs review, size/large, Hardware, BSP, Patches, Framework

Suggested reviewers

  • igorpecovnik
  • clee
  • rpardini
  • leggewie
  • EvilOlaf
  • PanderMusubi
  • teknoid
  • janprunk

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 15.09% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title succinctly and accurately summarizes the primary change of adding support for the OrangePi RV2 board on the RISC-V KY platform, reflecting the main focus of the pull request without introducing irrelevant details or vague terms.
Description Check ✅ Passed The pull request description clearly outlines the board support additions for the OrangePi RV2, including the KY family configuration, custom U-Boot and kernel sources, SPI flash boot support, hardware peripherals, BSP packages, boot scripts, and environment files, which directly corresponds to the files and changes summarized in the diff. It also describes the testing steps and results observed on SD card and NVMe SSD, matching the implementation details in the raw summary. Therefore, the description is relevant and sufficiently specific to the changeset.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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

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

@github-actions github-actions bot added 11 Milestone: Fourth quarter release size/large PR with 250 lines or more Needs review Seeking for review Hardware Hardware related like kernel, U-Boot, ... Patches Patches related to kernel, U-Boot, ... BSP Board Support Packages labels Oct 1, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 20

♻️ Duplicate comments (1)
packages/bsp/ky/usr/lib/systemd/system/serial-getty@.service.d/override.conf (1)

1-5: Same autologin security concerns as getty@.service.d/override.conf.

This override is identical to the getty@.service.d/override.conf and presents the same security considerations: it enables passwordless autologin as user orangepi on the serial console. The same recommendations apply:

  • Document this behavior prominently
  • Consider making it optional or configurable
  • Restrict to development/testing images if possible
  • Explain or remove the 10-second sleep if it's not necessary
🧹 Nitpick comments (20)
packages/bsp/ky/opt/camtest_sensor0_mode0.json (1)

2-4: Optional: keep 0/1 booleans unless consumer supports true/false.

If the camtest tooling accepts JSON booleans, consider using true/false for enable and src_from_file to improve clarity. Otherwise, leave as-is for compatibility.

Also applies to: 9-12, 20-23, 33-37, 52-56

packages/bsp/ky/usr/local/bin/create_ap (6)

638-638: Default to WPA2 only (drop WPA1 by default).

WPA1 is obsolete. Prefer WPA2 as default; still allow override via flags.

-WPA_VERSION=1+2
+WPA_VERSION=2

1700-1714: Disable TKIP; use CCMP-only for WPA2.

TKIP is deprecated and weak. Hostapd should advertise CCMP only by default.

 if [[ -n "$PASSPHRASE" ]]; then
   [[ "$WPA_VERSION" == "1+2" ]] && WPA_VERSION=3
   if [[ $USE_PSK -eq 0 ]]; then
     WPA_KEY_TYPE=passphrase
   else
     WPA_KEY_TYPE=psk
   fi
   cat << EOF >> $CONFDIR/hostapd.conf
 wpa=${WPA_VERSION}
 wpa_${WPA_KEY_TYPE}=${PASSPHRASE}
 wpa_key_mgmt=WPA-PSK
-wpa_pairwise=TKIP CCMP
-rsn_pairwise=CCMP
+rsn_pairwise=CCMP
 EOF
 fi

Note: rsn_pairwise=CCMP suffices (WPA2). If WPA1 is explicitly requested by user, conditionally add TKIP then.


503-559: Persistently editing NetworkManager.conf is risky; prefer nmcli runtime control.

Directly appending unmanaged-devices can leave NM permanently misconfigured. Use nmcli device set "$IFACE" managed no (newer NM) and avoid file writes unless explicitly requested.

If NM version supports it, replace writes to NetworkManager.conf with:

  • nmcli dev set "$IFACE" managed no
  • nmcli dev set "$IFACE" managed yes (on cleanup)

Otherwise, isolate the config into a drop-in keyfile under /etc/NetworkManager/conf.d/ (e.g., 99-create_ap-unmanaged.conf) instead of editing the main file, and remove it on cleanup.


465-476: Version detection: use robust parsing and guard against missing nmcli.

nmcli -v output formats vary; current grep may fail. Also guard if nmcli returns non-zero.

Consider:

  • NM_VER=$(nmcli -v 2>/dev/null | sed -nE 's/.version[[:space:]]+([0-9.]+)./\1/p' | head -n1)
  • Fail closed if NM_VER empty.

107-115: Replace which with command -v for POSIX compliance.

which may be absent or an alias; command -v is built-in and reliable.

Example:

  • which nmcli > /dev/null 2>&1 → command -v nmcli >/dev/null 2>&1
  • HOSTAPD=$(which hostapd) → HOSTAPD=$(command -v hostapd || true)

1769-1777: iptables backend compatibility (legacy vs nft).

On modern Debian/Ubuntu, iptables may be nft wrapper; rules could conflict with other managers. Consider detecting backend and offering nftables rules or warn if iptables-nft not present.

  • Detect: update-alternatives --query iptables | sed -n 's/^Value: //p'
  • Optionally support nft via iptables-nft or native nft script.
packages/bsp/ky/usr/local/bin/auto_login_cli.sh (1)

18-29: Clarify the relationship between this script and the static override files.

This script dynamically generates the same override content that already exists as static files in the repository:

  • packages/bsp/ky/usr/lib/systemd/system/getty@.service.d/override.conf
  • packages/bsp/ky/usr/lib/systemd/system/serial-getty@.service.d/override.conf

The static files hardcode user orangepi, while this script allows customization but defaults to root.

Consider:

  • If this script is the canonical way to configure autologin, remove the static override files to avoid confusion
  • If the static files are the defaults, document when/why this script should be used
  • Ensure the script and static files don't conflict during deployment

Additionally, consider using systemd-run or systemctl edit for safer systemd override management rather than manually writing files.

config/bootscripts/boot-ky.cmd (1)

39-39: Document lcd_backlight phandle removal.

The script removes /soc/lcd_backlight phandle from the device tree. This appears to be a hardware-specific workaround. Consider adding a comment explaining why this removal is necessary for the KY platform.

 fdt addr ${fdt_addr_r}
+# Remove lcd_backlight phandle to prevent conflicts with KY hardware configuration
 fdt rm /soc/lcd_backlight phandle
config/boards/orangepirv2.conf (1)

34-38: Remove unnecessary file removal.

Lines 35-37 remove a file that may not exist before creating the directory. The subsequent logic (line 42) will overwrite any existing file anyway for the orangepirv2x variant.

Apply this diff:

 	# Force load wireless module if available
-	if [[ -f "${destination}"/etc/modules-load.d/${BOARD}.conf ]]; then
-		run_host_command_logged rm -f "${destination}"/etc/modules-load.d/${BOARD}.conf
-	fi
 	run_host_command_logged mkdir -pv "${destination}"/etc/modules-load.d
packages/bsp/ky/usr/bin/gadget-setup.sh (10)

524-553: UDC conflict checks: avoid UUoC, quote patterns, and handle missing UDC files.

-  for config_path in "/sys/kernel/config/usb_gadget/"*; do
+  for config_path in /sys/kernel/config/usb_gadget/*; do
     udc_path="$config_path/UDC"
-    is_here=$(cat $udc_path | grep $selected_udc | wc -l)
+    [ -e "$udc_path" ] || continue
+    is_here=$(grep -Fxc "$selected_udc" "$udc_path" || true)
@@
- our_udc_occupied=$(cat /sys/kernel/config/usb_gadget/*/UDC | grep $selected_udc | wc -l)
+ our_udc_occupied=$(grep -hFxc "$selected_udc" /sys/kernel/config/usb_gadget/*/UDC 2>/dev/null | awk '{s+=$1} END{print s+0}')

480-484: Backticks and quoting in rndis_unlink; guard ifname read.

-[ -e $GFUNC_PATH/rndis.0/ifname ] && ifconfig `cat $GFUNC_PATH/rndis.0/ifname` down
-g_remove $GCONFIG/rndis.0
+[ -e "$GFUNC_PATH/rndis.0/ifname" ] && ifconfig "$(cat "$GFUNC_PATH/rndis.0/ifname")" down
+g_remove "$GCONFIG/rndis.0"

4-6: Prefer $(...) over legacy backticks; quote expansion.

-name=`basename $0`
+name="$(basename "$0")"

830-832: Use $EDITOR and quote path when editing config.

-vi $CONFIG_FILE
-[ -e $CONFIG_FILE ] && gadget_info ".usb_config updated"
+"${EDITOR:-vi}" "$CONFIG_FILE"
+[ -e "$CONFIG_FILE" ] && gadget_info ".usb_config updated"

516-522: Message says “none” but script writes empty string. Align log with behavior.

- gadget_info "Echo none to udc"
- gadget_info "We are now trying to echo None to UDC......"
+ gadget_info "Disconnecting UDC (writing empty string)"
+ gadget_info "We are now trying to clear UDC......"
@@
- gadget_info "echo none to UDC successfully done"
- gadget_info "echo none to UDC done."
+ gadget_info "UDC cleared successfully"
+ gadget_info "UDC cleared."

118-126: Quote DEVICE paths and redirections to avoid issues with spaces and globbing.

Illustrative fixes:

- elif [ -b $DEVICE ]; then
+ elif [ -b "$DEVICE" ]; then
@@
-   echo "$DEVICE" > $MSC_DIR/lun.0/file
+   echo "$DEVICE" > "$MSC_DIR/lun.0/file"
- DEVICE_SIZE=$(du -b $DEVICE | cut -f1)
- echo "fd_dev_name=${DEVICE},fd_dev_size=${DEVICE_SIZE}" > $BACKSTORE_DIR/control
+ DEVICE_SIZE="$(du -b "$DEVICE" | cut -f1)"
+ echo "fd_dev_name=${DEVICE},fd_dev_size=${DEVICE_SIZE}" > "$BACKSTORE_DIR/control"

Also applies to: 170-183


792-806: Minor: use $(...) and quote in print_info for robustness.

-echo "Board Model: `tr -d '\000' < /proc/device-tree/model`"
+echo "Board Model: $(tr -d '\000' < /proc/device-tree/model)"
@@
-echo "Available UDCs: `ls  -1 /sys/class/udc/ |  tr '\n' ' '`"
+echo "Available UDCs: $(ls -1 /sys/class/udc/ | tr '\n' ' ')"

100-106: Heavy dd for ramdisk; prefer truncate/fallocate for speed.

- dd if=/dev/zero of=$RAMDISK_PATH/disk.img bs=1M count=1038
+ truncate -s 1G "$RAMDISK_PATH/disk.img" || dd if=/dev/zero of="$RAMDISK_PATH/disk.img" bs=1M count=1024

1-3: Consider strict mode at top for safer bash execution.

 #!/bin/bash
+# set -euo pipefail
+# IFS=$'\n\t'

808-851: Usage/help text is outdated vs. implemented commands. Refresh usage() to list all supported subcommands.

Currently supports: stop, clean, restart, reload, start, pause/disconnect, resume/connect, config, help, info, set_role/aliases, and direct function selection. Please align usage output.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 1cf2083 and 9b43cb9.

⛔ Files ignored due to path filters (2)
  • patch/kernel/ky-current/series is excluded by !patch/**
  • patch/kernel/ky-edge/series is excluded by !patch/**
📒 Files selected for processing (14)
  • config/boards/orangepirv2.conf (1 hunks)
  • config/bootenv/ky.txt (1 hunks)
  • config/bootscripts/boot-ky.cmd (1 hunks)
  • config/sources/families/ky.conf (1 hunks)
  • packages/bsp/ky/etc/initramfs-tools/hooks/add_firmware_to_initrd.sh (1 hunks)
  • packages/bsp/ky/opt/camtest_sensor0_mode0.json (1 hunks)
  • packages/bsp/ky/opt/camtest_sensor2_mode0.json (1 hunks)
  • packages/bsp/ky/setup.sh (1 hunks)
  • packages/bsp/ky/usr/bin/gadget-setup.sh (1 hunks)
  • packages/bsp/ky/usr/lib/systemd/system/getty@.service.d/override.conf (1 hunks)
  • packages/bsp/ky/usr/lib/systemd/system/serial-getty@.service.d/override.conf (1 hunks)
  • packages/bsp/ky/usr/local/bin/auto_login_cli.sh (1 hunks)
  • packages/bsp/ky/usr/local/bin/create_ap (1 hunks)
  • packages/bsp/ky/usr/local/bin/reset_ssh.sh (1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-06-04T23:45:38.860Z
Learnt from: djurny
PR: armbian/build#8272
File: config/bootscripts/boot-mvebu.cmd:182-186
Timestamp: 2025-06-04T23:45:38.860Z
Learning: In config/bootscripts/boot-mvebu.cmd, the `fdtfile` variable is mandatory for booting and is pre-set by U-Boot, but can be overridden via armbianEnv.txt. If `fdtfile` is empty, the subsequent device tree file search logic will eventually fail and trigger the critical error "Cannot find DT!" with proper error handling.

Applied to files:

  • config/bootscripts/boot-ky.cmd
📚 Learning: 2025-09-11T06:12:54.213Z
Learnt from: SuperKali
PR: armbian/build#8609
File: config/boards/nanopi-r76s.conf:5-5
Timestamp: 2025-09-11T06:12:54.213Z
Learning: In the Armbian build system, board family configuration files (like config/sources/families/rk35xx.conf) can inherit kernel branch definitions from common include files (like config/sources/families/include/rockchip64_common.inc). Even if a branch like "edge" is not defined directly in the family conf file, it may be available through the sourced include file.

Applied to files:

  • config/sources/families/ky.conf
📚 Learning: 2025-08-11T12:39:22.861Z
Learnt from: pyavitz
PR: armbian/build#8481
File: config/kernel/linux-spacemit-edge.config:492-494
Timestamp: 2025-08-11T12:39:22.861Z
Learning: The SpacemiT platform requires esos.elf firmware to be embedded in the kernel image for boot. The Armbian build system handles this by copying esos.elf from $SRC/packages/blobs/riscv64/spacemit/ to ${kernel_work_dir}/firmware/ via the custom_kernel_config__spacemit_k1_firmware() function in config/sources/families/spacemit.conf, ensuring CONFIG_EXTRA_FIRMWARE="esos.elf" can successfully embed it during kernel build.

Applied to files:

  • packages/bsp/ky/etc/initramfs-tools/hooks/add_firmware_to_initrd.sh
🧬 Code graph analysis (5)
config/bootscripts/boot-ky.cmd (1)
lib/functions/bsp/armbian-bsp-cli-deb.sh (1)
  • board_side_bsp_cli_postinst_update_uboot_bootscript (331-358)
packages/bsp/ky/usr/local/bin/reset_ssh.sh (1)
extensions/fs-cryptroot-support.sh (2)
  • pre_install_kernel_debs__adjust_dropbear_configuration (36-74)
  • post_umount_final_image__export_private_key (76-84)
config/sources/families/ky.conf (2)
config/sources/vendors/mekotronics/mekotronics-rk3588.hooks.sh (2)
  • post_family_config__meko_use_mainline_uboot (22-42)
  • post_family_config__vendor_uboot_mekotronics (8-16)
lib/functions/configuration/main-config.sh (2)
  • write_config_summary_output_file (488-541)
  • source_family_config_and_arch (543-581)
packages/bsp/ky/usr/bin/gadget-setup.sh (1)
packages/bsp/usb-gadget-network/setup-usbgadget-network.sh (2)
  • setup_usb_network_configfs (9-73)
  • set_usbgadget_ipaddress (75-103)
config/boards/orangepirv2.conf (3)
config/sources/vendors/mekotronics/mekotronics-rk3588.hooks.sh (2)
  • post_family_config__vendor_uboot_mekotronics (8-16)
  • post_family_config__meko_use_mainline_uboot (22-42)
.github/generate_CODEOWNERS.sh (1)
  • generate_for_board (18-68)
lib/functions/configuration/main-config.sh (1)
  • do_extra_configuration (360-486)
🪛 Biome (2.1.2)
packages/bsp/ky/opt/camtest_sensor0_mode0.json

[error] 17-17: Expected a property but instead found '}'.

Expected a property here.

(parse)


[error] 27-27: Expected a property but instead found '}'.

Expected a property here.

(parse)


[error] 28-28: Expected an array, an object, or a literal but instead found ']'.

Expected an array, an object, or a literal here.

(parse)


[error] 49-49: Expected a property but instead found '}'.

Expected a property here.

(parse)


[error] 67-67: Expected a property but instead found '}'.

Expected a property here.

(parse)


[error] 68-68: Expected an array, an object, or a literal but instead found ']'.

Expected an array, an object, or a literal here.

(parse)

packages/bsp/ky/opt/camtest_sensor2_mode0.json

[error] 17-17: Expected a property but instead found '}'.

Expected a property here.

(parse)


[error] 27-27: Expected a property but instead found '}'.

Expected a property here.

(parse)


[error] 28-28: Expected an array, an object, or a literal but instead found ']'.

Expected an array, an object, or a literal here.

(parse)


[error] 49-49: Expected a property but instead found '}'.

Expected a property here.

(parse)


[error] 67-67: Expected a property but instead found '}'.

Expected a property here.

(parse)


[error] 68-68: Expected an array, an object, or a literal but instead found ']'.

Expected an array, an object, or a literal here.

(parse)

🪛 GitHub Actions: Shellcheck - PR #8702 ("RFC: Add OrangePi RV2 support with RISC-V KY platform")
packages/bsp/ky/usr/bin/gadget-setup.sh

[error] 292-292: SC2077 (error): You need spaces around the comparison operator.


[error] 320-320: SC2077 (error): You need spaces around the comparison operator.

🔇 Additional comments (22)
config/bootenv/ky.txt (1)

1-2: LGTM!

The boot environment defaults are reasonable: verbosity level 1 provides informative output without excessive detail, and disabling the boot logo is appropriate for an embedded RISC-V platform.

packages/bsp/ky/setup.sh (1)

1-11: LGTM with minor note.

The SPI flash detection and logging is straightforward. The conditional check safely handles the absence of /dev/mtdblock0.

packages/bsp/ky/usr/lib/systemd/system/getty@.service.d/override.conf (1)

1-5: Address autologin security and document the startup delay

  • This override grants passwordless shell access as user orangepi on the console; acceptable for dev boards but must be documented and gated (e.g. optional/configurable, limited to development images) before any production release.
  • The 10 second ExecStartPre sleep has no inline justification—please verify its necessity (hardware init or race‐condition workaround) and add a comment explaining why.
packages/bsp/ky/usr/local/bin/auto_login_cli.sh (1)

9-16: Logic error: cleanup operations run before -d flag check.

Lines 9-12 remove existing getty overrides unconditionally, but the -d (disable) flag is only checked at line 14. This means partial cleanup always occurs even when the script is invoked without -d.

Reorder to check the flag first:

+if [[ $1 == "-d" ]]; then
+	[[ -d /lib/systemd/system/getty@.service.d/ ]] && rm -rf /lib/systemd/system/getty@.service.d/
+	[[ -f /lib/systemd/system/serial-getty@.service.d/override.conf ]] && rm -f /lib/systemd/system/serial-getty@.service.d/override.conf
+	[[ -d /etc/systemd/system/getty@.service.d/ ]] && rm -rf /etc/systemd/system/getty@.service.d/
+	[[ -f /etc/systemd/system/serial-getty@.service.d/override.conf ]] && rm -f /etc/systemd/system/serial-getty@.service.d/override.conf
+	exit
+fi
+
 [[ -d /lib/systemd/system/getty@.service.d/ ]] && rm -rf /lib/systemd/system/getty@.service.d/
 [[ -f /lib/systemd/system/serial-getty@.service.d/override.conf ]] && rm -f /lib/systemd/system/serial-getty@.service.d/override.conf
 [[ -d /etc/systemd/system/getty@.service.d/ ]] && rm -rf /etc/systemd/system/getty@.service.d/
 [[ -f /etc/systemd/system/serial-getty@.service.d/override.conf ]] && rm -f /etc/systemd/system/serial-getty@.service.d/override.conf
-
-if [[ $1 == "-d" ]]; then
-	exit
-fi

Likely an incorrect or invalid review comment.

config/bootscripts/boot-ky.cmd (8)

1-14: LGTM! Clear defaults and user guidance.

The initial environment setup provides sensible defaults and clearly directs users to the appropriate configuration file.


18-21: Verify unconditional environment file load.

The test -e conditional is commented out, causing the script to always attempt loading orangepiEnv.txt regardless of whether it exists. This will produce error messages if the file is missing. Confirm this is intentional behavior for the KY platform.

If the file should be optional, apply this diff:

-#if test -e ${devtype} ${devnum} ${prefix}orangepiEnv.txt; then
+if test -e ${devtype} ${devnum} ${prefix}orangepiEnv.txt; then
 	load ${devtype} ${devnum} ${load_addr} ${prefix}orangepiEnv.txt
 	env import -t ${load_addr} ${filesize}
-#fi
+fi

23-28: LGTM! Console configuration logic is sound.

The console and earlycon setup correctly handles various boot scenarios. The earlycon=sbi parameter is appropriate for RISC-V platforms.


32-32: LGTM! Docker optimization handling is correct.

Conditional cgroup parameters are appropriate for Docker support.


37-37: Verify fdtfile is set for all boot scenarios.

The script loads the device tree using ${fdtfile} without checking if it's defined. Per board config (orangepirv2.conf line 6), BOOT_FDT_FILE is set to "ky/x1_orangepi-rv2.dtb". Ensure this value is properly propagated to the U-Boot environment as fdtfile during image creation.

Based on learnings.


41-52: LGTM! Overlay application logic is robust.

The overlay loops properly handle failures by setting overlay_error while allowing boot to continue. This is appropriate for optional hardware support.


54-54: LGTM! Boot command is correct.

The booti command properly references the loaded kernel, initrd, and device tree.


30-30: Verify presence of orangepiEnv.txt and rootdev population.

The bootargs line in config/bootscripts/boot-ky.cmd uses ${rootdev} which should come from orangepiEnv.txt, but no scripts were found that create or manage this file. Manually confirm that orangepiEnv.txt is generated and populated with the correct rootdev, or update the setup to ensure this variable is defined.

config/boards/orangepirv2.conf (3)

1-11: LGTM! Board metadata properly configured.

The board configuration follows Armbian conventions. The SRC_CMDLINE parameters appropriately duplicate some boot script settings to support extlinux boot mode. Package list aligns with PR objectives for WiFi, Bluetooth, CAN, and V4L support.


13-23: LGTM! U-Boot configuration hook is well-structured.

The function properly enables SPI flash and filesystem support configs required for the boot scenarios described in the PR objectives. Status displays and logging are appropriate.


44-48: LGTM! WiFi firmware handling is correct.

The conditional copy of AP6256 nvram firmware is properly guarded and logged.

config/sources/families/ky.conf (6)

10-38: LGTM! Family configuration is well-structured.

Architecture, boot, and kernel configurations are properly declared. The use of ${BOOTBRANCH_BOARD:-...} and ${BOOTPATCHDIR:-...} provides good flexibility for overrides. The current and edge branches appropriately share the same kernel source since both target the 6.6 kernel.


40-49: LGTM! Firmware setup includes proper validation.

The function correctly validates kernel_work_dir before attempting filesystem operations. The defensive check for .config existence is appropriate for a kernel configuration hook.


56-69: LGTM! U-Boot write logic handles both eMMC and SD boot.

The function correctly writes to eMMC boot0 partition when available (with proper force_ro handling) and falls back to standard block device layout. The offset calculations align with the bootloader component sizes.


71-78: LGTM! MTD write function properly targets SPI flash partitions.

The function correctly writes bootloader components to their respective MTD partitions as described in the PR objectives for SPI NOR flash boot support. The use of separate mtdblock devices aligns with the specialized bootloader layout mentioned.


80-102: LGTM! BSP extras installation is well-structured.

The function properly sets up KY platform utilities and desktop audio configuration. The PulseAudio setup for dual HDMI/AudioCodec sinks is appropriate for the hardware. Conditional checks and logging are properly used throughout.


51-54: OFFSET value confirmed correct – no changes needed.

OFFSET=30 MiB aligns with other family configs (e.g., imx8m=32 MiB, jethub=126 MiB), providing sufficient bootloader space.

packages/bsp/ky/usr/bin/gadget-setup.sh (1)

472-476: Wrong symlink target for RNDIS; link must be placed inside config directory.

Current ln -s $GFUNC_PATH/rndis.0 $GCONFIG creates a symlink named c.1. It should link to $GCONFIG/rndis.0.

- ln -s $GFUNC_PATH/rndis.0 $GCONFIG
- HOST_ADDR=`cat $GFUNC_PATH/rndis.0/host_addr`
- DEV_ADDR=`cat $GFUNC_PATH/rndis.0/dev_addr`
- IFNAME=`cat $GFUNC_PATH/rndis.0/ifname`
+ ln -s "$GFUNC_PATH/rndis.0" "$GCONFIG/rndis.0"
+ HOST_ADDR="$(cat "$GFUNC_PATH/rndis.0/host_addr")"
+ DEV_ADDR="$(cat "$GFUNC_PATH/rndis.0/dev_addr")"
+ IFNAME="$(cat "$GFUNC_PATH/rndis.0/ifname")"

Likely an incorrect or invalid review comment.

Comment on lines +41 to +42
if [[ "${BOARD}x" == "orangepirv2x" ]]; then
echo "bcmdhd" > "${destination}"/etc/modules-load.d/${BOARD}.conf
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

🧩 Analysis chain

Use consistent logging and clarify board variant naming.

Line 42 writes directly to the file without using run_host_command_logged, inconsistent with the rest of the function. Additionally, verify that the board identifier is indeed "orangepirv2x" for the AP6256 WiFi variant (vs "orangepirv2").

Apply this diff for consistency:

 	if [[ "${BOARD}x" == "orangepirv2x" ]]; then
-		echo "bcmdhd" > "${destination}"/etc/modules-load.d/${BOARD}.conf
+		run_host_command_logged bash -c "echo 'bcmdhd' > '${destination}/etc/modules-load.d/${BOARD}.conf'"

Verify the board name with:


🏁 Script executed:

#!/bin/bash
# Check if orangepirv2x is defined as a separate board or variant
fd -t f "orangepirv2" config/boards/

Length of output: 30


🏁 Script executed:

#!/bin/bash
sed -n '1,200p' config/boards/orangepirv2.conf

Length of output: 2286


Wrap echo in logged command, drop variant check suggestion.

Replace the direct file write to use run_host_command_logged for consistency:

-    echo "bcmdhd" > "${destination}"/etc/modules-load.d/${BOARD}.conf
+    run_host_command_logged bash -c "echo 'bcmdhd' > '${destination}/etc/modules-load.d/${BOARD}.conf'"

The test [[ "${BOARD}x" == "orangepirv2x" ]] correctly checks BOARD=="orangepirv2" via the suffix-hack—no variant rename needed.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if [[ "${BOARD}x" == "orangepirv2x" ]]; then
echo "bcmdhd" > "${destination}"/etc/modules-load.d/${BOARD}.conf
if [[ "${BOARD}x" == "orangepirv2x" ]]; then
run_host_command_logged bash -c "echo 'bcmdhd' > '${destination}/etc/modules-load.d/${BOARD}.conf'"
🤖 Prompt for AI Agents
In config/boards/orangepirv2.conf around lines 41-42, replace the direct echo
redirection that writes "bcmdhd" into
"${destination}/etc/modules-load.d/${BOARD}.conf" with a call to
run_host_command_logged to keep logging/consistency; keep the existing BOARD
check ([[ "${BOARD}x" == "orangepirv2x" ]]) unchanged, and have
run_host_command_logged execute a safe single-write operation (for example using
printf/tee in a single command invocation) to create/overwrite the target file
and surface any errors via the logger.

Comment on lines +1 to +8
#!/bin/sh
# Copy firmware file to initrd
#

mkdir -p "${DESTDIR}"/lib/firmware
cp -rf /lib/firmware/esos.elf "${DESTDIR}"/lib/firmware

exit 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Initramfs hook must use hook-functions and prereqs; avoid raw cp and handle missing firmware.

Current script may silently no-op if DESTDIR is unset and ignores initramfs-tools conventions. Use hook skeleton, source hook-functions, and guard copy.

Apply:

-#!/bin/sh
-# Copy firmware file to initrd
-#
-
-mkdir -p "${DESTDIR}"/lib/firmware
-cp -rf /lib/firmware/esos.elf "${DESTDIR}"/lib/firmware
-
-exit 0
+#!/bin/sh
+set -e
+
+# initramfs-tools hook: copy esos.elf into the initramfs if present
+# so early-boot userspace can load it.
+
+PREREQ=""
+prereqs() { echo "$PREREQ"; }
+case "$1" in
+  prereqs) prereqs; exit 0 ;;
+esac
+
+. /usr/share/initramfs-tools/hook-functions
+
+if [ -f /lib/firmware/esos.elf ]; then
+  mkdir -p "$DESTDIR/lib/firmware"
+  # Use helper to copy into the image; falls back to cp if needed.
+  copy_file /lib/firmware/esos.elf "$DESTDIR/lib/firmware/esos.elf" || \
+    cp -f /lib/firmware/esos.elf "$DESTDIR/lib/firmware/"
+fi
+
+exit 0

Based on learnings.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
#!/bin/sh
# Copy firmware file to initrd
#
mkdir -p "${DESTDIR}"/lib/firmware
cp -rf /lib/firmware/esos.elf "${DESTDIR}"/lib/firmware
exit 0
#!/bin/sh
set -e
# initramfs-tools hook: copy esos.elf into the initramfs if present
# so early-boot userspace can load it.
PREREQ=""
prereqs() { echo "$PREREQ"; }
case "$1" in
prereqs) prereqs; exit 0 ;;
esac
. /usr/share/initramfs-tools/hook-functions
if [ -f /lib/firmware/esos.elf ]; then
mkdir -p "$DESTDIR/lib/firmware"
# Use helper to copy into the image; falls back to cp if needed.
copy_file /lib/firmware/esos.elf "$DESTDIR/lib/firmware/esos.elf" || \
cp -f /lib/firmware/esos.elf "$DESTDIR/lib/firmware/"
fi
exit 0

Comment on lines +18 to +28
{
"name": "cpp1",
"enable": 0,
"format":"NV12",
"src_from_file": 0,

"src_path":"/vendor/etc/camera/",
"size_width":1920,
"size_height":1080,
},
],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Remove trailing comma after last element in cpp_node (JSON parse error).

Mirror the fix from sensor2 config here as well.

                 "size_height":1080,
-                },
+                }
         ],
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
{
"name": "cpp1",
"enable": 0,
"format":"NV12",
"src_from_file": 0,
"src_path":"/vendor/etc/camera/",
"size_width":1920,
"size_height":1080,
},
],
{
"name": "cpp1",
"enable": 0,
"format":"NV12",
"src_from_file": 0,
"src_path":"/vendor/etc/camera/",
"size_width":1920,
"size_height":1080
}
],
🧰 Tools
🪛 Biome (2.1.2)

[error] 27-27: Expected a property but instead found '}'.

Expected a property here.

(parse)


[error] 28-28: Expected an array, an object, or a literal but instead found ']'.

Expected an array, an object, or a literal here.

(parse)

🤖 Prompt for AI Agents
In packages/bsp/ky/opt/camtest_sensor0_mode0.json around lines 18 to 28, the
last object in the cpp_node array has a trailing comma which causes a JSON parse
error; remove the trailing comma after the closing brace of the last element
(mirror the same change made in the sensor2 config) so the array ends with "}"
followed immediately by the closing bracket and no extra comma.

Comment on lines +50 to +68
{
"name": "isp1",
"enable": 0,
"work_mode":"offline_capture",
"format":"NV12",
"out_width":1600,
"out_height":1200,

"src_file":"/tmp/1920x1080_raw12_long_packed.vrf",
"bit_depth": 12,
"in_width":1920,
"in_height":1080,

"sensor_name":"gc2375h_spm",
"sensor_id" : 1,
"sensor_work_mode":0,
"fps":30,
},
]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Remove trailing comma after last element in isp_node (JSON parse error).

Mirror the fix here.

                         "fps":30,
-                },
+                }
         ]
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
{
"name": "isp1",
"enable": 0,
"work_mode":"offline_capture",
"format":"NV12",
"out_width":1600,
"out_height":1200,
"src_file":"/tmp/1920x1080_raw12_long_packed.vrf",
"bit_depth": 12,
"in_width":1920,
"in_height":1080,
"sensor_name":"gc2375h_spm",
"sensor_id" : 1,
"sensor_work_mode":0,
"fps":30,
},
]
{
"name": "isp1",
"enable": 0,
"work_mode":"offline_capture",
"format":"NV12",
"out_width":1600,
"out_height":1200,
"src_file":"/tmp/1920x1080_raw12_long_packed.vrf",
"bit_depth": 12,
"in_width":1920,
"in_height":1080,
"sensor_name":"gc2375h_spm",
"sensor_id" : 1,
"sensor_work_mode":0,
"fps":30,
}
]
🧰 Tools
🪛 Biome (2.1.2)

[error] 67-67: Expected a property but instead found '}'.

Expected a property here.

(parse)


[error] 68-68: Expected an array, an object, or a literal but instead found ']'.

Expected an array, an object, or a literal here.

(parse)

🤖 Prompt for AI Agents
In packages/bsp/ky/opt/camtest_sensor0_mode0.json around lines 50 to 68, there
is a trailing comma after the last property/object in the isp_node array (the
"fps":30 entry and the comma after the object), which causes a JSON parse error;
remove the trailing comma after the final property/object so the last object
ends without a comma and the array closes properly.

Comment on lines +1 to +69
{
"tuning_server_enable":1,
"show_fps":1,
"auto_run": 1,

"cpp_node": [
{
"name": "cpp0",
"enable": 1,
"format":"NV12",
"src_from_file": 1,

"src_path":"/tmp/cpp_case_in_data/1920x1080/",
"size_width":1920,
"size_height":1080,

},
{
"name": "cpp1",
"enable": 0,
"format":"NV12",
"src_from_file": 0,

"src_path":"/vendor/etc/camera/",
"size_width":1920,
"size_height":1080,
},
],

"isp_node":[
{
"name": "isp0",
"enable": 1,
"work_mode":"online",
"format":"NV12",
"out_width":1920,
"out_height":1080,

"sensor_name":"ov13855_spm",
"sensor_id" : 2,
"sensor_work_mode":0,
"fps":30,

"src_file":"/tmp/1920x1080_raw12_long_packed.vrf",
"bit_depth": 12,
"in_width":1920,
"in_height":1080,

},
{
"name": "isp1",
"enable": 0,
"work_mode":"offline_capture",
"format":"NV12",
"out_width":1600,
"out_height":1200,

"src_file":"/tmp/1920x1080_raw12_long_packed.vrf",
"bit_depth": 12,
"in_width":1920,
"in_height":1080,

"sensor_name":"gc2375h_spm",
"sensor_id" : 1,
"sensor_work_mode":0,
"fps":30,
},
]
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Quick validation guard for CI/local.

Add a lightweight check to prevent regressions.


🏁 Script executed:

#!/bin/bash
# Validate KY camtest JSON files parse cleanly
set -euo pipefail
for f in packages/bsp/ky/opt/camtest_sensor{0,2}_mode0.json; do
  echo "Validating $f"
  jq -e . "$f" >/dev/null
  # Optional sanity checks
  jq -e '.cpp_node | length==2 and .isp_node | length==2' "$f" >/dev/null
done
echo "All good."

Length of output: 124


Fix invalid JSON in camtest_sensor0_mode0.json and add CI validation

• Remove trailing commas inside objects and arrays in packages/bsp/ky/opt/camtest_sensor0_mode0.json (e.g., the comma after "size_height":1080 on line 17) so the file parses as valid JSON.
• Once corrected, integrate and run the suggested jq-based sanity-check script in CI to guard against future regressions.

🧰 Tools
🪛 Biome (2.1.2)

[error] 17-17: Expected a property but instead found '}'.

Expected a property here.

(parse)


[error] 27-27: Expected a property but instead found '}'.

Expected a property here.

(parse)


[error] 28-28: Expected an array, an object, or a literal but instead found ']'.

Expected an array, an object, or a literal here.

(parse)


[error] 49-49: Expected a property but instead found '}'.

Expected a property here.

(parse)


[error] 67-67: Expected a property but instead found '}'.

Expected a property here.

(parse)


[error] 68-68: Expected an array, an object, or a literal but instead found ']'.

Expected an array, an object, or a literal here.

(parse)

🤖 Prompt for AI Agents
In packages/bsp/ky/opt/camtest_sensor2_mode0.json around lines 1 to 69, the JSON
contains trailing commas inside objects/arrays (e.g., after "size_height":1080
and other entries) which makes the file invalid; remove those trailing commas so
each object/array adheres to strict JSON (no comma after the last property or
last array element) and re-validate the file with a JSON parser; then add the
suggested jq-based sanity-check to CI (a step that runs a JSON parse/validation
against all *.json config files) to prevent future regressions.

Comment on lines +679 to +684
[ -e $CONFIG_FILE ] || die "$CONFIG_FILE not found, abort."
while read line
do
select_one $line
done < $CONFIG_FILE
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Robust config parsing: quote, read -r, skip blanks and leading-# comments.

-parse_config()
-{
-  [ -e  $CONFIG_FILE ] || die "$CONFIG_FILE not found, abort."
-  while read line
-  do
-    select_one $line
-  done < $CONFIG_FILE
-}
+parse_config() {
+  [ -e "$CONFIG_FILE" ] || die "$CONFIG_FILE not found, abort."
+  while IFS= read -r line; do
+    # skip empty or commented lines (allow leading spaces)
+    case "$line" in
+      ''|[[:space:]]|'#'*|[[:space:]]'#'*) continue ;;
+    esac
+    select_one "$line"
+  done < "$CONFIG_FILE"
+}

Comment on lines +688 to +692
gconfig
glink
[ $FUNCTION_CNT -lt 1 ] && die "No function selected, will not pullup."
echo_udc $1
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

UDC index argument not wired; resume/start pass $2 but echo_udc only reads USB_UDC_IDX env.

Export the env var and stop passing unused arg to echo_udc.

-gstart()
-{
+gstart() {
   gconfig
   glink
   [ $FUNCTION_CNT -lt 1 ] && die "No function selected, will not pullup."
-  echo_udc $1
+  echo_udc
 }
-start)
-  parse_config
-  gstart $2
+start)
+  parse_config
+  [ -n "$2" ] && export USB_UDC_IDX="$2"
+  gstart
-resume|connect)
-  USBDEV_IDX=$2
-  echo_udc
+resume|connect)
+  [ -n "$2" ] && export USB_UDC_IDX="$2"
+  echo_udc

Also applies to: 818-821, 825-828

🤖 Prompt for AI Agents
In packages/bsp/ky/usr/bin/gadget-setup.sh around lines 688-692 (also apply same
change at 818-821 and 825-828): the UDC index passed as $2 to resume/start is
never used because echo_udc reads USB_UDC_IDX from the environment; instead of
passing the unused positional argument, export USB_UDC_IDX="$2" (or set it from
the caller) before calling resume/start so the function reads the correct index,
and remove the unused argument from the echo_udc calls so callers only pass
needed parameters.

Comment on lines +3 to +7
if [[ -z $1 ]]; then
user=root
else
user=$1
fi
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Defaulting to root autologin is a critical security issue.

When no username is provided, the script defaults to root, granting passwordless superuser access on the console. This is more dangerous than the hardcoded orangepi user in the static override files.

Change the default to a less privileged user or require an explicit username:

 if [[ -z $1 ]]; then
-	user=root
+	echo "Error: username required" >&2
+	echo "Usage: $0 <username>" >&2
+	exit 1
 else
 	user=$1
 fi

Or align with the static overrides:

-	user=root
+	user=orangepi
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if [[ -z $1 ]]; then
user=root
else
user=$1
fi
if [[ -z $1 ]]; then
echo "Error: username required" >&2
echo "Usage: $0 <username>" >&2
exit 1
else
user=$1
fi
🤖 Prompt for AI Agents
In packages/bsp/ky/usr/local/bin/auto_login_cli.sh around lines 3 to 7, the
script currently defaults the username to "root" when no argument is provided
which creates a security risk; update the logic to either require an explicit
username (exit with an error if $1 is empty) or default to a non‑privileged user
used by static overrides (e.g., "orangepi" or another designated unprivileged
account) and document the change in a comment; ensure any callers of this script
are updated or validated to pass the correct username.

Comment on lines +703 to +707
for x in $CONFDIR/*.pid; do
# even if the $CONFDIR is empty, the for loop will assign
# a value in $x. so we need to check if the value is a file
[[ -f $x ]] && kill -9 $(cat $x)
done
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Avoid unconditional kill -9; terminate gracefully and then escalate.

SIGKILL prevents cleanup and may kill PIDs reused by the system if stale files exist.

-    [[ -f $x ]] && kill -9 $(cat $x)
+    if [[ -f $x ]]; then
+        pid="$(cat "$x")"
+        kill "$pid" 2>/dev/null || true
+        # wait a moment; escalate if still alive
+        for i in 1 2 3; do
+            kill -0 "$pid" 2>/dev/null || break
+            sleep 1
+        done
+        kill -9 "$pid" 2>/dev/null || true
+    fi
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
for x in $CONFDIR/*.pid; do
# even if the $CONFDIR is empty, the for loop will assign
# a value in $x. so we need to check if the value is a file
[[ -f $x ]] && kill -9 $(cat $x)
done
for x in $CONFDIR/*.pid; do
# even if the $CONFDIR is empty, the for loop will assign
# a value in $x. so we need to check if the value is a file
if [[ -f $x ]]; then
pid="$(cat "$x")"
# try graceful termination first
kill "$pid" 2>/dev/null || true
# wait a moment; escalate if still alive
for i in 1 2 3; do
kill -0 "$pid" 2>/dev/null || break
sleep 1
done
# force kill if still running
kill -9 "$pid" 2>/dev/null || true
fi
done
🤖 Prompt for AI Agents
In packages/bsp/ky/usr/local/bin/create_ap around lines 703–707, the loop
unconditionally uses kill -9 on PIDs read from *.pid files which prevents
graceful shutdown and risks killing unrelated processes if a stale PID file
exists; change the logic to: validate the PID is a positive integer and that the
process exists, send SIGTERM first, wait briefly in a loop checking if the PID
exits (use kill -0 to probe), and only if the process still exists after a
timeout send SIGKILL; also handle and remove stale/non-numeric pid files and log
failures rather than blindly issuing kill -9.

Comment on lines +1 to +4
#!/bin/bash

sudo rm /etc/ssh/ssh_host_*
sudo dpkg-reconfigure openssh-server
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Make SSH reset non-interactive and root-safe (avoid sudo, add safeguards).

Running sudo inside a script can block; dpkg-reconfigure may prompt; add safety flags and explicit restart. Also guard against missing keys and glob mishaps.

Apply:

-#!/bin/bash
-
-sudo rm /etc/ssh/ssh_host_*
-sudo dpkg-reconfigure openssh-server
+#!/bin/bash
+set -euo pipefail
+
+# Re-exec with sudo only if not root; avoid interactive prompt when possible.
+if [[ $EUID -ne 0 ]]; then
+  exec sudo -n "$0" "$@"
+fi
+
+# Remove existing host keys if any (handle no-match safely)
+shopt -s nullglob
+keys=(/etc/ssh/ssh_host_*_key /etc/ssh/ssh_host_*_key.pub)
+if (( ${#keys[@]} > 0 )); then
+  rm -f "${keys[@]}"
+fi
+
+# Regenerate keys non-interactively and restart ssh
+export DEBIAN_FRONTEND=noninteractive
+dpkg-reconfigure openssh-server
+systemctl try-restart ssh || true
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
#!/bin/bash
sudo rm /etc/ssh/ssh_host_*
sudo dpkg-reconfigure openssh-server
#!/bin/bash
set -euo pipefail
# Re-exec with sudo only if root; avoid interactive prompt when possible.
if [[ $EUID -ne 0 ]]; then
exec sudo -n "$0" "$@"
fi
# Remove existing host keys if any (handle no-match safely)
shopt -s nullglob
keys=(/etc/ssh/ssh_host_*_key /etc/ssh/ssh_host_*_key.pub)
if (( ${#keys[@]} > 0 )); then
rm -f "${keys[@]}"
fi
# Regenerate keys non-interactively and restart ssh
export DEBIAN_FRONTEND=noninteractive
dpkg-reconfigure openssh-server
systemctl try-restart ssh || true

declare -g LINUXCONFIG="linux-${LINUXFAMILY}-current"
;;
edge)
declare -g KERNELSOURCE="https://github.com/orangepi-xunlong/linux-orangepi.git"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't make sense when they're identical.

@EvilOlaf
Copy link
Member

EvilOlaf commented Oct 2, 2025

I'm no expert in riscv but from my guts feel it should be tried to merge this into existing riscv64 family if possible and do necessary adjustments on board level.

@tmshlvck
Copy link
Author

tmshlvck commented Oct 2, 2025

@EvilOlaf Yes, it seems ky.conf family has been forked from config/sources/families/spacemit.conf .

TBH I have no insight into Spacemit - Ky relationship and therefore this RFC PR is based on assumption that this SoC is indeed different from Spacemit K1 and therefore merits its own family. The public speculations go in both ways AFAIK:

If there is an argument for integrating these two together I am fine with that and will try to do the work.

@SteeManMI
Copy link
Contributor

I'm no expert in riscv but from my guts feel it should be tried to merge this into existing riscv64 family if possible and do necessary adjustments on board level.

In general we try to minimize the number of family's (i.e. the number of distinct kernels we have to build and support) as they take a significant amount of both system and human resources ongoing.
But it is always a balancing act, we could simply have a distinct family for every board, but then the maintenance of all those kernel patches across nearly identical boards would be taxing, but this provides ultimate flexibility. The other extreme is that we could try for one universal kernel that supports all archtectures. In the past there have been efforts to have one kernel that uspports all amlogic, allwinner and rockchip 64 bit cpus in one kernel. Obviously this has a lot of benefits as far as maintenance and system resources goes, but comes with the drawback of risk of breaking some random board for every patch applied. So the general strategy is to minimize the number of families but realize that there are good reasons to have different families at times.
Neither I or @EvilOlaf have familiarity with your cpu, so I can only provide general guidance to you in this matter.

@igorpecovnik
Copy link
Member

igorpecovnik commented Oct 6, 2025

indeed different from Spacemit K1 and therefore merits its own family

New kernel family represent stress and long term burden. In most cases the difference between vendor kernels are minor and we only take patches from one and base on top of the other. In our case, Banana f3 SpaceMit kernel. Also boot scripts - we already have tons of them - there is initiative to get that in order as its going out of our hands (maintaining wise). Those are important aspects and should be taken care of rather then going the easy way. I understand its a bit more work for you and i hope you do understand - thank you for the time you invested so far.

There is no rush - replace with existing files and configs, one at the time.

Also we probably don't want to include scripts such as create AP, a copy from https://github.com/oblique/create_ap

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

11 Milestone: Fourth quarter release BSP Board Support Packages Hardware Hardware related like kernel, U-Boot, ... Needs review Seeking for review Patches Patches related to kernel, U-Boot, ... size/large PR with 250 lines or more

Development

Successfully merging this pull request may close these issues.

4 participants