From: Darren Hart <hidden> Date: 2012-01-14 06:45:36
If the MAC is invalid or not implemented, use a randomly generated one rather
than failing the probe. Store the generated addr in a new sw_mac array in the
pch_gbe_mac_info structure. Take care to allow for assigning the MAC via
ifconfig by reusing sw_addr to store an assigned mac if probe populated it with
a random one (otherwise the assignment would rely on the ROM and the reset would
fail to write a valid MAC to the rx filter).
Tested on two platforms, one with a valid MAC, the other without a MAC. The
real MAC is used if present, a randomly generated one otherwise. Both are
capable of changing the MAC with ifconfig. They successfully get an IP over
DHCP and pass a simple ping and login over ssh test.
This does not make any attempt to address a missing or invalid MAC for the
pch_phub driver.
Signed-off-by: Darren Hart <redacted>
CC: Arjan van de Ven <redacted>
CC: Alan Cox <redacted>
CC: Tomoya MORINAGA <redacted>
CC: Jeff Kirsher <redacted> (commit_signer:1/3=33%,commit_signer:1/8=12%)
CC: "David S. Miller" <davem@davemloft.net>
CC: Paul Gortmaker <redacted>
CC: Jon Mason <jdmason@kudzu.us>
CC: netdev@vger.kernel.org
---
drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h | 3 ++
.../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c | 25 ++++++++++++++++++++
2 files changed, 28 insertions(+), 0 deletions(-)
@@ -2444,6 +2456,19 @@ static int pch_gbe_probe(struct pci_dev *pdev,pch_gbe_set_ethtool_ops(netdev);pch_gbe_mac_load_mac_addr(&adapter->hw);++/*+*TrytoreadtheMACaddress.Ifitisinvalid(orjustmissing),+*generatearandomonetousefromhereonout.+*/+pch_gbe_mac_read_mac_addr(&adapter->hw);+if(!is_valid_ether_addr(adapter->hw.mac.addr)){+dev_err(&pdev->dev,"Invalid MAC address, "+"using a random generated one.\n");+random_ether_addr(adapter->hw.mac.sw_addr);+memcpy(adapter->hw.mac.addr,adapter->hw.mac.sw_addr,netdev->addr_len);+}+pch_gbe_mac_reset_hw(&adapter->hw);/* setup the private structure */
From: David Miller <davem@davemloft.net> Date: 2012-01-14 08:14:42
From: Darren Hart <redacted>
Date: Fri, 13 Jan 2012 22:44:55 -0800
If the MAC is invalid or not implemented, use a randomly generated one rather
than failing the probe. Store the generated addr in a new sw_mac array in the
pch_gbe_mac_info structure. Take care to allow for assigning the MAC via
ifconfig by reusing sw_addr to store an assigned mac if probe populated it with
a random one (otherwise the assignment would rely on the ROM and the reset would
fail to write a valid MAC to the rx filter).
Tested on two platforms, one with a valid MAC, the other without a MAC. The
real MAC is used if present, a randomly generated one otherwise. Both are
capable of changing the MAC with ifconfig. They successfully get an IP over
DHCP and pass a simple ping and login over ssh test.
This does not make any attempt to address a missing or invalid MAC for the
pch_phub driver.
Signed-off-by: Darren Hart <redacted>
I don't want to see code like this added if it's "just in case."
Please correct any hardware that hasn't shipped yet or is alpha/beta
hardware in testing, so that we don't need stuff like this.
From: Darren Hart <hidden> Date: 2012-01-14 15:56:08
On 01/14/2012 12:14 AM, David Miller wrote:
From: Darren Hart <redacted>
Date: Fri, 13 Jan 2012 22:44:55 -0800
quoted
If the MAC is invalid or not implemented, use a randomly generated one rather
than failing the probe. Store the generated addr in a new sw_mac array in the
pch_gbe_mac_info structure. Take care to allow for assigning the MAC via
ifconfig by reusing sw_addr to store an assigned mac if probe populated it with
a random one (otherwise the assignment would rely on the ROM and the reset would
fail to write a valid MAC to the rx filter).
Tested on two platforms, one with a valid MAC, the other without a MAC. The
real MAC is used if present, a randomly generated one otherwise. Both are
capable of changing the MAC with ifconfig. They successfully get an IP over
DHCP and pass a simple ping and login over ssh test.
This does not make any attempt to address a missing or invalid MAC for the
pch_phub driver.
Signed-off-by: Darren Hart <redacted>
I don't want to see code like this added if it's "just in case."
I don't disagree, unfortunately it is not "just in case". The Inforce
Goldstein QSeven Module on the Portwell PQ7-C100XL carrier board are
already available and do not implement the MAC EEPROM in hardware.
Please correct any hardware that hasn't shipped yet or is alpha/beta
hardware in testing, so that we don't need stuff like this.
I saw that the use of random_ether_addr is fairly prevalent, and
attempted a roughly similar sort of approach to others I had seen. Do
you consider all of those to be "necessary evils" or are there
legitimate situations for its use?
In any case, with existing hardware out there that is unusable with the
current pch_gbe driver, can we consider this workaround for inclusion?
--
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel
From: David Miller <davem@davemloft.net> Date: 2012-01-14 19:56:14
From: Darren Hart <redacted>
Date: Sat, 14 Jan 2012 07:54:27 -0800
quoted
Please correct any hardware that hasn't shipped yet or is alpha/beta
hardware in testing, so that we don't need stuff like this.
I saw that the use of random_ether_addr is fairly prevalent, and
attempted a roughly similar sort of approach to others I had seen. Do
you consider all of those to be "necessary evils" or are there
legitimate situations for its use?
In any case, with existing hardware out there that is unusable with the
current pch_gbe driver, can we consider this workaround for inclusion?
I fear that people are just going to add this random MAC stuff way too
easily, it's a spreading disease.
Ship functional hardware instead.
From: Joe Perches <joe@perches.com> Date: 2012-01-14 20:29:28
On Sat, 2012-01-14 at 11:56 -0800, David Miller wrote:
From: Darren Hart <redacted>
Date: Sat, 14 Jan 2012 07:54:27 -0800
quoted
In any case, with existing hardware out there that is unusable with the
current pch_gbe driver, can we consider this workaround for inclusion?
I fear that people are just going to add this random MAC stuff way too
easily, it's a spreading disease.
Ship functional hardware instead.
Good advice, but that's _hard_.
Working systems are nearly always some combination
of hardware/firmware/software defect workarounds.
Anyway, perhaps setting random_ether_addr like this
should be in some generic routine with a standardized
"avoid buying bad hardware" output logging message.
From: Darren Hart <hidden> Date: 2012-01-14 22:36:30
On 01/14/2012 01:46 PM, Alan Cox wrote:
quoted
I fear that people are just going to add this random MAC stuff way too
easily, it's a spreading disease.
Ship functional hardware instead.
See "choir, preaching to the".
The stuff is out there and it's not Darren's fault !
Alan
So perhaps I should provide some more context into why I'm sending these
patches. The following hardware is currently available and the existing
Linux support is confined to a Timesys Fedora-Based pre-installed image
which requires user intervention to write a MAC using an old, not
upstreamed, modified ioh_gbe_mac driver.
The board documentation is available here:
http://www.inforcecomputing.com/SYS940X_ECX.html
In particular, see:
http://www.inforcecomputing.com/proddls/SYS940X-01_UserGuide_001329.pdf
and:
http://www.inforcecomputing.com/proddls/BLDK2_Kern_2.6.29-10_and_2.6.29-12_for_SYS940X-1_1304.pdf
I felt this patch made this hardware more accessible by allowing it to
work with current kernels without ugly userspace hacks. It is also
following precedent set by existing drivers.
So while I completely agree with the sentiment "Ship functional
hardware", this wasn't a product I was involved with, I'm just trying to
make a bad situation better. As Alan alludes to above, I do actually
spend a good deal of time trying to improve hardware to avoid this kind
of thing (it's an amazingly difficult task).
--
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel
I have since resolved this particular issue. I did not disable the
pcieqos driver I forward ported. With that disabled, pch_phub works as
expected.
Yes, you can not use both pcieqos and pch_phub at the same time.
Because pcieqos is previous version of pch_phub which is upstreamed.
Which is to say it lists pch_mac, reads all 0's, and does
nothing on write (since the MAC ROM doesn't exist). Please see the patch
thread from Friday to address this using a random mac if the ROM is
missing or invalid.
Saving MAC address into external ROM is generic method, I think.
Though I know the ROM-less system using eg20t-pch, however I think
this system is not common.
So, I think pch_gbe shouldn't have auto-mac address assignment.
BTW, as you know, a use can write MAC address using sysfs file system
like below.
echo aa:bb:cc:dd:ee:ff > pch_mac
thanks,
tomoya
Saving MAC address into external ROM is generic method, I think.
Though I know the ROM-less system using eg20t-pch, however I think
this system is not common.
So, I think pch_gbe shouldn't have auto-mac address assignment.
The problem is the module load fails for those cases. You cannot load
the module and use the standard ifconfig eth0 hw aa:bb:cc:dd:ee:ff
interface. The better fix might be to make sure it loads.
So change from
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\n");
ret = -EIO;
goto err_free_adapter;
}
to just printing a warning, and check the current address when a user
tries to ifconfig it up and refuse to allow the port to go active.
Alan
From: Nick Bowler <hidden> Date: 2012-01-16 15:22:45
On 2012-01-14 00:15 -0800, David Miller wrote:
From: Darren Hart <redacted>
Date: Fri, 13 Jan 2012 22:44:55 -0800
quoted
CC: Jeff Kirsher <redacted> (commit_signer:1/3=33%,commit_signer:1/8=12%)
BTW, please try to figure out where this "commit_signer" stuff came from.
It ended up corrupting the CC: list of your posting too.
While it ended up on the CC line, the result is still a valid and
correct email address per RFC 822.
Cheers,
--
Nick Bowler, Elliptic Technologies (http://www.elliptictech.com/)
From: Darren Hart <hidden> Date: 2012-01-16 15:38:30
On 01/15/2012 11:38 PM, Tomoya MORINAGA wrote:
2012/1/16 Darren Hart [off-list ref]:
quoted
I have since resolved this particular issue. I did not disable the
pcieqos driver I forward ported. With that disabled, pch_phub works as
expected.
Yes, you can not use both pcieqos and pch_phub at the same time.
Because pcieqos is previous version of pch_phub which is upstreamed.
Right, they both claim the same PCI ID.
quoted
Which is to say it lists pch_mac, reads all 0's, and does
nothing on write (since the MAC ROM doesn't exist). Please see the patch
thread from Friday to address this using a random mac if the ROM is
missing or invalid.
Saving MAC address into external ROM is generic method, I think.
Though I know the ROM-less system using eg20t-pch, however I think
this system is not common.
So, I think pch_gbe shouldn't have auto-mac address assignment.
BTW, as you know, a use can write MAC address using sysfs file system
like below.
echo aa:bb:cc:dd:ee:ff > pch_mac
Right, this doesn't work on the ROM-less system. At least, the
subsequent read returns all 0's. The same is true with the phub-util-mac
and pcieqos. I believe this is due to pci_map_rom failing.
Also, if you don't build the driver as a module, then the above still
isn't sufficient as the pci probe fails and the device isn't created.
--
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel
From: Darren Hart <hidden> Date: 2012-01-16 15:43:23
On 01/16/2012 04:31 AM, Alan Cox wrote:
quoted
Saving MAC address into external ROM is generic method, I think.
Though I know the ROM-less system using eg20t-pch, however I think
this system is not common.
So, I think pch_gbe shouldn't have auto-mac address assignment.
The problem is the module load fails for those cases. You cannot load
the module and use the standard ifconfig eth0 hw aa:bb:cc:dd:ee:ff
interface. The better fix might be to make sure it loads.
So change from
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\n");
ret = -EIO;
goto err_free_adapter;
}
to just printing a warning, and check the current address when a user
tries to ifconfig it up and refuse to allow the port to go active.
I can go this route I suppose. I don't really understand the objection
to the use of a random mac addr in the special case given the prevalence
of this approach within existing drivers.
One reason I don't care for this alternative approach is that this
particular hardware is targeted at embedded use where we can't assume a
full init system is available, etc. It can be made to work of course, it
just isn't as automated.
David, would you orefer/accept an alternative patch which allows the
driver to load without a MAC address so the user can set it via ifconfig
after boot?
--
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel
From: Arjan van de Ven <hidden> Date: 2012-01-16 16:07:48
On 1/16/2012 7:42 AM, Darren Hart wrote:
One reason I don't care for this alternative approach is that this
particular hardware is targeted at embedded use where we can't assume a
full init system is available, etc. It can be made to work of course, it
just isn't as automated.
the tricky thing with embedded hw like this is that all devices might
end up with the same, read-only filesystem, so storing the mac on the FS
and then loading it from there into the HW is... suboptimal.
Would be very nice if busybox had a command that would check the mac
from each IF, and created the random mac from userspace automatically...
From: David Laight <hidden> Date: 2012-01-16 16:20:52
the tricky thing with embedded hw like this is that all devices might
end up with the same, read-only filesystem, so storing the
mac on the FS
and then loading it from there into the HW is... suboptimal.
Would be very nice if busybox had a command that would check the mac
from each IF, and created the random mac from userspace
automatically...
Since multiple interfaces on a single system are unlikely
to be connected to the same LAN segment, it doesn't really
matter if they use same MAC address.
For a long time sun solaris systems used the same MAC
address (based on the system id) on all their ethernet
interfaces.
But yes, you don't want a 'random' number generator that
might give the same value for cloned systems.
You also really want a 'manufacturer id' for 'random address'
so that they can't collide with anuything using real addresses.
David
From: Arjan van de Ven <hidden> Date: 2012-01-16 16:35:16
On 1/16/2012 8:20 AM, David Laight wrote:
quoted
the tricky thing with embedded hw like this is that all devices might
end up with the same, read-only filesystem, so storing the
mac on the FS
and then loading it from there into the HW is... suboptimal.
Would be very nice if busybox had a command that would check the mac
from each IF, and created the random mac from userspace
automatically...
Since multiple interfaces on a single system are unlikely
to be connected to the same LAN segment, it doesn't really
matter if they use same MAC address.
I think you missed the point. All embedded devices have the same fs, so
if you have 2 boxes of the same model/brand on the same network, they'd
have the same MAC. That's generally frowned upon by network
administrators ;-)
From: Mark Brown <hidden> Date: 2012-01-16 16:44:48
On Mon, Jan 16, 2012 at 08:35:09AM -0800, Arjan van de Ven wrote:
On 1/16/2012 8:20 AM, David Laight wrote:
quoted
Since multiple interfaces on a single system are unlikely
to be connected to the same LAN segment, it doesn't really
matter if they use same MAC address.
I think you missed the point. All embedded devices have the same fs, so
if you have 2 boxes of the same model/brand on the same network, they'd
have the same MAC. That's generally frowned upon by network
administrators ;-)
This is generally handled by putting the MAC into a board-specific bit
of flash that isn't part the main firmware (for example, the bootloader
configuration) - for a lot of applications random generation doesn't
help that much as entropy is hard to come by.
From: Rick Jones <hidden> Date: 2012-01-16 21:06:11
On 01/16/2012 08:20 AM, David Laight wrote:
Since multiple interfaces on a single system are unlikely
to be connected to the same LAN segment, it doesn't really
matter if they use same MAC address.
For a long time sun solaris systems used the same MAC
address (based on the system id) on all their ethernet
interfaces.
And there was a perhaps small, but certainly non-trivial stream of
people asking how to change that because their system(s) were indeed
multiple ports connected to the same broadcast domain.
Long since then Sun changed their defaults, and we've gotten things like
bonding, so I think any assumption that a system with multiple ports
will not have then connected to the same broadcast domain is brittle at
best.
rick jones
From: Darren Hart <hidden> Date: 2012-01-16 23:07:28
On 01/16/2012 03:04 PM, David Miller wrote:
From: Nick Bowler <redacted>
Date: Mon, 16 Jan 2012 10:22:39 -0500
quoted
On 2012-01-14 00:15 -0800, David Miller wrote:
quoted
From: Darren Hart <redacted>
Date: Fri, 13 Jan 2012 22:44:55 -0800
quoted
CC: Jeff Kirsher <redacted> (commit_signer:1/3=33%,commit_signer:1/8=12%)
BTW, please try to figure out where this "commit_signer" stuff came from.
It ended up corrupting the CC: list of your posting too.
While it ended up on the CC line, the result is still a valid and
correct email address per RFC 822.
I didn't say it wasn't an "RFC compliant" email address, I just said
it's complete garbage.
Indeed. An oversight on my part and I've addressed it. Apologies for the
noise.
--
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel