[PATCH] sky2: Add module parameter for passing the MAC address

[PATCH] sky2: Add module parameter for passing the MAC address

[Liviu Dudau]

For designs where EEPROMs are not connected to PCI Yukon2
chips we need to get the MAC address from the firmware.
Add a module parameter called 'mac_address' for this. It
will be used if no DT node can be found and the B2_MAC
register holds an invalid value.

Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
---
 drivers/net/ethernet/marvell/sky2.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/marvell/sky2.c b/drivers/net/ethernet/marvell/sky2.c
index d9f4498..a977d95 100644
--- a/drivers/net/ethernet/marvell/sky2.c
+++ b/drivers/net/ethernet/marvell/sky2.c
@@ -101,6 +101,10 @@ static int legacy_pme = 0;
 module_param(legacy_pme, int, 0);
 MODULE_PARM_DESC(legacy_pme, "Legacy power management");
 
+/* Ugh!  Let the firmware tell us the hardware address */
+static int mac_address[ETH_ALEN] = { 0, };
+module_param_array(mac_address, int, NULL, 0);
+
 static const struct pci_device_id sky2_id_table[] = {
 	{ PCI_DEVICE(PCI_VENDOR_ID_SYSKONNECT, 0x9000) }, /* SK-9Sxx */
 	{ PCI_DEVICE(PCI_VENDOR_ID_SYSKONNECT, 0x9E00) }, /* SK-9Exx */
@@ -4811,13 +4815,21 @@ static struct net_device *sky2_init_netdev(struct sky2_hw *hw, unsigned port,
 	/* try to get mac address in the following order:
 	 * 1) from device tree data
 	 * 2) from internal registers set by bootloader
+	 * 3) from the command line parameter
 	 */
 	iap = of_get_mac_address(hw->pdev->dev.of_node);
 	if (iap)
 		memcpy(dev->dev_addr, iap, ETH_ALEN);
-	else
+	else {
 		memcpy_fromio(dev->dev_addr, hw->regs + B2_MAC_1 + port * 8,
 			      ETH_ALEN);
+		if (!is_valid_ether_addr(&dev->dev_addr[0])) {
+			int i;
+
+			for (i = 0; i < ETH_ALEN; i++)
+				dev->dev_addr[i] = mac_address[i];
+		}
+	}
 
 	return dev;
 }
-- 
2.4.6

Re: [PATCH] sky2: Add module parameter for passing the MAC address

[Stephen Hemminger]

On Wed,  5 Aug 2015 16:50:54 +0100
Liviu Dudau <Liviu.Dudau@arm.com> wrote:

> For designs where EEPROMs are not connected to PCI Yukon2
> chips we need to get the MAC address from the firmware.
> Add a module parameter called 'mac_address' for this. It
> will be used if no DT node can be found and the B2_MAC
> register holds an invalid value.
> 
> Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>

Yes, I can see that this can be a real problem, and other drivers
solve the problem. The standard method is to assign a random mac address
(and then let scripts overwrite) rather than introducing module parameter.
Module parameters are discouraged because they are device specific.

 

Re: [PATCH] sky2: Add module parameter for passing the MAC address

[Liviu Dudau]

On Wed, Aug 05, 2015 at 05:40:57PM +0100, Stephen Hemminger wrote:
> On Wed,  5 Aug 2015 16:50:54 +0100
> Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> 
> > For designs where EEPROMs are not connected to PCI Yukon2
> > chips we need to get the MAC address from the firmware.
> > Add a module parameter called 'mac_address' for this. It
> > will be used if no DT node can be found and the B2_MAC
> > register holds an invalid value.
> > 
> > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> 
> Yes, I can see that this can be a real problem, and other drivers
> solve the problem. The standard method is to assign a random mac address
> (and then let scripts overwrite) rather than introducing module parameter.
> Module parameters are discouraged because they are device specific.
> 

I agree. However, in my case, the boards people have assigned MAC addresses
to the chip, they just didn't built the board in such a way as to allow one
to store that MAC address in a permanent way :( And no, I can't use the DT
because the chip is actually on the PCIe bus.

Even with the generation of a random address, it still needs to be copied
into the device, so I would guess that a version of the patch I've sent is
still relevant?

Best regards,
Liviu

>  
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH] sky2: Add module parameter for passing the MAC address

[Liviu Dudau]

On Wed, Aug 05, 2015 at 06:15:37PM +0100, Ryan Harkin wrote:
>    On 5 August 2015 at 16:50, Liviu Dudau <[1]Liviu.Dudau@arm.com> wrote:
> 
>      For designs where EEPROMs are not connected to PCI Yukon2
>      chips we need to get the MAC address from the firmware.
>      Add a module parameter called 'mac_address' for this. It
>      will be used if no DT node can be found and the B2_MAC
>      register holds an invalid value.
> 
>      Signed-off-by: Liviu Dudau <[2]Liviu.Dudau@arm.com>
> 
>    Looks good to me.  FWIW, you can have a tested or reviewed-by at your preference:
>    Tested-by: Ryan Harkin <[3]ryan.harkin@linaro.org>
>    Reviewed-by: Ryan Harkin <[4]ryan.harkin@linaro.org>
> 

Thanks Ryan, I think one can provide both tags, so I will use them together.

Best regards,
Liviu

>     
> 
>      ---
>       drivers/net/ethernet/marvell/sky2.c | 14 +++++++++++++-
>       1 file changed, 13 insertions(+), 1 deletion(-)
> 
>      diff --git a/drivers/net/ethernet/marvell/sky2.c b/drivers/net/ethernet/marvell/sky2.c
>      index d9f4498..a977d95 100644
>      --- a/drivers/net/ethernet/marvell/sky2.c
>      +++ b/drivers/net/ethernet/marvell/sky2.c
>      @@ -101,6 +101,10 @@ static int legacy_pme = 0;
>       module_param(legacy_pme, int, 0);
>       MODULE_PARM_DESC(legacy_pme, "Legacy power management");
> 
>      +/* Ugh!  Let the firmware tell us the hardware address */
>      +static int mac_address[ETH_ALEN] = { 0, };
>      +module_param_array(mac_address, int, NULL, 0);
>      +
>       static const struct pci_device_id sky2_id_table[] = {
>              { PCI_DEVICE(PCI_VENDOR_ID_SYSKONNECT, 0x9000) }, /* SK-9Sxx */
>              { PCI_DEVICE(PCI_VENDOR_ID_SYSKONNECT, 0x9E00) }, /* SK-9Exx */
>      @@ -4811,13 +4815,21 @@ static struct net_device *sky2_init_netdev(struct sky2_hw *hw, unsigned port,
>              /* try to get mac address in the following order:
>               * 1) from device tree data
>               * 2) from internal registers set by bootloader
>      +        * 3) from the command line parameter
>               */
>              iap = of_get_mac_address(hw->pdev->dev.of_node);
>              if (iap)
>                      memcpy(dev->dev_addr, iap, ETH_ALEN);
>      -       else
>      +       else {
>                      memcpy_fromio(dev->dev_addr, hw->regs + B2_MAC_1 + port * 8,
>                                    ETH_ALEN);
>      +               if (!is_valid_ether_addr(&dev->dev_addr[0])) {
>      +                       int i;
>      +
>      +                       for (i = 0; i < ETH_ALEN; i++)
>      +                               dev->dev_addr[i] = mac_address[i];
>      +               }
>      +       }
> 
>              return dev;
>       }
>      --
>      2.4.6
> 
> References
> 
>    Visible links
>    1. mailto:Liviu.Dudau@arm.com
>    2. mailto:Liviu.Dudau@arm.com
>    3. mailto:ryan.harkin@linaro.org
>    4. mailto:ryan.harkin@linaro.org

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH] sky2: Add module parameter for passing the MAC address

[Francois Romieu]

Liviu Dudau <Liviu.Dudau@arm.com> :
> On Wed, Aug 05, 2015 at 05:40:57PM +0100, Stephen Hemminger wrote:
[...]
> > Yes, I can see that this can be a real problem, and other drivers
> > solve the problem. The standard method is to assign a random mac address
> > (and then let scripts overwrite) rather than introducing module parameter.
> > Module parameters are discouraged because they are device specific.
> > 
> 
> I agree. However, in my case, the boards people have assigned MAC addresses
> to the chip, they just didn't built the board in such a way as to allow one
> to store that MAC address in a permanent way :( And no, I can't use the DT
> because the chip is actually on the PCIe bus.
> 
> Even with the generation of a random address, it still needs to be copied
> into the device, so I would guess that a version of the patch I've sent is
> still relevant?

Assuming a random address is generated, could you elaborate what is needed
that sky2_set_mac_address fails to provide ?

-- 
Ueimor

Re: [PATCH] sky2: Add module parameter for passing the MAC address

[Stephen Hemminger]

Something like this:

Subject: [PATCH net-next] sky2: use random address if EEPROM is bad

On some embedded systems the EEPROM does not contain a valid MAC address.
In that case it is better to fallback to a generated mac address and
let init scripts fix the value later.

Reported-by: Liviu Dudau <Liviu.Dudau@arm.com>
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>


--- a/drivers/net/ethernet/marvell/sky2.c	2015-05-21 15:13:03.621126050 -0700
+++ b/drivers/net/ethernet/marvell/sky2.c	2015-08-05 16:12:38.734534467 -0700
@@ -4819,6 +4819,16 @@ static struct net_device *sky2_init_netd
 		memcpy_fromio(dev->dev_addr, hw->regs + B2_MAC_1 + port * 8,
 			      ETH_ALEN);
 
+	/* if the address is invalid, use a random value */
+	if (!is_valid_ether_addr(dev->dev_addr)) {
+		struct sockaddr sa = { AF_UNSPEC };
+
+		netdev_warn(dev,
+			 "Invalid MAC address defaulting to random\n");
+		sky2_set_mac_address(dev, &sa);
+		dev->addr_assign_type |= NET_ADDR_RANDOM;
+	}
+
 	return dev;
 }
 

Re: [PATCH] sky2: Add module parameter for passing the MAC address

[David Miller]

From: Liviu Dudau <Liviu.Dudau@arm.com>
Date: Wed,  5 Aug 2015 16:50:54 +0100

> For designs where EEPROMs are not connected to PCI Yukon2
> chips we need to get the MAC address from the firmware.
> Add a module parameter called 'mac_address' for this. It
> will be used if no DT node can be found and the B2_MAC
> register holds an invalid value.
> 
> Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>

Sorry, such module options are absolutely not allowed.

If an invalid MAC is present, it should be set to a random
one via eth_random_addr().

Re: [PATCH] sky2: Add module parameter for passing the MAC address

[Florian Fainelli]

On 05/08/15 16:16, Stephen Hemminger wrote:
> Something like this:
> 
> Subject: [PATCH net-next] sky2: use random address if EEPROM is bad
> 
> On some embedded systems the EEPROM does not contain a valid MAC address.
> In that case it is better to fallback to a generated mac address and
> let init scripts fix the value later.
> 
> Reported-by: Liviu Dudau <Liviu.Dudau@arm.com>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> 
> 
> --- a/drivers/net/ethernet/marvell/sky2.c	2015-05-21 15:13:03.621126050 -0700
> +++ b/drivers/net/ethernet/marvell/sky2.c	2015-08-05 16:12:38.734534467 -0700
> @@ -4819,6 +4819,16 @@ static struct net_device *sky2_init_netd
>  		memcpy_fromio(dev->dev_addr, hw->regs + B2_MAC_1 + port * 8,
>  			      ETH_ALEN);
>  
> +	/* if the address is invalid, use a random value */
> +	if (!is_valid_ether_addr(dev->dev_addr)) {
> +		struct sockaddr sa = { AF_UNSPEC };
> +
> +		netdev_warn(dev,
> +			 "Invalid MAC address defaulting to random\n");
> +		sky2_set_mac_address(dev, &sa);
> +		dev->addr_assign_type |= NET_ADDR_RANDOM;

There is a helper for that: eth_hw_addr_random() which sets the
addr_assign_type for you and copies the address to dev->dev_addr.
-- 
Florian

Re: [PATCH] sky2: Add module parameter for passing the MAC address

[Liviu Dudau]

On Thu, Aug 06, 2015 at 01:32:33AM +0100, David Miller wrote:
> From: Liviu Dudau <Liviu.Dudau@arm.com>
> Date: Wed,  5 Aug 2015 16:50:54 +0100
> 
> > For designs where EEPROMs are not connected to PCI Yukon2
> > chips we need to get the MAC address from the firmware.
> > Add a module parameter called 'mac_address' for this. It
> > will be used if no DT node can be found and the B2_MAC
> > register holds an invalid value.
> > 
> > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> 
> Sorry, such module options are absolutely not allowed.

Please clarify if this is a drivers/net/ethernet policy or 
that is true for the larger subsystem. There are a few
networking drivers that have a mac_addr module parameter. I
personally used drivers/net/ethernet/8390/pcnet_cs.c as inspiration
for my patch.

> 
> If an invalid MAC is present, it should be set to a random
> one via eth_random_addr().
> 

Understood. Stephen Hemminger has provided a patch that I can try
and I will report back with the results.

Best regards,
Liviu

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

[PATCH v2 net-next] sky2: use random address if EEPROM is bad

[Liviu Dudau]

On some embedded systems the EEPROM does not contain a valid MAC address.
In that case it is better to fallback to a generated mac address and
let init scripts fix the value later.

Reported-by: Liviu Dudau <Liviu.Dudau@arm.com>
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
[Changed handcoded setup to use eth_hw_addr_random() instead]
Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
---
I have tested this on my Juno platform and I can successfully do an nfsroot boot.

Best regards,
Liviu

 drivers/net/ethernet/marvell/sky2.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/ethernet/marvell/sky2.c b/drivers/net/ethernet/marvell/sky2.c
index d9f4498..c309879 100644
--- a/drivers/net/ethernet/marvell/sky2.c
+++ b/drivers/net/ethernet/marvell/sky2.c
@@ -4819,6 +4819,13 @@ static struct net_device *sky2_init_netdev(struct sky2_hw *hw, unsigned port,
 		memcpy_fromio(dev->dev_addr, hw->regs + B2_MAC_1 + port * 8,
 			      ETH_ALEN);
 
+	/* if the address is invalid, use a random value */
+	if (!is_valid_ether_addr(dev->dev_addr)) {
+		netdev_warn(dev,
+			"Invalid MAC address, defaulting to random\n");
+		eth_hw_addr_random(dev);
+	}
+
 	return dev;
 }
 
-- 
2.4.6

Re: [PATCH v2 net-next] sky2: use random address if EEPROM is bad

[Stephen Hemminger]

On Tue, 11 Aug 2015 15:35:56 +0100
Liviu Dudau <Liviu.Dudau@arm.com> wrote:

> On some embedded systems the EEPROM does not contain a valid MAC address.
> In that case it is better to fallback to a generated mac address and
> let init scripts fix the value later.
> 
> Reported-by: Liviu Dudau <Liviu.Dudau@arm.com>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> [Changed handcoded setup to use eth_hw_addr_random() instead]
> Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> ---
> I have tested this on my Juno platform and I can successfully do an nfsroot boot.
> 
> Best regards,
> Liviu
> 
>  drivers/net/ethernet/marvell/sky2.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/net/ethernet/marvell/sky2.c b/drivers/net/ethernet/marvell/sky2.c
> index d9f4498..c309879 100644
> --- a/drivers/net/ethernet/marvell/sky2.c
> +++ b/drivers/net/ethernet/marvell/sky2.c
> @@ -4819,6 +4819,13 @@ static struct net_device *sky2_init_netdev(struct sky2_hw *hw, unsigned port,
>  		memcpy_fromio(dev->dev_addr, hw->regs + B2_MAC_1 + port * 8,
>  			      ETH_ALEN);
>  
> +	/* if the address is invalid, use a random value */
> +	if (!is_valid_ether_addr(dev->dev_addr)) {
> +		netdev_warn(dev,
> +			"Invalid MAC address, defaulting to random\n");
> +		eth_hw_addr_random(dev);
> +	}
> +
>  	return dev;
>  }
>  

This is not enough, you need to program the hardware with the new random MAC
address. The easiest way is calling sky2_set_mac_address, but you need to convert
the address from array back to sockaddr.

Re: [PATCH v2 net-next] sky2: use random address if EEPROM is bad

[Sergei Shtylyov]

Hello.

On 08/11/2015 05:35 PM, Liviu Dudau wrote:

> On some embedded systems the EEPROM does not contain a valid MAC address.
> In that case it is better to fallback to a generated mac address and
> let init scripts fix the value later.

> Reported-by: Liviu Dudau <Liviu.Dudau@arm.com>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> [Changed handcoded setup to use eth_hw_addr_random() instead]
> Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> ---
> I have tested this on my Juno platform and I can successfully do an nfsroot boot.

> Best regards,
> Liviu

>   drivers/net/ethernet/marvell/sky2.c | 7 +++++++
>   1 file changed, 7 insertions(+)

> diff --git a/drivers/net/ethernet/marvell/sky2.c b/drivers/net/ethernet/marvell/sky2.c
> index d9f4498..c309879 100644
> --- a/drivers/net/ethernet/marvell/sky2.c
> +++ b/drivers/net/ethernet/marvell/sky2.c
> @@ -4819,6 +4819,13 @@ static struct net_device *sky2_init_netdev(struct sky2_hw *hw, unsigned port,
>   		memcpy_fromio(dev->dev_addr, hw->regs + B2_MAC_1 + port * 8,
>   			      ETH_ALEN);
>
> +	/* if the address is invalid, use a random value */
> +	if (!is_valid_ether_addr(dev->dev_addr)) {
> +		netdev_warn(dev,
> +			"Invalid MAC address, defaulting to random\n");

    Please start the continuation line right under 'dev' on the borken up line.

> +		eth_hw_addr_random(dev);
> +	}
> +
>   	return dev;
>   }

MBR, Sergei

Re: [PATCH v2 net-next] sky2: use random address if EEPROM is bad

[Liviu Dudau]

On Tue, Aug 11, 2015 at 07:56:06PM +0100, Sergei Shtylyov wrote:
> Hello.
> 
> On 08/11/2015 05:35 PM, Liviu Dudau wrote:
> 
> > On some embedded systems the EEPROM does not contain a valid MAC address.
> > In that case it is better to fallback to a generated mac address and
> > let init scripts fix the value later.
> 
> > Reported-by: Liviu Dudau <Liviu.Dudau@arm.com>
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > [Changed handcoded setup to use eth_hw_addr_random() instead]
> > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> > ---
> > I have tested this on my Juno platform and I can successfully do an nfsroot boot.
> 
> > Best regards,
> > Liviu
> 
> >   drivers/net/ethernet/marvell/sky2.c | 7 +++++++
> >   1 file changed, 7 insertions(+)
> 
> > diff --git a/drivers/net/ethernet/marvell/sky2.c b/drivers/net/ethernet/marvell/sky2.c
> > index d9f4498..c309879 100644
> > --- a/drivers/net/ethernet/marvell/sky2.c
> > +++ b/drivers/net/ethernet/marvell/sky2.c
> > @@ -4819,6 +4819,13 @@ static struct net_device *sky2_init_netdev(struct sky2_hw *hw, unsigned port,
> >   		memcpy_fromio(dev->dev_addr, hw->regs + B2_MAC_1 + port * 8,
> >   			      ETH_ALEN);
> >
> > +	/* if the address is invalid, use a random value */
> > +	if (!is_valid_ether_addr(dev->dev_addr)) {
> > +		netdev_warn(dev,
> > +			"Invalid MAC address, defaulting to random\n");
> 
>     Please start the continuation line right under 'dev' on the borken up line.

OK, I will make the changes for v3.

> 
> > +		eth_hw_addr_random(dev);
> > +	}
> > +
> >   	return dev;
> >   }
> 
> MBR, Sergei
> 

Thanks for reviewing this!

Best regards,
Liviu

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH v2 net-next] sky2: use random address if EEPROM is bad

[Liviu Dudau]

On Tue, Aug 11, 2015 at 06:01:32PM +0100, Stephen Hemminger wrote:
> On Tue, 11 Aug 2015 15:35:56 +0100
> Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> 
> > On some embedded systems the EEPROM does not contain a valid MAC address.
> > In that case it is better to fallback to a generated mac address and
> > let init scripts fix the value later.
> > 
> > Reported-by: Liviu Dudau <Liviu.Dudau@arm.com>
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > [Changed handcoded setup to use eth_hw_addr_random() instead]
> > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> > ---
> > I have tested this on my Juno platform and I can successfully do an nfsroot boot.
> > 
> > Best regards,
> > Liviu
> > 
> >  drivers/net/ethernet/marvell/sky2.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/net/ethernet/marvell/sky2.c b/drivers/net/ethernet/marvell/sky2.c
> > index d9f4498..c309879 100644
> > --- a/drivers/net/ethernet/marvell/sky2.c
> > +++ b/drivers/net/ethernet/marvell/sky2.c
> > @@ -4819,6 +4819,13 @@ static struct net_device *sky2_init_netdev(struct sky2_hw *hw, unsigned port,
> >  		memcpy_fromio(dev->dev_addr, hw->regs + B2_MAC_1 + port * 8,
> >  			      ETH_ALEN);
> >  
> > +	/* if the address is invalid, use a random value */
> > +	if (!is_valid_ether_addr(dev->dev_addr)) {
> > +		netdev_warn(dev,
> > +			"Invalid MAC address, defaulting to random\n");
> > +		eth_hw_addr_random(dev);
> > +	}
> > +
> >  	return dev;
> >  }
> >  
> 
> This is not enough, you need to program the hardware with the new random MAC
> address. The easiest way is calling sky2_set_mac_address, but you need to convert
> the address from array back to sockaddr.
> 

OK, I am a bit confused as to why sky2_set_mac_address is needed here, as this was not
required by the existing function. Given that in my tests I get a random MAC address
assigned every time to the device and I can see the same MAC address with ifconfig, how
can I test the effect of sky2_set_mac_address if I add it?

Best regards,
Liviu

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH v2 net-next] sky2: use random address if EEPROM is bad

[Stephen Hemminger]

On Wed, 12 Aug 2015 10:30:05 +0100
Liviu Dudau <Liviu.Dudau@arm.com> wrote:

> On Tue, Aug 11, 2015 at 06:01:32PM +0100, Stephen Hemminger wrote:
> > On Tue, 11 Aug 2015 15:35:56 +0100
> > Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> > 
> > > On some embedded systems the EEPROM does not contain a valid MAC address.
> > > In that case it is better to fallback to a generated mac address and
> > > let init scripts fix the value later.
> > > 
> > > Reported-by: Liviu Dudau <Liviu.Dudau@arm.com>
> > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > > [Changed handcoded setup to use eth_hw_addr_random() instead]
> > > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> > > ---
> > > I have tested this on my Juno platform and I can successfully do an nfsroot boot.
> > > 
> > > Best regards,
> > > Liviu
> > > 
> > >  drivers/net/ethernet/marvell/sky2.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/drivers/net/ethernet/marvell/sky2.c b/drivers/net/ethernet/marvell/sky2.c
> > > index d9f4498..c309879 100644
> > > --- a/drivers/net/ethernet/marvell/sky2.c
> > > +++ b/drivers/net/ethernet/marvell/sky2.c
> > > @@ -4819,6 +4819,13 @@ static struct net_device *sky2_init_netdev(struct sky2_hw *hw, unsigned port,
> > >  		memcpy_fromio(dev->dev_addr, hw->regs + B2_MAC_1 + port * 8,
> > >  			      ETH_ALEN);
> > >  
> > > +	/* if the address is invalid, use a random value */
> > > +	if (!is_valid_ether_addr(dev->dev_addr)) {
> > > +		netdev_warn(dev,
> > > +			"Invalid MAC address, defaulting to random\n");
> > > +		eth_hw_addr_random(dev);
> > > +	}
> > > +
> > >  	return dev;
> > >  }
> > >  
> > 
> > This is not enough, you need to program the hardware with the new random MAC
> > address. The easiest way is calling sky2_set_mac_address, but you need to convert
> > the address from array back to sockaddr.
> > 
> 
> OK, I am a bit confused as to why sky2_set_mac_address is needed here, as this was not
> required by the existing function. Given that in my tests I get a random MAC address
> assigned every time to the device and I can see the same MAC address with ifconfig, how
> can I test the effect of sky2_set_mac_address if I add it?

The network device address is stored in two places. One is in the
kernel (dev->dev_addr) and is  used by networking stack.
The other is the hardware (actually two places) and is used filtering received packets
in the PHY and for sending hardware generated pause frames.

When a random address is generated, you need to tell the hardware
to use that address as well. I suspect your hardware maybe limited in functionality
and not do the normal filtering.

Re: [PATCH v2 net-next] sky2: use random address if EEPROM is bad

[Liviu Dudau]

On Wed, Aug 12, 2015 at 04:28:23PM +0100, Stephen Hemminger wrote:
> On Wed, 12 Aug 2015 10:30:05 +0100
> Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> 
> > On Tue, Aug 11, 2015 at 06:01:32PM +0100, Stephen Hemminger wrote:
> > > On Tue, 11 Aug 2015 15:35:56 +0100
> > > Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> > > 
> > > > On some embedded systems the EEPROM does not contain a valid MAC address.
> > > > In that case it is better to fallback to a generated mac address and
> > > > let init scripts fix the value later.
> > > > 
> > > > Reported-by: Liviu Dudau <Liviu.Dudau@arm.com>
> > > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > > > [Changed handcoded setup to use eth_hw_addr_random() instead]
> > > > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> > > > ---
> > > > I have tested this on my Juno platform and I can successfully do an nfsroot boot.
> > > > 
> > > > Best regards,
> > > > Liviu
> > > > 
> > > >  drivers/net/ethernet/marvell/sky2.c | 7 +++++++
> > > >  1 file changed, 7 insertions(+)
> > > > 
> > > > diff --git a/drivers/net/ethernet/marvell/sky2.c b/drivers/net/ethernet/marvell/sky2.c
> > > > index d9f4498..c309879 100644
> > > > --- a/drivers/net/ethernet/marvell/sky2.c
> > > > +++ b/drivers/net/ethernet/marvell/sky2.c
> > > > @@ -4819,6 +4819,13 @@ static struct net_device *sky2_init_netdev(struct sky2_hw *hw, unsigned port,
> > > >  		memcpy_fromio(dev->dev_addr, hw->regs + B2_MAC_1 + port * 8,
> > > >  			      ETH_ALEN);
> > > >  
> > > > +	/* if the address is invalid, use a random value */
> > > > +	if (!is_valid_ether_addr(dev->dev_addr)) {
> > > > +		netdev_warn(dev,
> > > > +			"Invalid MAC address, defaulting to random\n");
> > > > +		eth_hw_addr_random(dev);
> > > > +	}
> > > > +
> > > >  	return dev;
> > > >  }
> > > >  
> > > 
> > > This is not enough, you need to program the hardware with the new random MAC
> > > address. The easiest way is calling sky2_set_mac_address, but you need to convert
> > > the address from array back to sockaddr.
> > > 
> > 
> > OK, I am a bit confused as to why sky2_set_mac_address is needed here, as this was not
> > required by the existing function. Given that in my tests I get a random MAC address
> > assigned every time to the device and I can see the same MAC address with ifconfig, how
> > can I test the effect of sky2_set_mac_address if I add it?
> 
> The network device address is stored in two places. One is in the
> kernel (dev->dev_addr) and is  used by networking stack.
> The other is the hardware (actually two places) and is used filtering received packets
> in the PHY and for sending hardware generated pause frames.
> 
> When a random address is generated, you need to tell the hardware
> to use that address as well. I suspect your hardware maybe limited in functionality
> and not do the normal filtering.
> 

I understand now, thanks for explaining it. I admit my tests were very limited and I did not
try to capture any packets using the interface. I thought the DHCP discovery requires the
MAC address to be correct in the hardware to work, but it might be that the kernel's DHCP
client is more thorough than userspace.

I will add the setting of the MAC address in v3.

Best regards,
Liviu

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯