Skip to content

net: use ethaddr= bootarg before random MAC fallback#2168

Open
tinylabs wants to merge 3 commits into
OpenIPC:masterfrom
tinylabs:kernel-ethaddr-bootargs
Open

net: use ethaddr= bootarg before random MAC fallback#2168
tinylabs wants to merge 3 commits into
OpenIPC:masterfrom
tinylabs:kernel-ethaddr-bootargs

Conversation

@tinylabs
Copy link
Copy Markdown

@tinylabs tinylabs commented Jun 3, 2026

Summary

Allow ethaddr= from bootargs to seed the MAC address during early network init instead of falling back to a random MAC.

Why

  • Needed for early boot cases like NFS root, useful when booting multiple cameras with NFS with different kernels/rootfs.
  • Avoids random MAC assignment when a valid ethaddr= is already present on the command line.

Changes

  • Added an OpenIPC kernel helper to expose ethaddr= via arch_get_platform_mac_address().
  • Updated femac eth driver to use eth_platform_get_mac_address() before random fallback.
  • Enabled the helper through the kernel config fragment.

@widgetii
Copy link
Copy Markdown
Member

widgetii commented Jun 4, 2026

Thanks for sending this — the use case (deterministic MACs for NFS-rooted lab cameras) is genuinely useful, and the early-param plumbing is clean. A few things to consider before this is ready to merge, mostly around build-time scope.

CI: looks like patch 0016 is in the wrong layer

The current CI run is red on ~40+ boards (every HiSilicon _lite, all SigmaStar, Fullhan, Rockchip, GM, and gk7205v500_lite), while the older Goke family and the *_neo HiSilicon kernels pass. The pattern matches the patchability of 0016-femac-use-platform-mac-fallback.patch:

  • That patch edits drivers/net/ethernet/goke/femac/femac.c, which only exists in the legacy Goke vendor BSP (gk7102/gk7202/gk7205v2xx/gk7605v100).
  • general/package/all-patches/linux/ is the global BR2_GLOBAL_PATCH_DIR, so the patch is applied to every kernel build. On any tree without that file (HiSilicon, SigmaStar, Rockchip, gk7205v500 which is HiSilicon-derived, etc.), patch -p1 fails and the kernel build aborts.

Would it be possible to move the femac patch into the Goke vendor BSP package (e.g. alongside the rest of femac under br-ext-chip-goke/...) so it only applies where the file exists? That alone should turn the matrix mostly green. The global all-patches dir today only carries tree-wide gcc-version shims (0010–0014), which feels like the right convention to preserve.

Scope of CONFIG_OPENIPC_ETHADDR

Right now the symbol is enabled globally via the new kernel fragment, but only the Goke femac driver actually consumes arch_get_platform_mac_address(). On every other vendor's ethernet driver (HiSilicon higmac/femac, SigmaStar GMAC, Rockchip stmmac, …) the random-MAC fallback path is untouched, so the new mechanism is effectively a no-op there.

Two ways to resolve it:

  • Narrow the config gate to the platforms it actually serves (depend on the Goke femac symbol), so the help text matches reality, or
  • Add parallel one-liner patches per vendor driver in the same PR so the feature really is board-wide.

Behavior question on the Goke side

The current diff drops the explicit of_get_mac_address(node) lookup and goes straight to eth_platform_get_mac_address(dev, ndev->dev_addr). On modern mainline that helper does consult OF/DT internally, but on the Goke vendor kernel the femac platform node's fwnode wiring is a bit unusual — could you confirm on a real Goke board that a DT-provided mac-address / local-mac-address is still honored when no ethaddr= is passed? Just want to make sure we're not silently regressing the DT path for users who already have valid MACs in DT/u-boot fixup.

Fleet-collision footgun with the universal u-boot

One thing worth thinking about: OpenIPC's universal u-boot ships ethaddr=00:00:23:34:45:66 baked into its default env. Today that's harmless because the kernel re-randomizes when DT/EEPROM give nothing useful. With this PR, every unprovisioned camera would deterministically come up as 00:00:23:34:45:66 — two on the same L2 segment would ARP-war.

Would you be open to rejecting that specific sentinel inside openipc_early_ethaddr() (treat it like "not set" and fall through to random)? Or at minimum calling it out in the Kconfig help text so deployers know to clear the u-boot default env first.

Smaller nits

  • ndev->dev_addr is const u8 * from Linux 5.15 on, so eth_platform_get_mac_address(dev, ndev->dev_addr) would stop compiling if/when goke's kernel gets bumped. Cheap to future-proof: read into a stack buffer, then eth_hw_addr_set(ndev, mac).
  • openipc_ethaddr_valid is redundant with is_valid_ether_addr() on the buffer — you could drop the flag and write return is_valid_ether_addr(openipc_ethaddr) ? openipc_ethaddr : NULL;.
  • A pr_warn on a malformed ethaddr= string would help users notice typos — right now a bad value silently falls through to random with no breadcrumbs.
  • BR2_LINUX_KERNEL_CONFIG_FRAGMENT_FILES=… (plain assignment) in general/openipc.fragment would clobber any per-board kernel fragment set in the future. Today no board sets it, so it's fine, but += is the safer convention.
  • Worth a quick grep of each target kernel for an existing arch_get_platform_mac_address definition — a couple of vendor BSPs out there carry their own, and you'd get a multiple-definition link error if one of ours does.

Verification suggestion

Once the patch is in the right layer, the smallest useful smoke test would be:

  1. Pick one Goke board (e.g. gk7205v200_lite) and one HiSilicon board (once HiSilicon driver wiring is in).
  2. Boot with ethaddr=02:11:22:33:44:55 on the kernel cmdline; confirm ip link show dev eth0 reflects it.
  3. Boot again with no ethaddr=; confirm DT-provided MAC (if any) is still honored, then random fallback.

Thanks again — happy to re-review as soon as CI is green.

Copy link
Copy Markdown
Member

@widgetii widgetii left a comment

Choose a reason for hiding this comment

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

Requesting changes — CI is red across ~40+ boards because patch 0016 lands in the global BR2_GLOBAL_PATCH_DIR but only applies cleanly to legacy Goke kernels. Detailed notes in the inline comment above; happy to re-review once the femac patch is moved into the Goke vendor BSP layer and CI is green.

@tinylabs
Copy link
Copy Markdown
Author

tinylabs commented Jun 4, 2026

Thank you for the full analysis @widgetii! I indeed missed a bunch of things.

For context this kernel change is part of a larger change set to make root NFS deployment easier. For my security cam deployment that's my preferred method but I understand it may be of marginal use to others outside the lab. All the other changes for NFS root I've been able to integrate into a buildroot package but unfortunately kernel support for a fixed MAC on boot is blocking me. Ultimately I'd like to be able to append BR2_PACKAGE_OPENIPC_NFS_ROOT=y to general/openipc.fragment and deploy NFS root for any target. I'd be happy to update the wiki article when complete to help other deploy this way if there's interest.

Scope issues

It would be nice to have this functionality global in scope so it doesn't have to be added adhoc to each camera later. I've started to refactor, only keeping the definition arch_get_platform_mac_address global with a copy for neo and non-neo targets. The ethernet drivers are now patched based on what kernel driver is found in the kernel config with the patches applied appropriately by appending patch paths to BR2_GLOBAL_PATCH_DIR. This prevents having to touch each defconfig per target. Currently I have goke and hisilicon (both legacy and neo) targets compiling with the patch.

Validation needed

  • I only have gk7205v300 target currently so that's all I can test on hardware. I have some sigmastar infinity 6c targets coming soon so I will test those as well.
  • I need to double check arch_get_platform_mac_address isn't getting defined for any other kernel to avoid that duplicate definition error.
  • I will try setting the device tree MAC value to ensure that we don't override that when set.

Fleet-collision footgun

Great point I didn't consider. Maybe it would be better to use another bootarg key like macaddr rather than ethaddr. That would prevent the collisions. Alternatively, although a bit hacky, we could check for the default MAC in arch_get_platform_mac_address and treat it like a NULL value so it falls through to reassigning a random MAC.

Smaller nits

All valid, I will fix those. Only one I'm not sure how to approach is the general/openipc.fragment. I don't think buildroot supports append operations in these files.

Again, I appreciate all the feedback. Once I get things in better looking shape we can discuss if it's a good fit for a merge.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants