[PATCH 1/2] net: add option to ignore 'local-mac-address' property

[PATCH 1/2] net: add option to ignore 'local-mac-address' property

[Leon M. Busch-George]

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.

Signed-off-by: Leon M. Busch-George <leon@georgemail.eu>
---
 net/core/of_net.c  | 18 ++++++++++--------
 net/ethernet/eth.c | 10 ++++++----
 2 files changed, 16 insertions(+), 12 deletions(-)

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;
 
-- 
2.44.0

[PATCH 2/2] dt-bindings: net: add option to ignore local-mac-address

[Leon M. Busch-George]

From: "Leon M. Busch-George" <leon@georgemail.eu>

Add the option 'ignore-local-mac-address' to allow ignoring bogus
addresses. This restores the ability to correctly set the MAC address from
the device tree if a boot program stores a randomly generated address in
'local-mac-address'.

Signed-off-by: Leon M. Busch-George <leon@georgemail.eu>
---
 .../devicetree/bindings/net/ethernet-controller.yaml        | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
index b2785b03139f..67b8437febdf 100644
--- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
+++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
@@ -23,6 +23,12 @@ properties:
     minItems: 6
     maxItems: 6
 
+  ignore-local-mac-address:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      Indicates that 'local-mac-address' is to be ignored. This can
+      be useful when the boot program stores a bogus address.
+
   mac-address:
     description:
       Specifies the MAC address that was last used by the boot
-- 
2.44.0

Re: [PATCH 1/2] net: add option to ignore 'local-mac-address' property

[Andrew Lunn]

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

Re: [PATCH 1/2] net: add option to ignore 'local-mac-address' property

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2214 bytes --]

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.




[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/2] net: add option to ignore 'local-mac-address' property

[Leon Busch-George]

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