Patchwork pch_gbe: Use a randomly generated MAC instead of failing probe

login
register
mail settings
Submitter Darren Hart
Date Jan. 14, 2012, 6:44 a.m.
Message ID <132d2a41a089905de3147b4656e350608aa7fd6f.1326523495.git.dvhart@linux.intel.com>
Download mbox | patch
Permalink /patch/136063/
State Rejected
Delegated to: David Miller
Headers show

Comments

Darren Hart - Jan. 14, 2012, 6:44 a.m.
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 <dvhart@linux.intel.com>
CC: Arjan van de Ven <arjan@linux.intel.com>
CC: Alan Cox <alan@linux.intel.com>
CC: Tomoya MORINAGA <tomoya.rohm@gmail.com>
CC: Jeff Kirsher <jeffrey.t.kirsher@intel.com> (commit_signer:1/3=33%,commit_signer:1/8=12%)
CC: "David S. Miller" <davem@davemloft.net>
CC: Paul Gortmaker <paul.gortmaker@windriver.com>
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(-)
David Miller - Jan. 14, 2012, 8:14 a.m.
From: Darren Hart <dvhart@linux.intel.com>
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 <dvhart@linux.intel.com>

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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Jan. 14, 2012, 8:15 a.m.
From: Darren Hart <dvhart@linux.intel.com>
Date: Fri, 13 Jan 2012 22:44:55 -0800

> CC: Jeff Kirsher <jeffrey.t.kirsher@intel.com> (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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
WANG Cong - Jan. 14, 2012, 8:18 a.m.
On 01/14/2012 04:15 PM, David Miller wrote:
> From: Darren Hart<dvhart@linux.intel.com>
> Date: Fri, 13 Jan 2012 22:44:55 -0800
>
>> CC: Jeff Kirsher<jeffrey.t.kirsher@intel.com>  (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.

It is from `./scripts/get_maintainer.pl`, and can be suppressed by 
adding '--norolestats' option. :)
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Darren Hart - Jan. 14, 2012, 3:45 p.m.
On 01/14/2012 12:15 AM, David Miller wrote:
> From: Darren Hart <dvhart@linux.intel.com>
> Date: Fri, 13 Jan 2012 22:44:55 -0800
> 
>> CC: Jeff Kirsher <jeffrey.t.kirsher@intel.com> (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.

Sorry about that, I caught it too late.
Darren Hart - Jan. 14, 2012, 3:54 p.m.
On 01/14/2012 12:14 AM, David Miller wrote:
> From: Darren Hart <dvhart@linux.intel.com>
> 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 <dvhart@linux.intel.com>
> 
> 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?
David Miller - Jan. 14, 2012, 7:56 p.m.
From: Darren Hart <dvhart@linux.intel.com>
Date: Sat, 14 Jan 2012 07:54:27 -0800

>> 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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joe Perches - Jan. 14, 2012, 8:29 p.m.
On Sat, 2012-01-14 at 11:56 -0800, David Miller wrote:
> From: Darren Hart <dvhart@linux.intel.com>
> Date: Sat, 14 Jan 2012 07:54:27 -0800
> > 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.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Cox - Jan. 14, 2012, 9:46 p.m.
> 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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Darren Hart - Jan. 14, 2012, 10:36 p.m.
On 01/14/2012 01:46 PM, Alan Cox wrote:
>> 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).
Tomoya MORINAGA - Jan. 16, 2012, 7:38 a.m.
2012/1/16 Darren Hart <darren.hart@intel.com>:
> 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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Cox - Jan. 16, 2012, 12:31 p.m.
> 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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nick Bowler - Jan. 16, 2012, 3:22 p.m.
On 2012-01-14 00:15 -0800, David Miller wrote:
> From: Darren Hart <dvhart@linux.intel.com>
> Date: Fri, 13 Jan 2012 22:44:55 -0800
> 
> > CC: Jeff Kirsher <jeffrey.t.kirsher@intel.com> (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,
Darren Hart - Jan. 16, 2012, 3:38 p.m.
On 01/15/2012 11:38 PM, Tomoya MORINAGA wrote:
> 2012/1/16 Darren Hart <darren.hart@intel.com>:
>> 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.

> 
>> 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 - Jan. 16, 2012, 3:42 p.m.
On 01/16/2012 04:31 AM, Alan Cox wrote:
>> 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?
Arjan van de Ven - Jan. 16, 2012, 4:07 p.m.
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...

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Laight - Jan. 16, 2012, 4:20 p.m.
> 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


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arjan van de Ven - Jan. 16, 2012, 4:35 p.m.
On 1/16/2012 8:20 AM, David Laight wrote:
>  
>> 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 ;-)
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown - Jan. 16, 2012, 4:44 p.m.
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:

> > 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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Cox - Jan. 16, 2012, 5:02 p.m.
> bootloader configuration) - for a lot of applications random
> generation doesn't help that much as entropy is hard to come by.

This is an x86 platform - plenty of entropy from the tsc etc.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rick Jones - Jan. 16, 2012, 9:06 p.m.
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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Jan. 16, 2012, 11:04 p.m.
From: Nick Bowler <nbowler@elliptictech.com>
Date: Mon, 16 Jan 2012 10:22:39 -0500

> On 2012-01-14 00:15 -0800, David Miller wrote:
>> From: Darren Hart <dvhart@linux.intel.com>
>> Date: Fri, 13 Jan 2012 22:44:55 -0800
>> 
>> > CC: Jeff Kirsher <jeffrey.t.kirsher@intel.com> (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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Darren Hart - Jan. 16, 2012, 11:07 p.m.
On 01/16/2012 03:04 PM, David Miller wrote:
> From: Nick Bowler <nbowler@elliptictech.com>
> Date: Mon, 16 Jan 2012 10:22:39 -0500
> 
>> On 2012-01-14 00:15 -0800, David Miller wrote:
>>> From: Darren Hart <dvhart@linux.intel.com>
>>> Date: Fri, 13 Jan 2012 22:44:55 -0800
>>>
>>>> CC: Jeff Kirsher <jeffrey.t.kirsher@intel.com> (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.

Patch

diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h
index a09a071..3a451a9 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h
@@ -356,6 +356,8 @@  struct pch_gbe_functions {
 /**
  * struct pch_gbe_mac_info - MAC information
  * @addr[6]:		Store the MAC address
+ * @sw_addr[6]:		Store a random or specified MAC address if the
+ *			ROM is invalid or missing.
  * @fc:			Mode of flow control
  * @fc_autoneg:		Auto negotiation enable for flow control setting
  * @tx_fc_enable:	Enable flag of Transmit flow control
@@ -367,6 +369,7 @@  struct pch_gbe_functions {
  */
 struct pch_gbe_mac_info {
 	u8 addr[6];
+	u8 sw_addr[6];
 	u8 fc;
 	u8 fc_autoneg;
 	u8 tx_fc_enable;
diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
index 964e9c0..6453a71 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
@@ -116,6 +116,16 @@  s32 pch_gbe_mac_read_mac_addr(struct pch_gbe_hw *hw)
 {
 	u32  adr1a, adr1b;
 
+	/*
+	 * If we generated a random mac address during probe, then the one in
+	 * the ROM is either invalid or missing, use the generated one instead.
+	 */
+	if (is_valid_ether_addr(hw->mac.sw_addr)) {
+		memcpy(hw->mac.addr, hw->mac.sw_addr, 6);
+		pr_debug("hw->mac.addr : %pM (using random generated addr)\n", hw->mac.addr);
+		return 0;
+	}
+
 	adr1a = ioread32(&hw->reg->mac_adr[0].high);
 	adr1b = ioread32(&hw->reg->mac_adr[0].low);
 
@@ -2036,6 +2046,8 @@  static int pch_gbe_set_mac(struct net_device *netdev, void *addr)
 		ret_val = -EADDRNOTAVAIL;
 	} else {
 		memcpy(netdev->dev_addr, skaddr->sa_data, netdev->addr_len);
+		if (is_valid_ether_addr(adapter->hw.mac.sw_addr))
+			memcpy(adapter->hw.mac.sw_addr, skaddr->sa_data, netdev->addr_len);
 		memcpy(adapter->hw.mac.addr, skaddr->sa_data, netdev->addr_len);
 		pch_gbe_mac_mar_set(&adapter->hw, adapter->hw.mac.addr, 0);
 		ret_val = 0;
@@ -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);
+
+	/*
+	 * Try to read the MAC address. If it is invalid (or just missing),
+	 * generate a random one to use from here on out.
+	 */
+	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 */