[PATCH] net: e1000e: add MAC address kernel cmd line parameter

[PATCH] net: e1000e: add MAC address kernel cmd line parameter

[Flavio Suligoi]

Sometimes, in some embedded systems boards (i.e. ARM boards),
the NVM eeprom is not mounted, to save cost and space.

In this case it is necessary to bypass the NVM management
and directly force the MAC address using a kernel command-line
parameter (macaddr).

Signed-off-by: Flavio Suligoi <f.suligoi@asem.it>
---
 drivers/net/ethernet/intel/e1000e/netdev.c | 50 ++++++++++++++++++++----------
 1 file changed, 33 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 189f231..054c398 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -39,6 +39,10 @@ static int debug = -1;
 module_param(debug, int, 0);
 MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)");
 
+static unsigned char macaddr[ETH_ALEN] = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
+module_param_array(macaddr, byte, NULL, 0);
+MODULE_PARM_DESC(macaddr, "e1000e Ethernet MAC address");
+
 static const struct e1000_info *e1000_info_tbl[] = {
 	[board_82571]		= &e1000_82571_info,
 	[board_82572]		= &e1000_82572_info,
@@ -7053,6 +7057,7 @@ static int e1000_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	static int cards_found;
 	u16 aspm_disable_flag = 0;
 	int bars, i, err, pci_using_dac;
+
 	u16 eeprom_data = 0;
 	u16 eeprom_apme_mask = E1000_EEPROM_APME;
 	s32 ret_val = 0;
@@ -7232,27 +7237,38 @@ static int e1000_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	 */
 	adapter->hw.mac.ops.reset_hw(&adapter->hw);
 
-	/* systems with ASPM and others may see the checksum fail on the first
-	 * attempt. Let's give it a few tries
-	 */
-	for (i = 0;; i++) {
-		if (e1000_validate_nvm_checksum(&adapter->hw) >= 0)
-			break;
-		if (i == 2) {
-			dev_err(&pdev->dev, "The NVM Checksum Is Not Valid\n");
-			err = -EIO;
-			goto err_eeprom;
+	/* If the MAC address is supplied by the bootloader, use it! */
+	if (!is_multicast_ether_addr(macaddr)) {
+		dev_info(&pdev->dev,
+			 "MAC address from kernel command line argument\n");
+		memcpy(adapter->hw.mac.addr, macaddr, netdev->addr_len);
+		memcpy(netdev->dev_addr,     macaddr, netdev->addr_len);
+	} else {
+		/* systems with ASPM and others may see the checksum fail on
+		 * the first attempt. Let's give it a few tries
+		 */
+		dev_info(&pdev->dev, "MAC address from NVM\n");
+		for (i = 0;; i++) {
+			if (e1000_validate_nvm_checksum(&adapter->hw) >= 0)
+				break;
+			if (i == 2) {
+				dev_err(&pdev->dev,
+					"The NVM Checksum Is Not Valid\n");
+				err = -EIO;
+				goto err_eeprom;
+			}
 		}
-	}
 
-	e1000_eeprom_checks(adapter);
+		e1000_eeprom_checks(adapter);
 
-	/* copy the MAC address */
-	if (e1000e_read_mac_addr(&adapter->hw))
-		dev_err(&pdev->dev,
-			"NVM Read Error while reading MAC address\n");
+		/* copy the MAC address */
+		if (e1000e_read_mac_addr(&adapter->hw))
+			dev_err(&pdev->dev,
+				"NVM Read Error while reading MAC address\n");
 
-	memcpy(netdev->dev_addr, adapter->hw.mac.addr, netdev->addr_len);
+		memcpy(netdev->dev_addr, adapter->hw.mac.addr,
+		       netdev->addr_len);
+	}
 
 	if (!is_valid_ether_addr(netdev->dev_addr)) {
 		dev_err(&pdev->dev, "Invalid MAC Address: %pM\n",
-- 
2.7.4

[PATCH v2] net: e1000e: add MAC address kernel cmd line parameter

[Flavio Suligoi]

Sometimes, in some embedded systems boards (i.e. ARM boards),
the NVM eeprom is not mounted, to save cost and space.

In this case it is necessary to bypass the NVM management
and directly force the MAC address using a kernel command-line
parameter (macaddr).

Signed-off-by: Flavio Suligoi <f.suligoi@asem.it>

v2: - e1000_probe: remove wrong newline in the middle of the local variable list
v1: - written

---
 drivers/net/ethernet/intel/e1000e/netdev.c | 49 +++++++++++++++++++-----------
 1 file changed, 32 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 189f231..34ab560 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -39,6 +39,10 @@ static int debug = -1;
 module_param(debug, int, 0);
 MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)");
 
+static unsigned char macaddr[ETH_ALEN] = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
+module_param_array(macaddr, byte, NULL, 0);
+MODULE_PARM_DESC(macaddr, "e1000e Ethernet MAC address");
+
 static const struct e1000_info *e1000_info_tbl[] = {
 	[board_82571]		= &e1000_82571_info,
 	[board_82572]		= &e1000_82572_info,
@@ -7232,27 +7236,38 @@ static int e1000_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	 */
 	adapter->hw.mac.ops.reset_hw(&adapter->hw);
 
-	/* systems with ASPM and others may see the checksum fail on the first
-	 * attempt. Let's give it a few tries
-	 */
-	for (i = 0;; i++) {
-		if (e1000_validate_nvm_checksum(&adapter->hw) >= 0)
-			break;
-		if (i == 2) {
-			dev_err(&pdev->dev, "The NVM Checksum Is Not Valid\n");
-			err = -EIO;
-			goto err_eeprom;
+	/* If the MAC address is supplied by the bootloader, use it! */
+	if (!is_multicast_ether_addr(macaddr)) {
+		dev_info(&pdev->dev,
+			 "MAC address from kernel command line argument\n");
+		memcpy(adapter->hw.mac.addr, macaddr, netdev->addr_len);
+		memcpy(netdev->dev_addr,     macaddr, netdev->addr_len);
+	} else {
+		/* systems with ASPM and others may see the checksum fail on
+		 * the first attempt. Let's give it a few tries
+		 */
+		dev_info(&pdev->dev, "MAC address from NVM\n");
+		for (i = 0;; i++) {
+			if (e1000_validate_nvm_checksum(&adapter->hw) >= 0)
+				break;
+			if (i == 2) {
+				dev_err(&pdev->dev,
+					"The NVM Checksum Is Not Valid\n");
+				err = -EIO;
+				goto err_eeprom;
+			}
 		}
-	}
 
-	e1000_eeprom_checks(adapter);
+		e1000_eeprom_checks(adapter);
 
-	/* copy the MAC address */
-	if (e1000e_read_mac_addr(&adapter->hw))
-		dev_err(&pdev->dev,
-			"NVM Read Error while reading MAC address\n");
+		/* copy the MAC address */
+		if (e1000e_read_mac_addr(&adapter->hw))
+			dev_err(&pdev->dev,
+				"NVM Read Error while reading MAC address\n");
 
-	memcpy(netdev->dev_addr, adapter->hw.mac.addr, netdev->addr_len);
+		memcpy(netdev->dev_addr, adapter->hw.mac.addr,
+		       netdev->addr_len);
+	}
 
 	if (!is_valid_ether_addr(netdev->dev_addr)) {
 		dev_err(&pdev->dev, "Invalid MAC Address: %pM\n",
-- 
2.7.4

[PATCH v3] net: e1000e: add MAC address kernel cmd line parameter

[Flavio Suligoi]

Sometimes, in some embedded systems boards (i.e. ARM boards),
the NVM eeprom is not mounted, to save cost and space.

In this case it is necessary to bypass the NVM management
and directly force the MAC address using a kernel command-line
parameter (macaddr).

Signed-off-by: Flavio Suligoi <f.suligoi@asem.it>
---

v3: - move patch changelog in the right place
v2: - e1000_probe: remove wrong newline in the middle of the local variable list
v1: - written

 drivers/net/ethernet/intel/e1000e/netdev.c | 49 +++++++++++++++++++-----------
 1 file changed, 32 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 189f231..34ab560 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -39,6 +39,10 @@ static int debug = -1;
 module_param(debug, int, 0);
 MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)");
 
+static unsigned char macaddr[ETH_ALEN] = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
+module_param_array(macaddr, byte, NULL, 0);
+MODULE_PARM_DESC(macaddr, "e1000e Ethernet MAC address");
+
 static const struct e1000_info *e1000_info_tbl[] = {
 	[board_82571]		= &e1000_82571_info,
 	[board_82572]		= &e1000_82572_info,
@@ -7232,27 +7236,38 @@ static int e1000_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	 */
 	adapter->hw.mac.ops.reset_hw(&adapter->hw);
 
-	/* systems with ASPM and others may see the checksum fail on the first
-	 * attempt. Let's give it a few tries
-	 */
-	for (i = 0;; i++) {
-		if (e1000_validate_nvm_checksum(&adapter->hw) >= 0)
-			break;
-		if (i == 2) {
-			dev_err(&pdev->dev, "The NVM Checksum Is Not Valid\n");
-			err = -EIO;
-			goto err_eeprom;
+	/* If the MAC address is supplied by the bootloader, use it! */
+	if (!is_multicast_ether_addr(macaddr)) {
+		dev_info(&pdev->dev,
+			 "MAC address from kernel command line argument\n");
+		memcpy(adapter->hw.mac.addr, macaddr, netdev->addr_len);
+		memcpy(netdev->dev_addr,     macaddr, netdev->addr_len);
+	} else {
+		/* systems with ASPM and others may see the checksum fail on
+		 * the first attempt. Let's give it a few tries
+		 */
+		dev_info(&pdev->dev, "MAC address from NVM\n");
+		for (i = 0;; i++) {
+			if (e1000_validate_nvm_checksum(&adapter->hw) >= 0)
+				break;
+			if (i == 2) {
+				dev_err(&pdev->dev,
+					"The NVM Checksum Is Not Valid\n");
+				err = -EIO;
+				goto err_eeprom;
+			}
 		}
-	}
 
-	e1000_eeprom_checks(adapter);
+		e1000_eeprom_checks(adapter);
 
-	/* copy the MAC address */
-	if (e1000e_read_mac_addr(&adapter->hw))
-		dev_err(&pdev->dev,
-			"NVM Read Error while reading MAC address\n");
+		/* copy the MAC address */
+		if (e1000e_read_mac_addr(&adapter->hw))
+			dev_err(&pdev->dev,
+				"NVM Read Error while reading MAC address\n");
 
-	memcpy(netdev->dev_addr, adapter->hw.mac.addr, netdev->addr_len);
+		memcpy(netdev->dev_addr, adapter->hw.mac.addr,
+		       netdev->addr_len);
+	}
 
 	if (!is_valid_ether_addr(netdev->dev_addr)) {
 		dev_err(&pdev->dev, "Invalid MAC Address: %pM\n",
-- 
2.7.4

Re: [PATCH v3] net: e1000e: add MAC address kernel cmd line parameter

[Andrew Lunn]

On Thu, Feb 28, 2019 at 02:52:31PM +0100, Flavio Suligoi wrote:
> Sometimes, in some embedded systems boards (i.e. ARM boards),
> the NVM eeprom is not mounted, to save cost and space.
> 
> In this case it is necessary to bypass the NVM management
> and directly force the MAC address using a kernel command-line
> parameter (macaddr).

Hi Flavio

Why not use device tree, since this is an ARM platform?

    Andrew

Re: [PATCH v3] net: e1000e: add MAC address kernel cmd line parameter

[Flavio Suligoi]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: giovedì 28 febbraio 2019 16:33
> To: Flavio Suligoi <f.suligoi@asem.it>
> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>; David S . Miller
> <davem@davemloft.net>; intel-wired-lan@lists.osuosl.org;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v3] net: e1000e: add MAC address kernel cmd line
> parameter
> 
> On Thu, Feb 28, 2019 at 02:52:31PM +0100, Flavio Suligoi wrote:
> > Sometimes, in some embedded systems boards (i.e. ARM boards),
> > the NVM eeprom is not mounted, to save cost and space.
> >
> > In this case it is necessary to bypass the NVM management
> > and directly force the MAC address using a kernel command-line
> > parameter (macaddr).
> 
> Hi Flavio
> 
> Why not use device tree, since this is an ARM platform?

Hi Andrew,

we produce a lot of boards and we have to change the MAC address, from u-boot, for every board.
So I must save in the u-boot environment (SPI NOR flash) the MAC address for every board.

Flavio
> 
>     Andrew

Re: [PATCH v3] net: e1000e: add MAC address kernel cmd line parameter

[Andrew Lunn]

> Hi Andrew,
> 
> we produce a lot of boards and we have to change the MAC address,
> from u-boot, for every board.  So I must save in the u-boot
> environment (SPI NOR flash) the MAC address for every board.

Hi Flavio

u-boot should be able to write the MAC address in the correct part of
device tree. Boards have been doing this a long time.

Module parameters are considered bad. You should only do it if you
have no other option. Here you do have another options, so it is going
to be a hard sell getting David to access your patch.

You will have more success by adding a call to
eth_platform_get_mac_address() to the e1000e driver.

   Andrew

RE: [PATCH v3] net: e1000e: add MAC address kernel cmd line parameter

[Flavio Suligoi]

> > Hi Andrew,
> >
> > we produce a lot of boards and we have to change the MAC address,
> > from u-boot, for every board.  So I must save in the u-boot
> > environment (SPI NOR flash) the MAC address for every board.
> 
> Hi Flavio
> 
> u-boot should be able to write the MAC address in the correct part of
> device tree. Boards have been doing this a long time.
> 
> Module parameters are considered bad. You should only do it if you
> have no other option. Here you do have another options, so it is going
> to be a hard sell getting David to access your patch.
> 
> You will have more success by adding a call to
> eth_platform_get_mac_address() to the e1000e driver.

You have right, and thanks for your suggestions, 
but with a kernel parameter I can use the same method
for any board where the NVM is missed, independently of any architecture
(with or without the device tree presence - ARM or x86 or others).

In fact slso the drivers/net/ethernet/freescale/fec_main.c provides
a kernel parameter for the MAC address, in fec_get_mac:

	/*
	 * try to get mac address in following order:
	 *
	 * 1) module parameter via kernel command line in form
	 *    fec.macaddr=0x00,0x04,0x9f,0x01,0x30,0xe0
	 */
	iap = macaddr;

Thanks,

Flavio

>    Andrew

Re: [PATCH v3] net: e1000e: add MAC address kernel cmd line parameter

[Andrew Lunn]

On Thu, Feb 28, 2019 at 05:13:27PM +0000, Flavio Suligoi wrote:
> > > Hi Andrew,
> > >
> > > we produce a lot of boards and we have to change the MAC address,
> > > from u-boot, for every board.  So I must save in the u-boot
> > > environment (SPI NOR flash) the MAC address for every board.
> > 
> > Hi Flavio
> > 
> > u-boot should be able to write the MAC address in the correct part of
> > device tree. Boards have been doing this a long time.
> > 
> > Module parameters are considered bad. You should only do it if you
> > have no other option. Here you do have another options, so it is going
> > to be a hard sell getting David to access your patch.
> > 
> > You will have more success by adding a call to
> > eth_platform_get_mac_address() to the e1000e driver.
> 
> You have right, and thanks for your suggestions, 
> but with a kernel parameter I can use the same method
> for any board where the NVM is missed, independently of any architecture
> (with or without the device tree presence - ARM or x86 or others).

Hi Flavio

Well, lets wait for David to say what he thinks about the module
parameter.

	Andrew

Re: [PATCH v3] net: e1000e: add MAC address kernel cmd line parameter

[David Miller]

From: Andrew Lunn <andrew@lunn.ch>
Date: Thu, 28 Feb 2019 20:29:58 +0100

> On Thu, Feb 28, 2019 at 05:13:27PM +0000, Flavio Suligoi wrote:
>> > > Hi Andrew,
>> > >
>> > > we produce a lot of boards and we have to change the MAC address,
>> > > from u-boot, for every board.  So I must save in the u-boot
>> > > environment (SPI NOR flash) the MAC address for every board.
>> > 
>> > Hi Flavio
>> > 
>> > u-boot should be able to write the MAC address in the correct part of
>> > device tree. Boards have been doing this a long time.
>> > 
>> > Module parameters are considered bad. You should only do it if you
>> > have no other option. Here you do have another options, so it is going
>> > to be a hard sell getting David to access your patch.
>> > 
>> > You will have more success by adding a call to
>> > eth_platform_get_mac_address() to the e1000e driver.
>> 
>> You have right, and thanks for your suggestions, 
>> but with a kernel parameter I can use the same method
>> for any board where the NVM is missed, independently of any architecture
>> (with or without the device tree presence - ARM or x86 or others).
> 
> Hi Flavio
> 
> Well, lets wait for David to say what he thinks about the module
> parameter.

I already rejected this, no way... Drivers that already have the
unacceptable module parameter are no an argument for spreading this
mistake further.

RE: [PATCH v3] net: e1000e: add MAC address kernel cmd line parameter

[Flavio Suligoi]

> >> > Hi Flavio
> >> >
> >> > u-boot should be able to write the MAC address in the correct part of
> >> > device tree. Boards have been doing this a long time.
> >> >
> >> > Module parameters are considered bad. You should only do it if you
> >> > have no other option. Here you do have another options, so it is
> going
> >> > to be a hard sell getting David to access your patch.
> >> >
> >> > You will have more success by adding a call to
> >> > eth_platform_get_mac_address() to the e1000e driver.
> >>
> >> You have right, and thanks for your suggestions,
> >> but with a kernel parameter I can use the same method
> >> for any board where the NVM is missed, independently of any
> architecture
> >> (with or without the device tree presence - ARM or x86 or others).
> >
> > Hi Flavio
> >
> > Well, lets wait for David to say what he thinks about the module
> > parameter.
> 
> I already rejected this, no way... Drivers that already have the
> unacceptable module parameter are no an argument for spreading this
> mistake further.

Hi David and Andrew,

ok, thank you for your suggestions and your time!

Flavio

Re: [PATCH] net: e1000e: add MAC address kernel cmd line parameter

[David Miller]

From: Flavio Suligoi <f.suligoi@asem.it>
Date: Thu, 28 Feb 2019 10:20:35 +0100

> Sometimes, in some embedded systems boards (i.e. ARM boards),
> the NVM eeprom is not mounted, to save cost and space.
> 
> In this case it is necessary to bypass the NVM management
> and directly force the MAC address using a kernel command-line
> parameter (macaddr).
> 
> Signed-off-by: Flavio Suligoi <f.suligoi@asem.it>

Sorry, we don't do things this way.  Otherwise every single driver would
add a hack like this.

Add either DT or appropriate initrd userland components to achieve
this.