On Thu, Jul 19, 2018 at 12:06:51PM +0200, Bartosz Golaszewski wrote:

quoted

It looks simpler as long as you don't add all the new routines
resulting from this approach. I've just tried to quickly implement
this solution and it resulted in much bigger and duplicated code
(checking the validity of mac_addr, copying it etc.). I would prefer
the current approach and would like to read someone else's opinion on
that.

It's not too bad.  There is two extra ether_addr_copy() calls and one
extra is_valid_ether_addr().  There are two extra alen declarations and
one extra addr declaration.  The functions don't share *that* much code.

regards,
dan carpenter

diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index fd8faa0dfa61..8ab7289a5069 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c

@@ -54,6 +54,8 @@
 #include <linux/if_ether.h>
 #include <linux/of_net.h>
 #include <linux/pci.h>
+#include <linux/nvmem-consumer.h>
+#include <linux/mtd/mtd.h>
 #include <net/dst.h>
 #include <net/arp.h>
 #include <net/sock.h>

@@ -525,7 +527,7 @@ unsigned char * __weak arch_get_platform_mac_address(void)
        return NULL;
 }

-int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr)
+static int of_eth_get_mac_address(struct device *dev, u8 *mac_addr)
 {
        const unsigned char *addr;
        struct device_node *dp;

@@ -547,4 +549,82 @@ int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr)
        ether_addr_copy(mac_addr, addr);
        return 0;
 }
+
+static int nvmem_eth_get_mac_address(struct device *dev, u8 *mac_addr)
+{
+       struct nvmem_cell *nvmem;
+       unsigned char *addr;
+       size_t alen;
+       int ret = 0;
+
+       nvmem = nvmem_cell_get(dev, "mac-address");
+       if (PTR_ERR(nvmem) == -EPROBE_DEFER)
+               /* We may have a lookup registered for MAC address but the
+                * corresponding nvmem provider hasn't been registered yet.
+                */
+               return -EPROBE_DEFER;
+       if (IS_ERR(nvmem))
+               return PTR_ERR(nvmem);
+       addr = nvmem_cell_read(nvmem, &alen);
+       if (IS_ERR(addr)) {
+               ret = PTR_ERR(addr);
+               goto put_nvmem;
+       }
+       if (alen != ETH_ALEN || !is_valid_ether_addr(addr)) {
+               ret = -EINVAL;
+               goto free_addr;
+       }
+       ether_addr_copy(mac_addr, addr);
+free_addr:
+       kfree(addr);
+put_nvmem:
+       nvmem_cell_put(nvmem);
+       return ret;
+}
+
+static int mtd_eth_get_mac_address(struct device *dev, u8 *mac_addr)
+{
+       struct mtd_info *mtd;
+       u8 addrbuf[ETH_ALEN];
+       size_t alen;
+       int ret;
+
+       /* This function should go away as soon as MTD gets nvmem support. */
+       if (!IS_ENABLED(CONFIG_MTD))
+               return -ENODEV;
+
+       mtd = get_mtd_device_nm("MAC-Address");
+       if (IS_ERR(mtd))
+               return PTR_ERR(mtd);
+       ret = mtd_read(mtd, 0, ETH_ALEN, &alen, addrbuf);
+       if (ret)
+               goto put_mtd;
+       if (!is_valid_ether_addr(addrbuf)) {
+               ret = -EINVAL;
+               goto put_mtd;
+       }
+       ether_addr_copy(mac_addr, addrbuf);
+put_mtd:
+       put_mtd_device(mtd);
+       return ret;
+}
+
+int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr)
+{
+       int ret;
+
+       ret = of_eth_get_mac_address(dev, mac_addr);
+       if (!ret)
+               return 0;
+       ret = nvmem_eth_get_mac_address(dev, mac_addr);
+       if (ret == -EPROBE_DEFER)
+               return -EPROBE_DEFER;
+       if (!ret)
+               return 0;
+       ret = mtd_eth_get_mac_address(dev, mac_addr);
+       if (!ret)
+               return 0;
+
+       return -ENODEV;
+}
 EXPORT_SYMBOL(eth_platform_get_mac_address);