[PATCH 0/5] net: extend eth_platform_get_mac_address()

[PATCH 0/5] net: extend eth_platform_get_mac_address()

[Bartosz Golaszewski]

This is a follow-up to a series I posted a while ago the goal of which
was to replace the at24 platform data with device properties. To do so
we need to somehow remove reading the MAC address from relevant board
files.

In my patches I used nvmem and MTD to read the MAC address from within
the davinci emac driver. It was suggested that we generalize it further
but since MTD doesn't support nvmem yet, the best we can do is to move
this code over to net core code.

The following patches modify the eth_platform_get_mac_address()
function which seems to be the best candidate for this code.

The first patch only visually shrinks the code because this routine
will grow significantly in the next patches.

The second patch adds an info message on success so that we keep
getting notifications from users who already are verbose.

The third patch calls is_valid_ether_addr() on the read address so
that we're sure it's correct.

The last two patches add nvmem and MTD support to the function. In
order to stay compatible with existing users, nvmem and MTD will be
tried last - after device tree and arch-specific callback.

If this series gets accepted I will modify my previous patches to
use it instead of handcoding the same operations in davinci_emac.

Bartosz Golaszewski (5):
  net: visually shrink eth_platform_get_mac_address()
  net: add an info message to eth_platform_get_mac_address()
  net: fortify eth_platform_get_mac_address()
  net: add support for nvmem to eth_platform_get_mac_address()
  net: add MTD support to eth_platform_get_mac_address()

 net/ethernet/eth.c | 77 ++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 65 insertions(+), 12 deletions(-)

-- 
2.17.1

[PATCH 1/5] net: visually shrink eth_platform_get_mac_address()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Initialize the variables with proper values so that we save a few
lines of code before we extend this function in the follow-up patches.

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

diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index ee28440f57c5..da8530879e1e 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -527,15 +527,10 @@ unsigned char * __weak arch_get_platform_mac_address(void)
 
 int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr)
 {
-	const unsigned char *addr;
-	struct device_node *dp;
+	struct device_node *dp = dev_is_pci(dev) ?
+			pci_device_to_OF_node(to_pci_dev(dev)) : dev->of_node;
+	const unsigned char *addr = NULL;
 
-	if (dev_is_pci(dev))
-		dp = pci_device_to_OF_node(to_pci_dev(dev));
-	else
-		dp = dev->of_node;
-
-	addr = NULL;
 	if (dp)
 		addr = of_get_mac_address(dp);
 	if (!addr)
-- 
2.17.1

[PATCH 2/5] net: add an info message to eth_platform_get_mac_address()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Many drivers that read the MAC address from EEPROM or MTD emit a log
message when they succeed. Since this function is meant to be reused
in those drivers instead of reimplementing the same operation
everywhere, add an info message when we successfully read the MAC
address.

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

diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index da8530879e1e..2a2173324d9e 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -530,15 +530,24 @@ int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr)
 	struct device_node *dp = dev_is_pci(dev) ?
 			pci_device_to_OF_node(to_pci_dev(dev)) : dev->of_node;
 	const unsigned char *addr = NULL;
+	const char *from = NULL;
 
-	if (dp)
+	if (dp) {
 		addr = of_get_mac_address(dp);
-	if (!addr)
+		if (addr)
+			from = "device tree";
+	}
+
+	if (!addr) {
 		addr = arch_get_platform_mac_address();
+		if (addr)
+			from = "arch callback";
+	}
 
 	if (!addr)
 		return -ENODEV;
 
+	dev_info(dev, "read MAC address from %s\n", from);
 	ether_addr_copy(mac_addr, addr);
 	return 0;
 }
-- 
2.17.1

[PATCH 3/5] net: fortify eth_platform_get_mac_address()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

We'll soon have more sources from which to read the MAC address in this
routine. Make sure the address is correct before returning it.

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

diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index 2a2173324d9e..6b64586fd2af 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -544,7 +544,7 @@ int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr)
 			from = "arch callback";
 	}
 
-	if (!addr)
+	if (!addr || !is_valid_ether_addr(addr))
 		return -ENODEV;
 
 	dev_info(dev, "read MAC address from %s\n", from);
-- 
2.17.1

[PATCH 4/5] net: add support for nvmem to eth_platform_get_mac_address()

[Bartosz Golaszewski]

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 | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index 6b64586fd2af..adf5bd03851f 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>
@@ -530,7 +531,10 @@ int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr)
 	struct device_node *dp = dev_is_pci(dev) ?
 			pci_device_to_OF_node(to_pci_dev(dev)) : dev->of_node;
 	const unsigned char *addr = NULL;
+	unsigned char addrbuf[ETH_ALEN];
+	struct nvmem_cell *nvmem;
 	const char *from = NULL;
+	size_t alen;
 
 	if (dp) {
 		addr = of_get_mac_address(dp);
@@ -544,6 +548,31 @@ int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr)
 			from = "arch callback";
 	}
 
+	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)) {
+				from = "nvmem";
+				/* Don't use ether_addr_copy() in case we
+				 * didn't get the right size.
+				 */
+				memcpy(addrbuf, addr, alen);
+				kfree(addr);
+				addr = addrbuf;
+			}
+
+			nvmem_cell_put(nvmem);
+		}
+	}
+
 	if (!addr || !is_valid_ether_addr(addr))
 		return -ENODEV;
 
-- 
2.17.1

[PATCH 5/5] net: add MTD support to eth_platform_get_mac_address()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

MTD doesn't support nvmem yet. Some platforms use MTD to read the MAC
address from SPI flash. If we want this function to generalize reading
the MAC address, we need to separately try to use MTD.

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

diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index adf5bd03851f..f7dbd2cff7f9 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -55,6 +55,7 @@
 #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>
@@ -573,6 +574,25 @@ int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr)
 		}
 	}
 
+#ifdef CONFIG_MTD
+	/* NOTE: this should go away as soon as MTD gets nvmem support. */
+	if (!addr) {
+		struct mtd_info *mtd;
+		int rv;
+
+		mtd = get_mtd_device_nm("MAC-Address");
+		if (!IS_ERR(mtd)) {
+			rv = mtd_read(mtd, 0, ETH_ALEN, &alen, addrbuf);
+			if (rv == 0) {
+				from = "MTD";
+				addr = addrbuf;
+			}
+
+			put_mtd_device(mtd);
+		}
+	}
+#endif /* CONFIG_MTD */
+
 	if (!addr || !is_valid_ether_addr(addr))
 		return -ENODEV;
 
-- 
2.17.1

Re: [PATCH 1/5] net: visually shrink eth_platform_get_mac_address()

[Andrew Lunn]

On Wed, Jul 18, 2018 at 06:10:31PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Initialize the variables with proper values so that we save a few
> lines of code before we extend this function in the follow-up patches.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  net/ethernet/eth.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
> index ee28440f57c5..da8530879e1e 100644
> --- a/net/ethernet/eth.c
> +++ b/net/ethernet/eth.c
> @@ -527,15 +527,10 @@ unsigned char * __weak arch_get_platform_mac_address(void)
>  
>  int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr)
>  {
> -	const unsigned char *addr;
> -	struct device_node *dp;
> +	struct device_node *dp = dev_is_pci(dev) ?
> +			pci_device_to_OF_node(to_pci_dev(dev)) : dev->of_node;
> +	const unsigned char *addr = NULL;

Hi Bartosz

You are now in the net subsystem, which has its own set of additional
coding styles. One of them is reverse Christmas tree.

You might want to read Documentation/networking/netdev-FAQ.txt.

    Andrew

Re: [PATCH 2/5] net: add an info message to eth_platform_get_mac_address()

[Andrew Lunn]

On Wed, Jul 18, 2018 at 06:10:32PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Many drivers that read the MAC address from EEPROM or MTD emit a log
> message when they succeed. Since this function is meant to be reused
> in those drivers instead of reimplementing the same operation
> everywhere, add an info message when we successfully read the MAC
> address.

Hi Bartosz

This makes eth_platform_get_mac_address() generally more verbose,
which i guess people won't like. To keep it backwards compatible, it
would be better to issue the message just when EEPROM to MTD is used.

      Andrew

Re: [PATCH 1/5] net: visually shrink eth_platform_get_mac_address()

[Bartosz Golaszewski]

2018-07-18 18:28 GMT+02:00 Andrew Lunn <andrew@lunn.ch>:
> On Wed, Jul 18, 2018 at 06:10:31PM +0200, Bartosz Golaszewski wrote:
>> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>>
>> Initialize the variables with proper values so that we save a few
>> lines of code before we extend this function in the follow-up patches.
>>
>> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>> ---
>>  net/ethernet/eth.c | 11 +++--------
>>  1 file changed, 3 insertions(+), 8 deletions(-)
>>
>> diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
>> index ee28440f57c5..da8530879e1e 100644
>> --- a/net/ethernet/eth.c
>> +++ b/net/ethernet/eth.c
>> @@ -527,15 +527,10 @@ unsigned char * __weak arch_get_platform_mac_address(void)
>>
>>  int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr)
>>  {
>> -     const unsigned char *addr;
>> -     struct device_node *dp;
>> +     struct device_node *dp = dev_is_pci(dev) ?
>> +                     pci_device_to_OF_node(to_pci_dev(dev)) : dev->of_node;
>> +     const unsigned char *addr = NULL;
>
> Hi Bartosz
>
> You are now in the net subsystem, which has its own set of additional
> coding styles. One of them is reverse Christmas tree.
>
> You might want to read Documentation/networking/netdev-FAQ.txt.
>
>     Andrew

Hi Andrew,

it's still reverse Christmas tree in this patch except that now we're
taking the length of the variable + initializer into account. I'm not
sure if this is the right approach though.

Bart

Re: [PATCH 2/5] net: add an info message to eth_platform_get_mac_address()

[Bartosz Golaszewski]

2018-07-18 18:31 GMT+02:00 Andrew Lunn <andrew@lunn.ch>:
> On Wed, Jul 18, 2018 at 06:10:32PM +0200, Bartosz Golaszewski wrote:
>> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>>
>> Many drivers that read the MAC address from EEPROM or MTD emit a log
>> message when they succeed. Since this function is meant to be reused
>> in those drivers instead of reimplementing the same operation
>> everywhere, add an info message when we successfully read the MAC
>> address.
>
> Hi Bartosz
>
> This makes eth_platform_get_mac_address() generally more verbose,
> which i guess people won't like. To keep it backwards compatible, it
> would be better to issue the message just when EEPROM to MTD is used.
>
>       Andrew

This function should be used at most once per interface - I guess it's
not like it's a huge impact on verbosity.

Bart

Re: [PATCH 3/5] net: fortify eth_platform_get_mac_address()

[Andrew Lunn]

On Wed, Jul 18, 2018 at 06:10:33PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> We'll soon have more sources from which to read the MAC address in this
> routine. Make sure the address is correct before returning it.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH 4/5] net: add support for nvmem to eth_platform_get_mac_address()

[Andrew Lunn]

> +		if (!IS_ERR(nvmem)) {
> +			addr = nvmem_cell_read(nvmem, &alen);
> +			if (!IS_ERR(addr)) {
> +				from = "nvmem";
> +				/* Don't use ether_addr_copy() in case we
> +				 * didn't get the right size.
> +				 */

Please verify the size. A short read can still give a valid MAC
address, so the is_valid_ether_addr(addr) is not sufficient.

> +				memcpy(addrbuf, addr, alen);

Another reason to check the length is that you appear to have a buffer
overflow here, if alen > 6.

	 Andrew

> +				kfree(addr);
> +				addr = addrbuf;
> +			}
> +
> +			nvmem_cell_put(nvmem);
> +		}
> +	}
> +
>  	if (!addr || !is_valid_ether_addr(addr))
>  		return -ENODEV;
>  
> -- 
> 2.17.1
> 

Re: [PATCH 5/5] net: add MTD support to eth_platform_get_mac_address()

[Andrew Lunn]

On Wed, Jul 18, 2018 at 06:10:35PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> MTD doesn't support nvmem yet. Some platforms use MTD to read the MAC
> address from SPI flash. If we want this function to generalize reading
> the MAC address, we need to separately try to use MTD.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  net/ethernet/eth.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
> index adf5bd03851f..f7dbd2cff7f9 100644
> --- a/net/ethernet/eth.c
> +++ b/net/ethernet/eth.c
> @@ -55,6 +55,7 @@
>  #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>
> @@ -573,6 +574,25 @@ int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr)
>  		}
>  	}
>  
> +#ifdef CONFIG_MTD
> +	/* NOTE: this should go away as soon as MTD gets nvmem support. */
> +	if (!addr) {
> +		struct mtd_info *mtd;
> +		int rv;
> +
> +		mtd = get_mtd_device_nm("MAC-Address");

In order for this to go away, you need to keep backwards
compatibility. When using nvmem, you look for a cell called
"mac-address". Here you are looking for "MAC-Address". That is going
to make backwards compatibility harder. How do you plan to do it?

   Andrew

Re: [PATCH 5/5] net: add MTD support to eth_platform_get_mac_address()

[Bartosz Golaszewski]

2018-07-18 18:47 GMT+02:00 Andrew Lunn <andrew@lunn.ch>:
> On Wed, Jul 18, 2018 at 06:10:35PM +0200, Bartosz Golaszewski wrote:
>> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>>
>> MTD doesn't support nvmem yet. Some platforms use MTD to read the MAC
>> address from SPI flash. If we want this function to generalize reading
>> the MAC address, we need to separately try to use MTD.
>>
>> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>> ---
>>  net/ethernet/eth.c | 20 ++++++++++++++++++++
>>  1 file changed, 20 insertions(+)
>>
>> diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
>> index adf5bd03851f..f7dbd2cff7f9 100644
>> --- a/net/ethernet/eth.c
>> +++ b/net/ethernet/eth.c
>> @@ -55,6 +55,7 @@
>>  #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>
>> @@ -573,6 +574,25 @@ int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr)
>>               }
>>       }
>>
>> +#ifdef CONFIG_MTD
>> +     /* NOTE: this should go away as soon as MTD gets nvmem support. */
>> +     if (!addr) {
>> +             struct mtd_info *mtd;
>> +             int rv;
>> +
>> +             mtd = get_mtd_device_nm("MAC-Address");
>
> In order for this to go away, you need to keep backwards
> compatibility. When using nvmem, you look for a cell called
> "mac-address". Here you are looking for "MAC-Address". That is going
> to make backwards compatibility harder. How do you plan to do it?
>
>    Andrew

I'm trying to adjust to already existing users. The only user of
get_mtd_device_nm() who calls it to read the MAC address registers a
partition called "MAC-Address". We can't change it since it's visible
from user space. In the future we'd just have to have a list of
supported string that we'd use to do the nvmem lookup.

Bart

Re: [PATCH 5/5] net: add MTD support to eth_platform_get_mac_address()

[Andrew Lunn]

> >> +#ifdef CONFIG_MTD
> >> +     /* NOTE: this should go away as soon as MTD gets nvmem support. */
> >> +     if (!addr) {
> >> +             struct mtd_info *mtd;
> >> +             int rv;
> >> +
> >> +             mtd = get_mtd_device_nm("MAC-Address");
> >
> > In order for this to go away, you need to keep backwards
> > compatibility. When using nvmem, you look for a cell called
> > "mac-address". Here you are looking for "MAC-Address". That is going
> > to make backwards compatibility harder. How do you plan to do it?
> >
> >    Andrew
> 
> I'm trying to adjust to already existing users. The only user of
> get_mtd_device_nm() who calls it to read the MAC address registers a
> partition called "MAC-Address". We can't change it since it's visible
> from user space. In the future we'd just have to have a list of
> supported string that we'd use to do the nvmem lookup.

Why not have the nvmem cell called "MAC-Address"? When you add nvmem
support to MTD, i assume you are going to map each MTD partition to an
nvmem cell?

    Andrew

Re: [PATCH 1/5] net: visually shrink eth_platform_get_mac_address()

[David Miller]

From: Bartosz Golaszewski <brgl@bgdev.pl>
Date: Wed, 18 Jul 2018 18:10:31 +0200

> @@ -527,15 +527,10 @@ unsigned char * __weak arch_get_platform_mac_address(void)
>  
>  int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr)
>  {
> -	const unsigned char *addr;
> -	struct device_node *dp;
> +	struct device_node *dp = dev_is_pci(dev) ?
> +			pci_device_to_OF_node(to_pci_dev(dev)) : dev->of_node;
> +	const unsigned char *addr = NULL;
>  
> -	if (dev_is_pci(dev))
> -		dp = pci_device_to_OF_node(to_pci_dev(dev));
> -	else
> -		dp = dev->of_node;
> -

Reverse christmas tree is why the assignments are in the body of
the function instead of the declaration area.

Please don't do this, thanks.

Re: [PATCH 2/5] net: add an info message to eth_platform_get_mac_address()

[David Miller]

From: Bartosz Golaszewski <brgl@bgdev.pl>
Date: Wed, 18 Jul 2018 18:10:32 +0200

>  
> +	dev_info(dev, "read MAC address from %s\n", from);
>  	ether_addr_copy(mac_addr, addr);
>  	return 0;

Ugh, please don't do this.

We probe various bits of information from various sources during
driver probe, and none of them are more or less important than
the MAC address.  So singling this out for log info output is
really not such a great idea.

Thank you.

Re: [PATCH 5/5] net: add MTD support to eth_platform_get_mac_address()

[Bartosz Golaszewski]

2018-07-18 19:03 GMT+02:00 Andrew Lunn <andrew@lunn.ch>:
>> >> +#ifdef CONFIG_MTD
>> >> +     /* NOTE: this should go away as soon as MTD gets nvmem support. */
>> >> +     if (!addr) {
>> >> +             struct mtd_info *mtd;
>> >> +             int rv;
>> >> +
>> >> +             mtd = get_mtd_device_nm("MAC-Address");
>> >
>> > In order for this to go away, you need to keep backwards
>> > compatibility. When using nvmem, you look for a cell called
>> > "mac-address". Here you are looking for "MAC-Address". That is going
>> > to make backwards compatibility harder. How do you plan to do it?
>> >
>> >    Andrew
>>
>> I'm trying to adjust to already existing users. The only user of
>> get_mtd_device_nm() who calls it to read the MAC address registers a
>> partition called "MAC-Address". We can't change it since it's visible
>> from user space. In the future we'd just have to have a list of
>> supported string that we'd use to do the nvmem lookup.
>
> Why not have the nvmem cell called "MAC-Address"? When you add nvmem
> support to MTD, i assume you are going to map each MTD partition to an
> nvmem cell?

Because all existing users of nvmem use "mac-address" as the name of
this cell already. I guess we will need to live with both in this
particular function.

Bart

Re: [PATCH 5/5] net: add MTD support to eth_platform_get_mac_address()

[Andrew Lunn]

On Thu, Jul 19, 2018 at 10:14:29AM +0200, Bartosz Golaszewski wrote:
> 2018-07-18 19:03 GMT+02:00 Andrew Lunn <andrew@lunn.ch>:
> >> >> +#ifdef CONFIG_MTD
> >> >> +     /* NOTE: this should go away as soon as MTD gets nvmem support. */
> >> >> +     if (!addr) {
> >> >> +             struct mtd_info *mtd;
> >> >> +             int rv;
> >> >> +
> >> >> +             mtd = get_mtd_device_nm("MAC-Address");
> >> >
> >> > In order for this to go away, you need to keep backwards
> >> > compatibility. When using nvmem, you look for a cell called
> >> > "mac-address". Here you are looking for "MAC-Address". That is going
> >> > to make backwards compatibility harder. How do you plan to do it?
> >> >
> >> >    Andrew
> >>
> >> I'm trying to adjust to already existing users. The only user of
> >> get_mtd_device_nm() who calls it to read the MAC address registers a
> >> partition called "MAC-Address". We can't change it since it's visible
> >> from user space. In the future we'd just have to have a list of
> >> supported string that we'd use to do the nvmem lookup.
> >
> > Why not have the nvmem cell called "MAC-Address"? When you add nvmem
> > support to MTD, i assume you are going to map each MTD partition to an
> > nvmem cell?
> 
> Because all existing users of nvmem use "mac-address" as the name of
> this cell already. I guess we will need to live with both in this
> particular function.

So i'm not convinced this last patch is making things better. I would
prefer if it was dropped for the moment. Wait until MTD via nvmem is
actually implemented and there is a concrete concept of how a MAC
address would be looked up without having lots of ugly code.

	Andrew

Re: [PATCH 5/5] net: add MTD support to eth_platform_get_mac_address()

[Bartosz Golaszewski]

2018-07-19 17:01 GMT+02:00 Andrew Lunn <andrew@lunn.ch>:
> On Thu, Jul 19, 2018 at 10:14:29AM +0200, Bartosz Golaszewski wrote:
>> 2018-07-18 19:03 GMT+02:00 Andrew Lunn <andrew@lunn.ch>:
>> >> >> +#ifdef CONFIG_MTD
>> >> >> +     /* NOTE: this should go away as soon as MTD gets nvmem support. */
>> >> >> +     if (!addr) {
>> >> >> +             struct mtd_info *mtd;
>> >> >> +             int rv;
>> >> >> +
>> >> >> +             mtd = get_mtd_device_nm("MAC-Address");
>> >> >
>> >> > In order for this to go away, you need to keep backwards
>> >> > compatibility. When using nvmem, you look for a cell called
>> >> > "mac-address". Here you are looking for "MAC-Address". That is going
>> >> > to make backwards compatibility harder. How do you plan to do it?
>> >> >
>> >> >    Andrew
>> >>
>> >> I'm trying to adjust to already existing users. The only user of
>> >> get_mtd_device_nm() who calls it to read the MAC address registers a
>> >> partition called "MAC-Address". We can't change it since it's visible
>> >> from user space. In the future we'd just have to have a list of
>> >> supported string that we'd use to do the nvmem lookup.
>> >
>> > Why not have the nvmem cell called "MAC-Address"? When you add nvmem
>> > support to MTD, i assume you are going to map each MTD partition to an
>> > nvmem cell?
>>
>> Because all existing users of nvmem use "mac-address" as the name of
>> this cell already. I guess we will need to live with both in this
>> particular function.
>
> So i'm not convinced this last patch is making things better. I would
> prefer if it was dropped for the moment. Wait until MTD via nvmem is
> actually implemented and there is a concrete concept of how a MAC
> address would be looked up without having lots of ugly code.
>
>         Andrew

Unfortunately: this would effectively block me from improving the
support for older davinci boards. Having a (subjectively) ugly but
generalized way of reading the MAC address from MTD is still better
than using the MTD notifier from board files IMO.

Bart

Re: [PATCH 4/5] net: add support for nvmem to eth_platform_get_mac_address()

[Andrew Lunn]

On Wed, Jul 18, 2018 at 06:10:34PM +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 | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
> index 6b64586fd2af..adf5bd03851f 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>
> @@ -530,7 +531,10 @@ int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr)
>  	struct device_node *dp = dev_is_pci(dev) ?
>  			pci_device_to_OF_node(to_pci_dev(dev)) : dev->of_node;
>  	const unsigned char *addr = NULL;
> +	unsigned char addrbuf[ETH_ALEN];
> +	struct nvmem_cell *nvmem;
>  	const char *from = NULL;
> +	size_t alen;
>  
>  	if (dp) {
>  		addr = of_get_mac_address(dp);
> @@ -544,6 +548,31 @@ int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr)
>  			from = "arch callback";
>  	}
>  
> +	if (!addr) {
> +		nvmem = nvmem_cell_get(dev, "mac-address");
> +		if (IS_ERR(nvmem) && PTR_ERR(nvmem) == -EPROBE_DEFER)

How does EPROBE_DEFER work here? You say the use case is
Non-DT. Without having DT, how do you know the cell should exist, but
does not yet exist? I might be looking at old code, but i only see
-EPROBE_DEFER inside the if (np) case.

> +			/* We may have a lookup registered for MAC address but
> +			 * the corresponding nvmem provider hasn't been
> +			 * registered yet.
> +			 */
> +			return -EPROBE_DEFER;

You really should return real errors. If i'm reading
__nvmem_device_get() right, it will return a NULL pointer when the
cell does not exist. NULL is not an error, so IS_ERR() will return
false. So you should return all errors from nvmem_cell_get().

       Andrew

Re: [PATCH 4/5] net: add support for nvmem to eth_platform_get_mac_address()

[Bartosz Golaszewski]

2018-07-19 17:22 GMT+02:00 Andrew Lunn <andrew@lunn.ch>:
> On Wed, Jul 18, 2018 at 06:10:34PM +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 | 29 +++++++++++++++++++++++++++++
>>  1 file changed, 29 insertions(+)
>>
>> diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
>> index 6b64586fd2af..adf5bd03851f 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>
>> @@ -530,7 +531,10 @@ int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr)
>>       struct device_node *dp = dev_is_pci(dev) ?
>>                       pci_device_to_OF_node(to_pci_dev(dev)) : dev->of_node;
>>       const unsigned char *addr = NULL;
>> +     unsigned char addrbuf[ETH_ALEN];
>> +     struct nvmem_cell *nvmem;
>>       const char *from = NULL;
>> +     size_t alen;
>>
>>       if (dp) {
>>               addr = of_get_mac_address(dp);
>> @@ -544,6 +548,31 @@ int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr)
>>                       from = "arch callback";
>>       }
>>
>> +     if (!addr) {
>> +             nvmem = nvmem_cell_get(dev, "mac-address");
>> +             if (IS_ERR(nvmem) && PTR_ERR(nvmem) == -EPROBE_DEFER)
>
> How does EPROBE_DEFER work here? You say the use case is
> Non-DT. Without having DT, how do you know the cell should exist, but
> does not yet exist? I might be looking at old code, but i only see
> -EPROBE_DEFER inside the if (np) case.
>
>> +                     /* We may have a lookup registered for MAC address but
>> +                      * the corresponding nvmem provider hasn't been
>> +                      * registered yet.
>> +                      */
>> +                     return -EPROBE_DEFER;
>
> You really should return real errors. If i'm reading
> __nvmem_device_get() right, it will return a NULL pointer when the
> cell does not exist. NULL is not an error, so IS_ERR() will return
> false. So you should return all errors from nvmem_cell_get().
>
>        Andrew

We have a patch queued for nvmem for 4.19 which adds a notion of nvmem
cell lookups similar to gpio lookups:
https://patchwork.kernel.org/patch/10496045/

This will work fine with probe deferral.

Bart

Re: [PATCH 5/5] net: add MTD support to eth_platform_get_mac_address()

[Andrew Lunn]

> Unfortunately: this would effectively block me from improving the
> support for older davinci boards.

Is there something blocking you from converting the board to device
tree? This is something i did with a lot of the Marvell boards a few
years ago. For a while, we had both DT and board setup files. After a
couple of cycles, we killed off the setup files.

       Andrew

Re: [PATCH 5/5] net: add MTD support to eth_platform_get_mac_address()

[Bartosz Golaszewski]

2018-07-19 17:27 GMT+02:00 Andrew Lunn <andrew@lunn.ch>:
>> Unfortunately: this would effectively block me from improving the
>> support for older davinci boards.
>
> Is there something blocking you from converting the board to device
> tree? This is something i did with a lot of the Marvell boards a few
> years ago. For a while, we had both DT and board setup files. After a
> couple of cycles, we killed off the setup files.
>
>        Andrew

Actually some board are supported both in DT and board files
(da850-evm) right now, but Sekhar wants to keep the support via board
files in the kernel so that's a no go.

Bart

Re: [PATCH 4/5] net: add support for nvmem to eth_platform_get_mac_address()

[Russell King - ARM Linux]

On Wed, Jul 18, 2018 at 06:10:34PM +0200, Bartosz Golaszewski wrote:
> @@ -544,6 +548,31 @@ int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr)
>  			from = "arch callback";
>  	}
>  
> +	if (!addr) {
> +		nvmem = nvmem_cell_get(dev, "mac-address");
> +		if (IS_ERR(nvmem) && PTR_ERR(nvmem) == -EPROBE_DEFER)

This is way too verbose.  To quote Al Viro from earlier today:

<viro> sigh...
<viro>         if (IS_ERR(link) && PTR_ERR(link) == -EEXIST)
<viro> what the hell is wrong with if (link == ERR_PTR(-EEXIST))?

I wonder why so many people haven't heard of pointer comparison... ;)

IS_ERR(ERR_PTR(-EPROBE_DEFER)) is always true - if it wasn't, then
we'd be in for problems.

So, if you're asserting that nvmem is ERR_PTR(-EPROBE_DEFER) then
there's no need to do the IS_ERR(nvmem) must also be true.  Hence, a
simple pointer comparison is sufficient:

		if (nvmem == ERR_PTR(-EPROBE_DEFER))
			return -EPROBE_DEFER;

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
According to speedtest.net: 13Mbps down 490kbps up

Re: [PATCH 4/5] net: add support for nvmem to eth_platform_get_mac_address()

[Bartosz Golaszewski]

2018-07-19 19:47 GMT+02:00 Russell King - ARM Linux <linux@armlinux.org.uk>:
> On Wed, Jul 18, 2018 at 06:10:34PM +0200, Bartosz Golaszewski wrote:
>> @@ -544,6 +548,31 @@ int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr)
>>                       from = "arch callback";
>>       }
>>
>> +     if (!addr) {
>> +             nvmem = nvmem_cell_get(dev, "mac-address");
>> +             if (IS_ERR(nvmem) && PTR_ERR(nvmem) == -EPROBE_DEFER)
>
> This is way too verbose.  To quote Al Viro from earlier today:
>
> <viro> sigh...
> <viro>         if (IS_ERR(link) && PTR_ERR(link) == -EEXIST)
> <viro> what the hell is wrong with if (link == ERR_PTR(-EEXIST))?
>
> I wonder why so many people haven't heard of pointer comparison... ;)
>
> IS_ERR(ERR_PTR(-EPROBE_DEFER)) is always true - if it wasn't, then
> we'd be in for problems.
>
> So, if you're asserting that nvmem is ERR_PTR(-EPROBE_DEFER) then
> there's no need to do the IS_ERR(nvmem) must also be true.  Hence, a
> simple pointer comparison is sufficient:
>
>                 if (nvmem == ERR_PTR(-EPROBE_DEFER))
>                         return -EPROBE_DEFER;
>

Hi Russell,

this issue is gone now in v3 but thanks for the example - I somehow
never thought about it.

Bart

Re: [PATCH 4/5] net: add support for nvmem to eth_platform_get_mac_address()

[Andrew Lunn]

> Hi Russell,
> 
> this issue is gone now in v3 but thanks for the example - I somehow
> never thought about it.

Please don't send patchset so fast. It wastes people time, reviewing
outdated versions. No more than one version of a patchset per day.

	 Andrew

Re: [PATCH 5/5] net: add MTD support to eth_platform_get_mac_address()

[Sekhar Nori]

On Thursday 19 July 2018 09:05 PM, Bartosz Golaszewski wrote:
> 2018-07-19 17:27 GMT+02:00 Andrew Lunn <andrew@lunn.ch>:
>>> Unfortunately: this would effectively block me from improving the
>>> support for older davinci boards.
>>
>> Is there something blocking you from converting the board to device
>> tree? This is something i did with a lot of the Marvell boards a few
>> years ago. For a while, we had both DT and board setup files. After a
>> couple of cycles, we killed off the setup files.
>>
>>        Andrew
> 
> Actually some board are supported both in DT and board files
> (da850-evm) right now, but Sekhar wants to keep the support via board
> files in the kernel so that's a no go.

Its not that I want it that way, but we cannot get rid of board files
till DT has equivalent support.

The bigger issue is not on DA850, but on the 5 older DaVinci SoCs which
do not support device-tree based boot today.

Thanks,
Sekhar

Re: [PATCH 5/5] net: add MTD support to eth_platform_get_mac_address()

[Andrew Lunn]

On Fri, Jul 20, 2018 at 10:47:51AM +0530, Sekhar Nori wrote:
> On Thursday 19 July 2018 09:05 PM, Bartosz Golaszewski wrote:
> > 2018-07-19 17:27 GMT+02:00 Andrew Lunn <andrew@lunn.ch>:
> >>> Unfortunately: this would effectively block me from improving the
> >>> support for older davinci boards.
> >>
> >> Is there something blocking you from converting the board to device
> >> tree? This is something i did with a lot of the Marvell boards a few
> >> years ago. For a while, we had both DT and board setup files. After a
> >> couple of cycles, we killed off the setup files.
> >>
> >>        Andrew
> > 
> > Actually some board are supported both in DT and board files
> > (da850-evm) right now, but Sekhar wants to keep the support via board
> > files in the kernel so that's a no go.
> 
> Its not that I want it that way, but we cannot get rid of board files
> till DT has equivalent support.
> 
> The bigger issue is not on DA850, but on the 5 older DaVinci SoCs which
> do not support device-tree based boot today.

The nice thing about board files is they keep any ugly code near to
where it is needed. The proposal here is to put some 'temporary' code
in the net core. And it is assumed at some point somebody will write
nvmem over MTD, which can be used to replace this temporary code, but
that in itself needs an ugly list of special cases when using board
files?

I would prefer somebody just did the work to convert these 5 boards to
DT, with a clean design of how nvmem over MTD would work. Having
converted a number of Marvell boards to DT, i have an idea of the
effort required. If most of the drivers already support DT, it can be
done quickly. So the big job here is probably nvmem over MTD.

     Andrew