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(-)
@@ -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 */+staticintmac_address[ETH_ALEN]={0,};+module_param_array(mac_address,int,NULL,0);+staticconststructpci_device_idsky2_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)fromdevicetreedata*2)frominternalregisterssetbybootloader+*3)fromthecommandlineparameter*/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])){+inti;++for(i=0;i<ETH_ALEN;i++)+dev->dev_addr[i]=mac_address[i];+}+}returndev;}
From: Stephen Hemminger <stephen@networkplumber.org> Date: 2015-08-05 16:40:51
On Wed, 5 Aug 2015 16:50:54 +0100
Liviu Dudau [off-list ref] 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.
On Wed, Aug 05, 2015 at 05:40:57PM +0100, Stephen Hemminger wrote:
On Wed, 5 Aug 2015 16:50:54 +0100
Liviu Dudau [off-list ref] wrote:
quoted
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! /
---------------
¯\_(ツ)_/¯
On Wed, Aug 05, 2015 at 05:40:57PM +0100, Stephen Hemminger wrote:
[...]
quoted
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
From: Stephen Hemminger <stephen@networkplumber.org> Date: 2015-08-05 23:16:04
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>
@@ -4819,6 +4819,16 @@ static struct net_device *sky2_init_netdmemcpy_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)){+structsockaddrsa={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;+}+returndev;}
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().
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>
@@ -4819,6 +4819,16 @@ static struct net_device *sky2_init_netdmemcpy_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)){+structsockaddrsa={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
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! /
---------------
¯\_(ツ)_/¯
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(+)
@@ -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);+}+returndev;}
From: Stephen Hemminger <stephen@networkplumber.org> Date: 2015-08-11 17:01:24
On Tue, 11 Aug 2015 15:35:56 +0100
Liviu Dudau [off-list ref] wrote:
quoted hunk
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(+)
@@ -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);+}+returndev;}
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.
From: Sergei Shtylyov <hidden> Date: 2015-08-11 18:56:13
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.
@@ -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.
On Tue, Aug 11, 2015 at 07:56:06PM +0100, Sergei Shtylyov wrote:
Hello.
On 08/11/2015 05:35 PM, Liviu Dudau wrote:
quoted
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.
quoted
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.
@@ -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.
quoted
+ 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! /
---------------
¯\_(ツ)_/¯
On Tue, Aug 11, 2015 at 06:01:32PM +0100, Stephen Hemminger wrote:
On Tue, 11 Aug 2015 15:35:56 +0100
Liviu Dudau [off-list ref] wrote:
quoted
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(+)
@@ -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);+}+returndev;}
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! /
---------------
¯\_(ツ)_/¯
From: Stephen Hemminger <stephen@networkplumber.org> Date: 2015-08-12 15:28:23
On Wed, 12 Aug 2015 10:30:05 +0100
Liviu Dudau [off-list ref] wrote:
On Tue, Aug 11, 2015 at 06:01:32PM +0100, Stephen Hemminger wrote:
quoted
On Tue, 11 Aug 2015 15:35:56 +0100
Liviu Dudau [off-list ref] wrote:
quoted
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(+)
@@ -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);+}+returndev;}
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.
On Wed, Aug 12, 2015 at 04:28:23PM +0100, Stephen Hemminger wrote:
On Wed, 12 Aug 2015 10:30:05 +0100
Liviu Dudau [off-list ref] wrote:
quoted
On Tue, Aug 11, 2015 at 06:01:32PM +0100, Stephen Hemminger wrote:
quoted
On Tue, 11 Aug 2015 15:35:56 +0100
Liviu Dudau [off-list ref] wrote:
quoted
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(+)
@@ -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);+}+returndev;}
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! /
---------------
¯\_(ツ)_/¯