| Message ID | 20240517123909.680686-1-leon@georgemail.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [1/2] net: add option to ignore 'local-mac-address' property | expand |
On Fri, May 17, 2024 at 02:39:07PM +0200, Leon M. Busch-George wrote: > From: "Leon M. Busch-George" <leon@georgemail.eu> > > Here is the definition of a mac that looks like its address would be > loaded from nvmem: > > mac@0 { > compatible = "mediatek,eth-mac"; > reg = <0>; > phy-mode = "2500base-x"; > phy-handle = <&rtl8221b_phy>; > > nvmem-cell-names = "mac-address"; > nvmem-cells = <&macaddr_bdinfo_de00 1>; /* this is ignored */ > }; > > Because the boot program inserts a 'local-mac-address', which is preferred > over other detection methods, the definition using nvmem is ignored. > By itself, that is only a mild annoyance when dealing with device trees. > After all, the 'local-mac-address' property exists primarily to pass MAC > addresses to the kernel. > > But it is also possible for this address to be randomly generated (on each > boot), which turns an annoyance into a hindrance. In such a case, it is no > longer possible to set the correct address from the device tree. This > behaviour has been observed on two types of MT7981B devices from different > vendors (Cudy M3000, Yuncore AX835). > > Restore the ability to set addresses through the device tree by adding an > option to ignore the 'local-mac-address' property. I'm not convinced you are fixing the right thing here. To me, this is the bootloader which is broken. You should be fixing the bootloader. One concession might be, does the bootloader correctly generate a random MAC address? i.e. does it have the locally administered bit set? If that bit is set, and there are other sources of a MAC address, then it seems worth while checking them to see if there is a better MAC address available, which as global scope. Andrew
On Fri, May 17, 2024 at 04:07:08PM +0200, Andrew Lunn wrote: > On Fri, May 17, 2024 at 02:39:07PM +0200, Leon M. Busch-George wrote: > > From: "Leon M. Busch-George" <leon@georgemail.eu> > > > > Here is the definition of a mac that looks like its address would be > > loaded from nvmem: > > > > mac@0 { > > compatible = "mediatek,eth-mac"; > > reg = <0>; > > phy-mode = "2500base-x"; > > phy-handle = <&rtl8221b_phy>; > > > > nvmem-cell-names = "mac-address"; > > nvmem-cells = <&macaddr_bdinfo_de00 1>; /* this is ignored */ > > }; > > > > Because the boot program inserts a 'local-mac-address', which is preferred > > over other detection methods, the definition using nvmem is ignored. > > By itself, that is only a mild annoyance when dealing with device trees. > > After all, the 'local-mac-address' property exists primarily to pass MAC > > addresses to the kernel. > > > > But it is also possible for this address to be randomly generated (on each > > boot), which turns an annoyance into a hindrance. In such a case, it is no > > longer possible to set the correct address from the device tree. This > > behaviour has been observed on two types of MT7981B devices from different > > vendors (Cudy M3000, Yuncore AX835). > > > > Restore the ability to set addresses through the device tree by adding an > > option to ignore the 'local-mac-address' property. > > I'm not convinced you are fixing the right thing here. > > To me, this is the bootloader which is broken. You should be fixing > the bootloader. IMO this is firmly in the "setting software policy" category of properties and is therefore not really welcome. If we can patch the DT provided to the kernel with this property, how come the bootloader cannot be changed to stop patching the random MAC address in the first place? > One concession might be, does the bootloader correctly generate a > random MAC address? i.e. does it have the locally administered bit > set? If that bit is set, and there are other sources of a MAC address, > then it seems worth while checking them to see if there is a better > MAC address available, which as global scope.
Hi :-) On Fri, 17 May 2024 17:13:18 +0100 Conor Dooley <conor@kernel.org> wrote: > On Fri, May 17, 2024 at 04:07:08PM +0200, Andrew Lunn wrote: > > On Fri, May 17, 2024 at 02:39:07PM +0200, Leon M. Busch-George > > wrote: > > > > > Restore the ability to set addresses through the device tree by > > > adding an option to ignore the 'local-mac-address' property. > > > > I'm not convinced you are fixing the right thing here. > > > > To me, this is the bootloader which is broken. You should be fixing > > the bootloader. > > IMO this is firmly in the "setting software policy" category of > properties and is therefore not really welcome. > If we can patch the DT provided to the kernel with this property, how > come the bootloader cannot be changed to stop patching the random MAC > address in the first place? .. and .. On Fri, May 17, 2024 at 04:07:08PM +0200, Andrew Lunn wrote: > I'm not convinced you are fixing the right thing here. > > To me, this is the bootloader which is broken. You should be fixing > the bootloader. I agree changing the bootloader is the better approach and I'm absolutely willing to accept if this isn't the way of the kernel. Also, since posting this, I was made aware that it's possible to remove the 'ethernet0' alias to stop the unwanted activity. There's no longer much reason for me to work on this. There's only that slight annoyance of configuring a mac address assignment on the device tree and the kernel silently ignoring it. But, I guess, that doesn't happen if the proprietary bootloader isn't creating "local-mac-address" properties - rather than only changing existing ones (which is what mainline U-Boot does). On altering/replacing the bootloader: It is not always possible or feasible to replace proprietary bootloaders on proprietary hardware. Many of the routers I work with effectively become bricks if the bootloader doesn't work. If it has one of these chunky DIP SPI NORs, then might be possible to restore using the right hardware but the two devices mentioned in the commit both have QFP NAND that I cannot read without the help of software running on the board. > One concession might be, does the bootloader correctly generate a > random MAC address? i.e. does it have the locally administered bit > set? If that bit is set, and there are other sources of a MAC > address, then it seems worth while checking them to see if there is > a better MAC address available, which as global scope. I like that idea! All the addresses that were generated on a few reboots for testing have it set. Let's hope we wont get a reason to implement that too soon :-D kind regards, Leon
diff --git a/net/core/of_net.c b/net/core/of_net.c index 93ea425b9248..4cf4171bc7fe 100644 --- a/net/core/of_net.c +++ b/net/core/of_net.c @@ -104,11 +104,11 @@ EXPORT_SYMBOL(of_get_mac_address_nvmem); * * Search the device tree for the best MAC address to use. 'mac-address' is * checked first, because that is supposed to contain to "most recent" MAC - * address. If that isn't set, then 'local-mac-address' is checked next, - * because that is the default address. If that isn't set, then the obsolete - * 'address' is checked, just in case we're using an old device tree. If any - * of the above isn't set, then try to get MAC address from nvmem cell named - * 'mac-address'. + * address. If there is no valid 'mac-address' and `ignore-local-mac-address' + * isn't set, then 'local-mac-address' is checked next, because that is the + * default address. If that isn't set, then the obsolete * 'address' is + * checked, just in case we're using an old device tree. If any of the above + * isn't set, then try to get MAC address from nvmem cell named 'mac-address'. * * Note that the 'address' property is supposed to contain a virtual address of * the register set, but some DTS files have redefined that property to be the @@ -134,9 +134,11 @@ int of_get_mac_address(struct device_node *np, u8 *addr) if (!ret) return 0; - ret = of_get_mac_addr(np, "local-mac-address", addr); - if (!ret) - return 0; + if (!of_find_property(np, "ignore-local-mac-address", NULL)) { + ret = of_get_mac_addr(np, "local-mac-address", addr); + if (!ret) + return 0; + } ret = of_get_mac_addr(np, "address", addr); if (!ret) diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c index 049c3adeb850..8f4efba90d96 100644 --- a/net/ethernet/eth.c +++ b/net/ethernet/eth.c @@ -582,9 +582,10 @@ static int fwnode_get_mac_addr(struct fwnode_handle *fwnode, * * Search the firmware node for the best MAC address to use. 'mac-address' is * checked first, because that is supposed to contain to "most recent" MAC - * address. If that isn't set, then 'local-mac-address' is checked next, - * because that is the default address. If that isn't set, then the obsolete - * 'address' is checked, just in case we're using an old device tree. + * address. If there is no valid 'mac-address' and `ignore-local-mac-address' + * isn't set, then 'local-mac-address' is checked next, because that is the + * default address. If that isn't set, then the obsolete 'address' is checked, + * just in case we're using an old device tree. * * Note that the 'address' property is supposed to contain a virtual address of * the register set, but some DTS files have redefined that property to be the @@ -600,7 +601,8 @@ static int fwnode_get_mac_addr(struct fwnode_handle *fwnode, int fwnode_get_mac_address(struct fwnode_handle *fwnode, char *addr) { if (!fwnode_get_mac_addr(fwnode, "mac-address", addr) || - !fwnode_get_mac_addr(fwnode, "local-mac-address", addr) || + (!fwnode_property_present(fwnode, "ignore-local-mac-address") && + !fwnode_get_mac_addr(fwnode, "local-mac-address", addr)) || !fwnode_get_mac_addr(fwnode, "address", addr)) return 0;