Conversation
The iptables patch only added new xtables extension files (libxt_*.c) and UAPI headers — it didn't modify any existing iptables source. Since iptables loads extensions dynamically from /usr/lib/<triplet>/xtables/ at runtime, we can ship these as standalone .so files without patching or holding the iptables package. Changes: - Compile the 4 xtables extensions in chroot against libxtables-dev - Install .so files to /usr/lib/<triplet>/xtables/ - Bundle .so files into the gateway-dk-ask .deb - Remove iptables from rebuild_patched_deb calls - Remove iptables from apt-mark hold (postinst, build-time, postrm) - Remove iptables from patch_dirs in prepare_ask_patches - Remove iptables from apt-get build-dep - Pin ASK repo to commit with new iptables-extensions/ dir Users can now freely upgrade iptables without breaking ASK offloading. The libnetfilter-conntrack and libnfnetlink patches remain — those modify library internals and cannot be decoupled.
The board has a pcf2131 I2C RTC. Installing util-linux-extra provides hwclock plus the udev rule that automatically runs 'hwclock --hctosys' when /dev/rtc0 appears at boot — no custom script or service needed. Also remove fake-hwclock which is installed by default on Armbian. It saves system time to a file on shutdown and restores it on boot, which interferes with real RTC sync. Same pattern used by helios64 and other boards with battery-backed RTCs.
The board has a power-hold circuit controlled by GPIO3_21 (DT label &gpio2, pin 21): driving it high cuts power. Add a gpio-poweroff node and enable the driver so 'systemctl poweroff' actually turns the device off instead of halting the CPU in an undefined state. - DTS: gpio-poweroff node on &gpio2 21 active-high, 1s timeout - Kernel: CONFIG_POWER_RESET_GPIO=y (must be builtin, not module)
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a Mono Gateway DK device tree and DTB with gpio-poweroff and reworked SFP LED layout, enables GPIO power-reset in the kernel, replaces Changes
Sequence Diagram(s)sequenceDiagram
participant Bootloader
participant Kernel
participant DT as DeviceTree(mono-gateway-dk.dtb)
participant Drivers
participant Userspace
Bootloader->>Kernel: load kernel + DTB
Kernel->>DT: parse nodes (gpio-poweroff, sfp-led-controller, RTC, etc.)
Kernel->>Drivers: probe devices per DT (gpio-poweroff, leds, RTC, PHYs)
Drivers->>Kernel: register devices (poweroff handler, led controller, hwclock)
Kernel->>Userspace: expose /dev/rtc, hwmon, net interfaces
Userspace->>Kernel: util-linux hwclock, lm-sensors, fancontrol, networking start
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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
05
|
Apologies for the PR spam, I'm new here :) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
patch/kernel/archive/ls1046a-6.12/002-arm64-dts-Add-Mono-Gateway-DK-device-tree.patch (2)
603-651:⚠️ Potential issue | 🟠 MajorRemove the invalid
partition@flashentry.The fixed-partitions schema requires partition node names matching the pattern
@[0-9a-f]+$(hexadecimal digits only).partition@flashis invalid because "flash" contains non-hexadecimal characters ('l' and 's'). Additionally, this entry spans the entire flash range (0x0–0x4000000), overlapping all sibling partitions and creating layout ambiguity. The parent NOR MTD device already provides whole-chip access.🧩 Minimal fix
partitions { compatible = "fixed-partitions"; `#address-cells` = <1>; `#size-cells` = <1>; - - partition@flash { - label = "flash"; - reg = <0x0 0x4000000>; - }; partition@0 { label = "rcw-bl2"; reg = <0x0 0x100000>; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/ls1046a-6.12/002-arm64-dts-Add-Mono-Gateway-DK-device-tree.patch` around lines 603 - 651, Remove the invalid partition node named partition@flash under the partitions fixed-partitions node: delete the entire partition@flash block (label = "flash"; reg = <0x0 0x4000000>;), since node names must match the @[0-9a-f]+ pattern and this entry overlaps all other partition entries; rely on the parent NOR MTD device for whole-chip access and keep the remaining partition@0, partition@100000, partition@300000, etc. entries unchanged.
100-119:⚠️ Potential issue | 🟠 MajorRemove
ledsproperty fromsff,sfpnodes—it violates the binding schema.The
sff,sfpbinding (Documentation/devicetree/bindings/net/sff,sfp.yaml) defines onlycompatible,i2c-bus,maximum-power-milliwatt, GPIO properties, and pinctrl properties, withadditionalProperties: false. Theledsentries at lines 107 and 118 will fail schema validation.The patch already includes the correct pattern: a separate
sfp_led_controllernode withcompatible = "mono,sfp-led". Move the LED references there or into theleds { compatible = "gpio-leds"; }section instead of extending the SFP nodes directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/ls1046a-6.12/002-arm64-dts-Add-Mono-Gateway-DK-device-tree.patch` around lines 100 - 119, The sff,sfp nodes sfp_xfi0 and sfp_xfi1 include an invalid "leds" property that violates the sff,sfp binding; remove the leds = <&led_sfp0_link>, <&led_sfp0_activity> and leds = <&led_sfp1_link>, <&led_sfp1_activity> from those nodes and instead reference or instantiate those LED phandles under the existing sfp_led_controller (or under a gpio-leds node) so the LED topology is declared outside the sff,sfp nodes; update sfp_led_controller to include the led phandles (led_sfp0_link, led_sfp0_activity, led_sfp1_link, led_sfp1_activity) or add them to the gpio-leds node accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/kernel/linux-ls1046a-current.config`:
- Line 446: The config enables CONFIG_POWER_RESET_GPIO but omits its required
parent CONFIG_POWER_RESET; update the kernel config to also enable
CONFIG_POWER_RESET so the gpio-poweroff DTS node becomes functional—ensure both
CONFIG_POWER_RESET and CONFIG_POWER_RESET_GPIO are set to y (mirror the pattern
used for other boards) by adding CONFIG_POWER_RESET=y alongside the existing
CONFIG_POWER_RESET_GPIO=y.
In `@extensions/gateway-dk-ask.sh`:
- Around line 189-194: The package installs iptables into the chroot via
chroot_sdcard_apt_get_install (so xtables plugins will require the iptables
binary) but the generated Debian control stanza doesn't list iptables in
Depends; update the control-stanza generation code that builds the package
Depends for this ASK extension to include "iptables" (or a suitable versioned
dependency) so standalone dpkg -i/OTA installs pull in iptables when the
extension that installs xtables plugins is packaged.
---
Outside diff comments:
In
`@patch/kernel/archive/ls1046a-6.12/002-arm64-dts-Add-Mono-Gateway-DK-device-tree.patch`:
- Around line 603-651: Remove the invalid partition node named partition@flash
under the partitions fixed-partitions node: delete the entire partition@flash
block (label = "flash"; reg = <0x0 0x4000000>;), since node names must match the
@[0-9a-f]+ pattern and this entry overlaps all other partition entries; rely on
the parent NOR MTD device for whole-chip access and keep the remaining
partition@0, partition@100000, partition@300000, etc. entries unchanged.
- Around line 100-119: The sff,sfp nodes sfp_xfi0 and sfp_xfi1 include an
invalid "leds" property that violates the sff,sfp binding; remove the leds =
<&led_sfp0_link>, <&led_sfp0_activity> and leds = <&led_sfp1_link>,
<&led_sfp1_activity> from those nodes and instead reference or instantiate those
LED phandles under the existing sfp_led_controller (or under a gpio-leds node)
so the LED topology is declared outside the sff,sfp nodes; update
sfp_led_controller to include the led phandles (led_sfp0_link,
led_sfp0_activity, led_sfp1_link, led_sfp1_activity) or add them to the
gpio-leds node accordingly.
🪄 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: f9198605-73b6-46ba-9508-327b6542a846
📒 Files selected for processing (4)
config/boards/gateway-dk.confconfig/kernel/linux-ls1046a-current.configextensions/gateway-dk-ask.shpatch/kernel/archive/ls1046a-6.12/002-arm64-dts-Add-Mono-Gateway-DK-device-tree.patch
| CONFIG_GPIO_MPC8XXX=y | ||
| CONFIG_GPIO_SYSCON=y | ||
| CONFIG_GPIO_PCA953X=y | ||
| CONFIG_POWER_RESET_GPIO=y |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if we have the DTS patch mentioned in the review
find . -name "*002-arm64-dts-Add-Mono-Gateway-DK-device-tree.patch" -o -name "*ls1046a*" | head -20Repository: armbian/build
Length of output: 394
🏁 Script executed:
# Check the current config file to understand the context
find . -name "linux-ls1046a-current.config" -type fRepository: armbian/build
Length of output: 101
🏁 Script executed:
# Search for POWER_RESET in any existing config files
rg "CONFIG_POWER_RESET" --type txtRepository: armbian/build
Length of output: 39
🏁 Script executed:
# Search for references to ARCH_LAYERSCAPE in the repo
rg "ARCH_LAYERSCAPE" --type txtRepository: armbian/build
Length of output: 39
🏁 Script executed:
# Check the DTS patch to see what it adds
cat -n patch/kernel/archive/ls1046a-6.12/002-arm64-dts-Add-Mono-Gateway-DK-device-tree.patch | sed -n '60,70p'Repository: armbian/build
Length of output: 338
🏁 Script executed:
# Check the config file around line 446
cat -n config/kernel/linux-ls1046a-current.config | sed -n '440,455p'Repository: armbian/build
Length of output: 554
🏁 Script executed:
# Look for any other references to POWER_RESET in patches or configs
rg "POWER_RESET" --type-list | head -5Repository: armbian/build
Length of output: 446
🏁 Script executed:
# Search in all files, not just txt
rg "POWER_RESET" 2>/dev/null | head -20Repository: armbian/build
Length of output: 2643
🌐 Web query:
Linux 6.12 kernel POWER_RESET Kconfig POWER_RESET_GPIO dependency
💡 Result:
In Linux kernel 6.12, CONFIG_POWER_RESET_GPIO is the Kconfig option for the GPIO power-off driver (drivers/power/reset/gpio-poweroff.c), which toggles a GPIO pin to power off the board. It is located under the "Board level reset or power off" menu in drivers/power/reset/Kconfig. It depends on CONFIG_POWER_RESET (the menuconfig enabling the board-level poweroff/reset drivers) and CONFIG_OF_GPIO (for Device Tree GPIO support). Sources indicate this dependency structure has been consistent from kernels 3.8 through 6.19. Note: There is a separate CONFIG_POWER_RESET_GPIO_RESTART for GPIO-based restart (gpio-restart.c), and RESET_GPIO in drivers/reset/Kconfig (depends on GPIOLIB) for the generic reset controller framework, which is distinct from poweroff.
Citations:
- 1: https://elixir.bootlin.com/linux/v6.12/source/kernel/power/Kconfig
- 2: https://github.com/torvalds/linux/blob/master/drivers/reset/Kconfig
- 3: https://www.kernelconfig.io/CONFIG_POWER_RESET
- 4: https://cateee.net/lkddb/web-lkddb/POWER_RESET_GPIO.html
- 5: https://github.com/torvalds/linux/tree/master/drivers/power/reset/Kconfig
- 6: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/power/reset
- 7: https://github.com/torvalds/linux/blob/master/drivers/power/reset/gpio-poweroff.c
Add CONFIG_POWER_RESET=y to enable the gpio-poweroff functionality.
CONFIG_POWER_RESET_GPIO depends on the parent CONFIG_POWER_RESET option in Linux 6.12. The DTS patch adds a gpio-poweroff device node, but without the parent symbol enabled, this node will be non-functional. This follows the established pattern in Armbian across other boards (filogic-6.16, sunxi kernels) where both symbols are explicitly enabled together.
🔧 Suggested config change
+CONFIG_POWER_RESET=y
CONFIG_POWER_RESET_GPIO=y🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/kernel/linux-ls1046a-current.config` at line 446, The config enables
CONFIG_POWER_RESET_GPIO but omits its required parent CONFIG_POWER_RESET; update
the kernel config to also enable CONFIG_POWER_RESET so the gpio-poweroff DTS
node becomes functional—ensure both CONFIG_POWER_RESET and
CONFIG_POWER_RESET_GPIO are set to y (mirror the pattern used for other boards)
by adding CONFIG_POWER_RESET=y alongside the existing CONFIG_POWER_RESET_GPIO=y.
There was a problem hiding this comment.
@tomazzaman can you run ./compile.sh rewrite-kernel-config BOARD=gateway-dk BRANCH=current and check if the added config persists? Else the suggestion from coderabbit might need to be applied.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
xtables extensions (QOSMARK/QOSCONNMARK) shipped in this package are loaded by iptables at runtime. Add iptables to dpkg Depends so OTA installs pull it in automatically.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
extensions/gateway-dk-ask.sh (1)
274-287: Use pkg-config to derive libxtables build flags instead of hardcoding them.The current gcc line (
-D_init=\${name}_init -I./include) relies on bundled headers and misses the-lxtableslibrary link, despite libxtables-dev being installed in the chroot. The recommended approach—as documented by libxtables-dev—is to usepkg-config --cflags --libs xtables, which ensures the shared object links correctly and respects the build-time ABI metadata. Update the gcc command to incorporate$(pkg-config --cflags --libs xtables)and remove the local-I./include, following the same pattern already used elsewhere in the build for libnetfilter-conntrack (e.g., in the CMM build step).The unversioned
Depends: iptablesis acceptable for Debian ABI compatibility, as libxtables maintains backward compatibility within major versions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/gateway-dk-ask.sh` around lines 274 - 287, The xtables build step in the chroot (inside the chroot_sdcard block that builds libxt_*.c) is hardcoding -I./include and missing libxtables link flags; replace the local include flag with a pkg-config invocation so gcc uses libxtables ABI flags: invoke pkg-config --cflags --libs xtables inside the same chrooted command and add its output to the gcc command (keep the existing -D_init=\${name}_init and -shared -fPIC -O2 flags), removing -I./include so the shared objects link correctly; update the chrooted loop around building libxt_*.c accordingly and preserve the surrounding install and exit_with_error behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@extensions/gateway-dk-ask.sh`:
- Around line 274-287: The build installs all libxt_*.so artifacts into
/usr/lib/${ASK_HOST_TRIPLET}/xtables while the packaging path later copies only
an explicit subset (qos/QOS), causing divergence; fix by introducing and using a
single explicit allowlist (e.g., XTABLES_ALLOWLIST) of expected extension
basenames and use that list both in the chroot build step (replace the glob
install -m 644 libxt_*.so with iterating over XTABLES_ALLOWLIST and installing
each named .so, failing if missing) and in the packaging/copy path (use the same
XTABLES_ALLOWLIST and error out if any expected file is absent) so builds and
.deb packaging remain identical; reference symbols to change: ASK_CACHE_DIR,
chroot_sdcard invocation that runs gcc and install, the install target
/usr/lib/${ASK_HOST_TRIPLET}/xtables, and the packaging copy logic that
currently handles qos/QOS matches.
---
Nitpick comments:
In `@extensions/gateway-dk-ask.sh`:
- Around line 274-287: The xtables build step in the chroot (inside the
chroot_sdcard block that builds libxt_*.c) is hardcoding -I./include and missing
libxtables link flags; replace the local include flag with a pkg-config
invocation so gcc uses libxtables ABI flags: invoke pkg-config --cflags --libs
xtables inside the same chrooted command and add its output to the gcc command
(keep the existing -D_init=\${name}_init and -shared -fPIC -O2 flags), removing
-I./include so the shared objects link correctly; update the chrooted loop
around building libxt_*.c accordingly and preserve the surrounding install and
exit_with_error behavior.
🪄 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: a6f9b4b5-43ed-486a-81e3-1f7948f199e4
📒 Files selected for processing (1)
extensions/gateway-dk-ask.sh
Replace the libxt_*.c glob in the build step and libxt_qos*/libxt_QOS* glob in the packaging step with a single explicit list of module names. Both steps now reference the same ask_xtables_modules array, so adding or removing a module requires updating only one place per function. The packaging step now errors out if any expected .so is missing instead of silently skipping it. Also document why we don't use pkg-config for libxtables: these are dlopen()-loaded extensions that don't link against libxtables.so and use our local UAPI headers (xt_QOSMARK.h etc.) which aren't in libxtables-dev.
Update ASK commit to include the modernized iptables extensions that use the x6 xtables API (.x6_parse, .x6_options, .x6_fcheck) instead of the legacy API (.parse, .extra_opts). The legacy API doesn't work with iptables 1.8.11 on Debian Trixie.
The xtables header (xtables.h) has: #ifdef _INIT # define _init __attribute__((constructor)) _INIT #endif When -D_INIT=libxt_<name>_init (uppercase) is defined, the header chains _init → __attribute__((constructor)) _INIT → libxt_<name>_init. The constructor attribute makes dlopen() automatically call the init function, which registers the target with xtables_register_target(). Our build was using -D_init (lowercase) which directly renamed the symbol but bypassed the header's #ifdef _INIT block, so the constructor attribute was never applied. The extension loaded but _init was never called, leaving options unregistered. This matches how iptables itself builds extensions (Makefile.am uses -D_INIT=lib$*_init).
The xtables.h _init/_INIT macro chain proved unreliable — the _init macro wasn't being applied despite _INIT being defined. The ASK repo now uses __attribute__((constructor)) directly in the extension source, which is the modern pattern and doesn't need any -D flags. Pin ASK to commit with the constructor fix.
No need to apologise :) |
|
I'm also addressing other concerns CodeRabbit brought up. Will push shortly. |
The sff,sfp DT binding has additionalProperties: false, so our custom leds property on SFP nodes violates the schema. Move LED-to-SFP association into per-port child nodes of the sfp_led_controller. DTS: sfp_led_controller now has port@0/port@1 sub-nodes, each with sfp phandle and leds list. SFP nodes no longer carry leds property. Driver: sfp_led_probe() iterates child nodes instead of sfp-ports phandle array. sfp_led_register_port() receives the port child node, parses the sfp phandle from it, and reads LEDs from the child node instead of from the SFP node.
|
I'd also like some guidance on the kernel config. Since we patch the file dynamically, it gets left in a dirty state after image build. Is it okay if I commit these? They will get discarded if unused anyway. |
|
External sources is asking for compilers compatibility issues :( They work on todays Debian compiler, while it already fails with Ubuntu. https://paste.armbian.com/begodatojo.bash (After enabling Ubuntu src package https://paste.armbian.com/ekesawotik.diff) Now how much sense and additional work will be to make sources future (compilers) proof? At least for next generation. |
|
Alternatively - we can forget about Ubuntu builds and ship only Debian. As it is now: https://www.armbian.com/boards/gateway-dk |
|
I'll sort out Ubuntu as well, I don't see why it wouldn't work. Haven't gotten to it yet though. |
3 participants
Description
Three small, independent improvements to the Mono Gateway DK board support:
1. Decouple QOSMARK iptables extensions from iptables package
The ASK extension previously patched the
iptablessource package to add QOSMARK/QOSCONNMARK xtables extensions, which required holdingiptablesto prevent upgrades from overwriting them. This turned out to be unnecessary - the patch only added new files (4libxt_*.c+ 4 UAPI headers), no modifications to existing iptables source.The extensions are now compiled as standalone
.sofiles againstlibxtables-devheaders and installed to/usr/lib/aarch64-linux-gnu/xtables/, where iptables loads them dynamically at runtime. They're bundled into thegateway-dk-ask.deb.Result:
iptablescan now be upgraded freely viaapt upgrade. The extensions survive iptables upgrades because they're owned bygateway-dk-ask, notiptables.libnetfilter-conntrack3andlibnfnetlink0remain patched + held - those patches modify library internals and cannot be decoupled. These are rarely updated (every couple of years), so having them held indefinitely is not an issue.2. RTC sync via util-linux-extra (pcf2131)
The board has a PCF2131 I2C RTC, but the default
fake-hwclockpackage interfered with proper RTC handling. Installutil-linux-extra(provideshwclock+ udev rule for automatic RTC→system sync at boot) and removefake-hwclock. Same pattern as Helios64 and JetHub boards.Result: system clock is seeded from the real RTC at boot without any custom script or service.
3. GPIO-based power off
The board has a power-hold circuit controlled by GPIO3_21 (DT
&gpio2pin 21): driving it high cuts power. Add agpio-poweroffdevice tree node and enableCONFIG_POWER_RESET_GPIO=ysosystemctl poweroffactually turns the device off instead of halting the CPU in an undefined state.How Has This Been Tested?
CLEAN_LEVEL=make-kernel,images,debsgateway-dk-ask.deb, owned by that packageapt-mark showholdshows onlylibnetfilter-conntrack3andlibnfnetlink0iptables -j QOSMARK/-j QOSCONNMARKrules work on targetsystemctl poweroffcuts powerChecklist:
Summary by CodeRabbit
New Features
Infrastructure