diff mbox

[v2,2/3] net: add support for nvmem to eth_platform_get_mac_address()

Message ID 20180719082028.26116-3-brgl@bgdev.pl (mailing list archive)
State New, archived
Headers show

Commit Message

Bartosz Golaszewski July 19, 2018, 8:20 a.m. UTC
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Many non-DT platforms read the MAC address from EEPROM. Usually it's
either done with callbacks defined in board files or from SoC-specific
ethernet drivers.

In order to generalize this, try to read the MAC from nvmem in
eth_platform_get_mac_address() using a standard lookup name:
"mac-address".

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 net/ethernet/eth.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

Comments

Dan Carpenter July 19, 2018, 8:48 a.m. UTC | #1
On Thu, Jul 19, 2018 at 10:20:27AM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Many non-DT platforms read the MAC address from EEPROM. Usually it's
> either done with callbacks defined in board files or from SoC-specific
> ethernet drivers.
> 
> In order to generalize this, try to read the MAC from nvmem in
> eth_platform_get_mac_address() using a standard lookup name:
> "mac-address".
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  net/ethernet/eth.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
> index 39af03894598..af3b4b1b77eb 100644
> --- a/net/ethernet/eth.c
> +++ b/net/ethernet/eth.c
> @@ -54,6 +54,7 @@
>  #include <linux/if_ether.h>
>  #include <linux/of_net.h>
>  #include <linux/pci.h>
> +#include <linux/nvmem-consumer.h>
>  #include <net/dst.h>
>  #include <net/arp.h>
>  #include <net/sock.h>
> @@ -527,8 +528,11 @@ unsigned char * __weak arch_get_platform_mac_address(void)
>  
>  int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr)
>  {
> +	unsigned char addrbuf[ETH_ALEN];
>  	const unsigned char *addr;
> +	struct nvmem_cell *nvmem;
>  	struct device_node *dp;
> +	size_t alen;
>  
>  	if (dev_is_pci(dev))
>  		dp = pci_device_to_OF_node(to_pci_dev(dev));
> @@ -541,6 +545,29 @@ int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr)
>  	if (!addr)
>  		addr = arch_get_platform_mac_address();
>  
> +	if (!addr) {
> +		nvmem = nvmem_cell_get(dev, "mac-address");
> +		if (IS_ERR(nvmem) && 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)) {
> +			addr = nvmem_cell_read(nvmem, &alen);
> +			if (!IS_ERR(addr)) {
                                    ^^^^
Never do success handling.  Always error handling.  Otherwise the code
is indent a lot and the error handling is far from the call.

> +				if (alen == ETH_ALEN)
> +					ether_addr_copy(addrbuf, addr);
> +
> +				kfree(addr);
> +				addr = alen == ETH_ALEN ? addrbuf : NULL;
> +			}
> +
> +			nvmem_cell_put(nvmem);
> +		}
> +	}
> +
>  	if (!addr || !is_valid_ether_addr(addr))
                                          ^^^^
Instead of handling the error we dereference the error pointer here.

*frowny face*

>  		return -ENODEV;
>  
> --

Maybe this?

	if (!addr) {
		nvmem = nvmem_cell_get(dev, "mac-address");
		if (PTR_ERR(nvmem) == -EPROBE_DEFER)
			return -EPROBE_DEFER;
		if (IS_ERR(nvmem))
			return -ENODEV;
		addr = nvmem_cell_read(nvmem, &alen);
		if (IS_ERR(addr))
			return PTR_ERR(addr);
		if (alen != ETH_ALEN) {
			kfree(addr);
			return -ENODEV;
		}
		ether_addr_copy(addrbuf, addr);
		kfree(addr);
		addr = addrbuf;
	}
	if (!is_valid_ether_addr(addr))
		return -ENODEV;
	ether_addr_copy(mac_addr, addr);
	return 0;

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bartosz Golaszewski July 19, 2018, 8:57 a.m. UTC | #2
2018-07-19 10:48 GMT+02:00 Dan Carpenter <dan.carpenter@oracle.com>:
> On Thu, Jul 19, 2018 at 10:20:27AM +0200, Bartosz Golaszewski wrote:
>> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>>
>> Many non-DT platforms read the MAC address from EEPROM. Usually it's
>> either done with callbacks defined in board files or from SoC-specific
>> ethernet drivers.
>>
>> In order to generalize this, try to read the MAC from nvmem in
>> eth_platform_get_mac_address() using a standard lookup name:
>> "mac-address".
>>
>> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>> ---
>>  net/ethernet/eth.c | 27 +++++++++++++++++++++++++++
>>  1 file changed, 27 insertions(+)
>>
>> diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
>> index 39af03894598..af3b4b1b77eb 100644
>> --- a/net/ethernet/eth.c
>> +++ b/net/ethernet/eth.c
>> @@ -54,6 +54,7 @@
>>  #include <linux/if_ether.h>
>>  #include <linux/of_net.h>
>>  #include <linux/pci.h>
>> +#include <linux/nvmem-consumer.h>
>>  #include <net/dst.h>
>>  #include <net/arp.h>
>>  #include <net/sock.h>
>> @@ -527,8 +528,11 @@ unsigned char * __weak arch_get_platform_mac_address(void)
>>
>>  int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr)
>>  {
>> +     unsigned char addrbuf[ETH_ALEN];
>>       const unsigned char *addr;
>> +     struct nvmem_cell *nvmem;
>>       struct device_node *dp;
>> +     size_t alen;
>>
>>       if (dev_is_pci(dev))
>>               dp = pci_device_to_OF_node(to_pci_dev(dev));
>> @@ -541,6 +545,29 @@ int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr)
>>       if (!addr)
>>               addr = arch_get_platform_mac_address();
>>
>> +     if (!addr) {
>> +             nvmem = nvmem_cell_get(dev, "mac-address");
>> +             if (IS_ERR(nvmem) && 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)) {
>> +                     addr = nvmem_cell_read(nvmem, &alen);
>> +                     if (!IS_ERR(addr)) {
>                                     ^^^^
> Never do success handling.  Always error handling.  Otherwise the code
> is indent a lot and the error handling is far from the call.
>
>> +                             if (alen == ETH_ALEN)
>> +                                     ether_addr_copy(addrbuf, addr);
>> +
>> +                             kfree(addr);
>> +                             addr = alen == ETH_ALEN ? addrbuf : NULL;
>> +                     }
>> +
>> +                     nvmem_cell_put(nvmem);
>> +             }
>> +     }
>> +
>>       if (!addr || !is_valid_ether_addr(addr))
>                                           ^^^^
> Instead of handling the error we dereference the error pointer here.
>

True - we should add a check for IS_ERR(addr) here.

> *frowny face*
>
>>               return -ENODEV;
>>
>> --
>
> Maybe this?
>
>         if (!addr) {
>                 nvmem = nvmem_cell_get(dev, "mac-address");
>                 if (PTR_ERR(nvmem) == -EPROBE_DEFER)
>                         return -EPROBE_DEFER;
>                 if (IS_ERR(nvmem))
>                         return -ENODEV;
>                 addr = nvmem_cell_read(nvmem, &alen);
>                 if (IS_ERR(addr))
>                         return PTR_ERR(addr);
>                 if (alen != ETH_ALEN) {
>                         kfree(addr);
>                         return -ENODEV;
>                 }
>                 ether_addr_copy(addrbuf, addr);
>                 kfree(addr);
>                 addr = addrbuf;
>         }
>         if (!is_valid_ether_addr(addr))
>                 return -ENODEV;
>         ether_addr_copy(mac_addr, addr);
>         return 0;
>

I would normally go this way but here we don't want to bail out when
we encounter an error but rather continue on to the next possible
source of a MAC address. We'll get -ENODEV from nvmem_cell_get() if
the lookup fails for "mac-address" but instead of returning an error
code we should then check if we can read the MAC from MTD.

Bart
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dan Carpenter July 19, 2018, 9:09 a.m. UTC | #3
Maybe it would be simpler as three separate functions:

int of_eth_get_mac_address() <- rename existing function to this
int nvmem_eth_get_mac_address() <- patch 2
int mtd_eth_nvmem_get_mac_address() patch 3

	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 == -EPROBEDEFER)
		return ret;
	if (!ret)
		return 0;
	ret = mtd_eth_nvmem_get_mac_address(dev, mac_addr);
	if (!ret)
		return 0;

	return -ENODEV;

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bartosz Golaszewski July 19, 2018, 10:06 a.m. UTC | #4
2018-07-19 11:09 GMT+02:00 Dan Carpenter <dan.carpenter@oracle.com>:
>
> Maybe it would be simpler as three separate functions:
>
> int of_eth_get_mac_address() <- rename existing function to this
> int nvmem_eth_get_mac_address() <- patch 2
> int mtd_eth_nvmem_get_mac_address() patch 3
>
>         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 == -EPROBEDEFER)
>                 return ret;
>         if (!ret)
>                 return 0;
>         ret = mtd_eth_nvmem_get_mac_address(dev, mac_addr);
>         if (!ret)
>                 return 0;
>
>         return -ENODEV;
>
> regards,
> dan carpenter

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.

Thanks,
Bart
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Russell King (Oracle) July 19, 2018, 12:23 p.m. UTC | #5
On Thu, Jul 19, 2018 at 11:48:37AM +0300, Dan Carpenter wrote:
> On Thu, Jul 19, 2018 at 10:20:27AM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > 
> > Many non-DT platforms read the MAC address from EEPROM. Usually it's
> > either done with callbacks defined in board files or from SoC-specific
> > ethernet drivers.
> > 
> > In order to generalize this, try to read the MAC from nvmem in
> > eth_platform_get_mac_address() using a standard lookup name:
> > "mac-address".
> > 
> > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > ---
> >  net/ethernet/eth.c | 27 +++++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> > 
> > diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
> > index 39af03894598..af3b4b1b77eb 100644
> > --- a/net/ethernet/eth.c
> > +++ b/net/ethernet/eth.c
> > @@ -54,6 +54,7 @@
> >  #include <linux/if_ether.h>
> >  #include <linux/of_net.h>
> >  #include <linux/pci.h>
> > +#include <linux/nvmem-consumer.h>
> >  #include <net/dst.h>
> >  #include <net/arp.h>
> >  #include <net/sock.h>
> > @@ -527,8 +528,11 @@ unsigned char * __weak arch_get_platform_mac_address(void)
> >  
> >  int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr)
> >  {
> > +	unsigned char addrbuf[ETH_ALEN];
> >  	const unsigned char *addr;
> > +	struct nvmem_cell *nvmem;
> >  	struct device_node *dp;
> > +	size_t alen;
> >  
> >  	if (dev_is_pci(dev))
> >  		dp = pci_device_to_OF_node(to_pci_dev(dev));
> > @@ -541,6 +545,29 @@ int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr)
> >  	if (!addr)
> >  		addr = arch_get_platform_mac_address();
> >  
> > +	if (!addr) {
> > +		nvmem = nvmem_cell_get(dev, "mac-address");
> > +		if (IS_ERR(nvmem) && 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)) {
> > +			addr = nvmem_cell_read(nvmem, &alen);
> > +			if (!IS_ERR(addr)) {
>                                     ^^^^
> Never do success handling.  Always error handling.  Otherwise the code
> is indent a lot and the error handling is far from the call.
> 
> > +				if (alen == ETH_ALEN)
> > +					ether_addr_copy(addrbuf, addr);
> > +
> > +				kfree(addr);
> > +				addr = alen == ETH_ALEN ? addrbuf : NULL;
> > +			}
> > +
> > +			nvmem_cell_put(nvmem);
> > +		}
> > +	}
> > +
> >  	if (!addr || !is_valid_ether_addr(addr))
>                                           ^^^^
> Instead of handling the error we dereference the error pointer here.
> 
> *frowny face*
> 
> >  		return -ENODEV;
> >  
> > --
> 
> Maybe this?
> 
> 	if (!addr) {
> 		nvmem = nvmem_cell_get(dev, "mac-address");
> 		if (PTR_ERR(nvmem) == -EPROBE_DEFER)
> 			return -EPROBE_DEFER;
> 		if (IS_ERR(nvmem))
> 			return -ENODEV;
> 		addr = nvmem_cell_read(nvmem, &alen);
> 		if (IS_ERR(addr))
> 			return PTR_ERR(addr);

The problem with doing it this way is... error handling is Hard(tm).
You missed the call to nvmem_cell_put() here.

> 		if (alen != ETH_ALEN) {
> 			kfree(addr);

and again here.

> 			return -ENODEV;
> 		}
> 		ether_addr_copy(addrbuf, addr);
> 		kfree(addr);
> 		addr = addrbuf;

and here.

> 	}
> 	if (!is_valid_ether_addr(addr))
> 		return -ENODEV;
> 	ether_addr_copy(mac_addr, addr);
> 	return 0;

Without checking the semantics, a possible solution to that could be:

	if (!addr) {
		nvmem = nvmem_cell_get(dev, "mac-address");
		if (PTR_ERR(nvmem) == -EPROBE_DEFER)
			return -EPROBE_DEFER;
		if (IS_ERR(nvmem))
			return -ENODEV;
		addr = nvmem_cell_read(nvmem, &alen);
+		nvmem_cell_put(nvmem);
		if (IS_ERR(addr))
			return PTR_ERR(addr);
		if (alen != ETH_ALEN) {
			kfree(addr);
			return -ENODEV;
		}
		ether_addr_copy(addrbuf, addr);
		kfree(addr);
		addr = addrbuf;
	}
	if (!is_valid_ether_addr(addr))
		return -ENODEV;
	ether_addr_copy(mac_addr, addr);
	return 0;

A potential better solution would be to put this code in a separate
function, and then do:

	if (!addr)
		addr = eth_platform_get_nvmem_mac_address(dev, addrbuf);

which returns either NULL or addrbuf depending on whether it failed or
succeeded, which would probably end up with cleaner code.
diff mbox

Patch

diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index 39af03894598..af3b4b1b77eb 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -54,6 +54,7 @@ 
 #include <linux/if_ether.h>
 #include <linux/of_net.h>
 #include <linux/pci.h>
+#include <linux/nvmem-consumer.h>
 #include <net/dst.h>
 #include <net/arp.h>
 #include <net/sock.h>
@@ -527,8 +528,11 @@  unsigned char * __weak arch_get_platform_mac_address(void)
 
 int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr)
 {
+	unsigned char addrbuf[ETH_ALEN];
 	const unsigned char *addr;
+	struct nvmem_cell *nvmem;
 	struct device_node *dp;
+	size_t alen;
 
 	if (dev_is_pci(dev))
 		dp = pci_device_to_OF_node(to_pci_dev(dev));
@@ -541,6 +545,29 @@  int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr)
 	if (!addr)
 		addr = arch_get_platform_mac_address();
 
+	if (!addr) {
+		nvmem = nvmem_cell_get(dev, "mac-address");
+		if (IS_ERR(nvmem) && 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)) {
+			addr = nvmem_cell_read(nvmem, &alen);
+			if (!IS_ERR(addr)) {
+				if (alen == ETH_ALEN)
+					ether_addr_copy(addrbuf, addr);
+
+				kfree(addr);
+				addr = alen == ETH_ALEN ? addrbuf : NULL;
+			}
+
+			nvmem_cell_put(nvmem);
+		}
+	}
+
 	if (!addr || !is_valid_ether_addr(addr))
 		return -ENODEV;