net: use ethaddr= bootarg before random MAC fallback#2168
Conversation
|
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 layerThe current CI run is red on ~40+ boards (every HiSilicon
Would it be possible to move the femac patch into the Goke vendor BSP package (e.g. alongside the rest of femac under Scope of
|
widgetii
left a comment
There was a problem hiding this comment.
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.
…llow space for intermediate patches.
|
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 issuesIt 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
Fleet-collision footgunGreat 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 nitsAll 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. |
|
The ethernet drivers vary quite a lot about how they fetch a mac address and default back to a random address. Patching and maintaining them all seems like a tall task. How would you feel about directly patching ipconfig.c but only enabling it when the kernel feature (CONFIG_OPENIPC_ETHADDR) is enabled? Something like this: Then in the top-level Makefile we could enable CONFIG_OPENIPC_ETHADDR only if BR2_PACKAGE_OPENIPC_NFS_ROOT is enabled (which contains the userspace overlay changes). |
|
Re-reviewed after the recent commits — really nice work, the restructure is exactly along the lines I'd hoped. What's improved
CI red count is misleadingI sampled six failing jobs ( A re-run once the upstream tarball hashes are refreshed should turn most of those reds green without further code changes. Remaining small nits (none blocking)
Happy to flip my |
Structural concerns from the original review are addressed by the recent commits — patch layering, scope, and the openipc_ethaddr.c cleanups all look good (see follow-up comment for details). Dismissing the block. CI still needs to come back green before merge: the current matrix has ~36 reds, but on inspection those are upstream tarball hash drift (wireguard-linux-compat, wireguard-tools, squashfs), not anything in this PR. A re-run once the source hashes are refreshed should clear them.
|
Thank you for the review @widgetii. I've changed tack to what I think might be a better approach. Rather than having global patches to linux that apply to every build and touch every ethernet driver I moved the patchset to a buildroot package and finished plumbing in buildroot support for linux kernel extensions from packages. The feature is described here: https://buildroot.org/downloads/manual/adding-packages-linux-kernel-spec-infra.txt. This approach has a few benefits:
Cons:
Notes:
Current branch is here: master...tinylabs:openipc-firmware:br-package-nfs-root Let me know your thoughts. I'd like to close this PR and open a new one with the other branch. |
|
Thanks for thinking this through again — I think you've landed on the right design. Patching
The noisy log (driver assigns random MAC → ipconfig overwrites) is a fair tradeoff for the architectural simplification; a single Happy with closing this PR and opening a fresh one against the
Looking forward to reviewing the new one. Thanks for the persistence here — this is going to be a much nicer thing to maintain than the original direction. |
|
Thanks @widgetii! I'll create a new PR after I finish polishing and testing the branch. |
Summary
Allow
ethaddr=from bootargs to seed the MAC address during early network init instead of falling back to a random MAC.Why
ethaddr=is already present on the command line.Changes
ethaddr=viaarch_get_platform_mac_address().eth_platform_get_mac_address()before random fallback.