Add eeprom_bad_csum_allow module option to e1000.

When the EEPROM gets corrupted, you can fix it with ethtool, but only if the module loads and creates a network device. But, without this option, if the EEPROM is corrupted, the driver will not create a network device.

Signed-off-by Adam Jackson <[email protected]>

---
 drivers/net/e1000/e1000_main.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

drivers/net/e1000/e1000_main.c +8 -2

index f1ce348..b308c32 100644
@@ -255,6 +255,10 @@ static int debug = NETIF_MSG_DRV | NETIF_MSG_PROBE;
 module_param(debug, int, 0);
 MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)");
 
+static int eeprom_bad_csum_allow = 0;
+module_param(eeprom_bad_csum_allow, int, 0);
+MODULE_PARM_DESC(eeprom_bad_csum_allow, "Allow bad eeprom checksums");
+
 /**
  * e1000_init_module - Driver Registration Routine
  *

@@ -1012,7 +1016,8 @@ e1000_probe(struct pci_dev *pdev,
 
 	if (e1000_validate_eeprom_checksum(&adapter->hw) < 0) {
 		DPRINTK(PROBE, ERR, "The EEPROM Checksum Is Not Valid\n");
-		goto err_eeprom;
+		if (!eeprom_bad_csum_allow)
+			goto err_eeprom;
 	}
 
 	/* copy the MAC address out of the EEPROM */

@@ -1024,7 +1029,8 @@ e1000_probe(struct pci_dev *pdev,
 
 	if (!is_valid_ether_addr(netdev->perm_addr)) {
 		DPRINTK(PROBE, ERR, "Invalid MAC Address\n");
-		goto err_eeprom;
+		if (!eeprom_bad_csum_allow)
+			goto err_eeprom;
 	}
 
 	e1000_get_bus_info(&adapter->hw);

Quoted 4 lines · Adam Jackson

When the EEPROM gets corrupted, you can fix it with ethtool, but only if the module loads and creates a network device. But, without this option, if the EEPROM is corrupted, the driver will not create a network device.

Quoted 1 line

Signed-off-by Adam Jackson <[email protected]>

NAK

wrong list, not sent to me, and while for e100 I was OK with this patch, for e1000 it really does not make sense to 'just allow' a bad checksum - if your eeprom is randomly messed up then you cannot just fix it like this anyway.

Quoted 6 lines · Auke

Quoted 5 lines · Adam Jackson

When the EEPROM gets corrupted, you can fix it with ethtool, but only if the module loads and creates a network device. But, without this option, if the EEPROM is corrupted, the driver will not create a network device.

Signed-off-by Adam Jackson <[email protected]>

Quoted 5 lines

NAK

wrong list, not sent to me, and while for e100 I was OK with this patch, for e1000 it really does not make sense to 'just allow' a bad checksum - if your eeprom is randomly messed up then you cannot just fix it like this anyway.

That's strange, I managed to recover an otherwise horked e1000 with it. What should I have done instead?

Quoted 6 lines · Adam Jackson

Quoted 5 lines · Auke

Quoted 4 lines · Adam Jackson

When the EEPROM gets corrupted, you can fix it with ethtool, but only if the module loads and creates a network device. But, without this option, if the EEPROM is corrupted, the driver will not create a network device.

Quoted 8 lines

Quoted 6 lines

Quoted 1 line

Signed-off-by Adam Jackson <[email protected]>

NAK

wrong list, not sent to me, and while for e100 I was OK with this patch, for e1000 it really does not make sense to 'just allow' a bad checksum - if your eeprom is randomly messed up then you cannot just fix it like this anyway.

That's strange, I managed to recover an otherwise horked e1000 with it. What should I have done instead?

Dump the eeprom and send us a copy, plus any and all information to the card, system etc.. I realize that you need the patch to actually create it but the danger is that people will start using it *without* troubleshooting the real issue. In various systems the eeprom checksum failure is actually due to a misconfigured powersavings feature and the checksum is really not bad at all, but the card just reports random values.

In any case, this patch should not be merged. We often send it around to users to debug their issue in case it involves eeproms, but merging it will just conceal the real issue and all of a sudden a flood of people stop reporting *real* issues to us.

Quoted 34 lines · Auke

Quoted 12 lines · Adam Jackson

Quoted 11 lines · Auke

Quoted 8 lines · Adam Jackson

When the EEPROM gets corrupted, you can fix it with ethtool, but only if
the module loads and creates a network device. But, without this option,
if the EEPROM is corrupted, the driver will not create a network device.

Signed-off-by Adam Jackson <[email protected]>

NAK

Quoted 7 lines

Quoted 5 lines

wrong list, not sent to me, and while for e100 I was OK with this patch, for e1000
it really does not make sense to 'just allow' a bad checksum - if your eeprom is
randomly messed up then you cannot just fix it like this anyway.

That's strange, I managed to recover an otherwise horked e1000 with it. What should I have done instead?

Dump the eeprom and send us a copy, plus any and all information to the card,
system etc.. I realize that you need the patch to actually create it but the
danger is that people will start using it *without* troubleshooting the real
issue. In various systems the eeprom checksum failure is actually due to a
misconfigured powersavings feature and the checksum is really not bad at all, but
the card just reports random values.

Quoted 7 lines

In any case, this patch should not be merged. We often send it around to users to
debug their issue in case it involves eeproms, but merging it will just conceal
the real issue and all of a sudden a flood of people stop reporting *real* issues
to us.

Sorry, I disagree. Just as with e100, if there is a clear way the user can recover their setup -- and Adam says his was effective -- I don't see why we should be denying users the ability to use their own hardware.

We help everyone out, and if you merge this patch you will prevent users from getting to us for support in the first place.

Quoted 4 lines

In any case, this patch should not be merged. We often send it around to users to debug their issue in case it involves eeproms, but merging it will just conceal the real issue and all of a sudden a flood of people stop reporting *real* issues to us.

Sorry, I disagree. Just as with e100, if there is a clear way the user can recover their setup -- and Adam says his was effective -- I don't see why we should be denying users the ability to use their own hardware.

People aren't going to report this as a bug. They aren't going to try out patches, they're going to do what I did and stick another network card in the box and go on with life.

Our users deserve better than this.

Indeed. This is a common enough problem that not including it causes more pain than its worth. I have two affected boxes myself that I actually thought the hardware was dead before I tried ajax's patch.

People aren't going to report this as a bug. They aren't going to try out patches, they're going to do what I did and stick another network card in the box and go on with life.

Our users deserve better than this.

From: Dave Jones <[email protected]>
Date: Tue, 23 Oct 2007 17:20:26 -0400

Quoted 8 lines

Indeed. This is a common enough problem that not including it causes more pain than its worth. I have two affected boxes myself that I actually thought the hardware was dead before I tried ajax's patch.

People aren't going to report this as a bug. They aren't going to try out patches, they're going to do what I did and stick another network card in the box and go on with life.

Quoted 1 line

Our users deserve better than this.

Seconded. The resistence to this patch is just flat-out rediculious, just like it was in the e100 case.

And I think all of this "e1000 is different!" talk is merely a scarecrow for the fact that Intel simply doesn't want this patch merged for some other reason.

Actually, looking over the code I see obvious bugs in the logic:

An invalid ethernet address should not cause device loading to fail, because the user is given the opportunity to supply a MAC address via userspace (ifconfig or whatever) before the interface goes up.

I just created the attached -bug fix- patch as illustration, though I have not committed it, waiting for comment.

This patch will make no difference for users hitting invalid-eep-csum rather than invalid-MAC-addr condition, but it's a problem I noticed while reviewing Adam's patch in detail.

I'm wondering if there is a way to avoid adding

	if (!is_valid_ether_addr(dev->dev_addr))
		return -EINVAL;

to every ethernet driver's ->open() hook.

From: Jeff Garzik <[email protected]>
Date: Tue, 23 Oct 2007 21:03:36 -0400

Quoted 6 lines

I'm wondering if there is a way to avoid adding

	if (!is_valid_ether_addr(dev->dev_addr))
		return -EINVAL;

to every ethernet driver's ->open() hook.

The first idea I get is:

1) Create netdev->validate_dev_addr().

2) If it exists, invoke it before ->open(), abort
and return if any errors signaled.

etherdev init hooks up a function that does the above check, which allows us to avoid editing every ethernet driver

What do you think?

Quoted 17 lines · David Miller

From: Jeff Garzik <[email protected]>
Date: Tue, 23 Oct 2007 21:03:36 -0400

Quoted 6 lines

I'm wondering if there is a way to avoid adding

	if (!is_valid_ether_addr(dev->dev_addr))
		return -EINVAL;

to every ethernet driver's ->open() hook.

The first idea I get is:

1) Create netdev->validate_dev_addr().

2) If it exists, invoke it before ->open(), abort
and return if any errors signaled.

Quoted 5 lines

etherdev init hooks up a function that does the above check, which allows us to avoid editing every ethernet driver

What do you think?

Seems sane to me. Something like this (attached)?

From: Jeff Garzik <[email protected]>
Date: Tue, 23 Oct 2007 22:20:30 -0400

Quoted 21 lines

Quoted 20 lines · David Miller

From: Jeff Garzik <[email protected]>
Date: Tue, 23 Oct 2007 21:03:36 -0400

Quoted 6 lines

I'm wondering if there is a way to avoid adding

	if (!is_valid_ether_addr(dev->dev_addr))
		return -EINVAL;

to every ethernet driver's ->open() hook.

The first idea I get is:

1) Create netdev->validate_dev_addr().

2) If it exists, invoke it before ->open(), abort
and return if any errors signaled.

etherdev init hooks up a function that does the above check, which allows us to avoid editing every ethernet driver

Quoted 2 lines

Quoted 1 line

What do you think?

Seems sane to me. Something like this (attached)?

Looks great:

Acked-by David S. Miller <[email protected]>

Quoted 26 lines · David Miller

From: Jeff Garzik <[email protected]>
Date: Tue, 23 Oct 2007 22:20:30 -0400

Quoted 23 lines

Quoted 13 lines · David Miller

From: Jeff Garzik <[email protected]>
Date: Tue, 23 Oct 2007 21:03:36 -0400

Quoted 6 lines

I'm wondering if there is a way to avoid adding

	if (!is_valid_ether_addr(dev->dev_addr))
		return -EINVAL;

to every ethernet driver's ->open() hook.

The first idea I get is:

1) Create netdev->validate_dev_addr().

Quoted 8 lines

2) If it exists, invoke it before ->open(), abort
and return if any errors signaled.

etherdev init hooks up a function that does the above check, which allows us to avoid editing every ethernet driver

What do you think?

Seems sane to me. Something like this (attached)?

Quoted 3 lines

Looks great:

Acked-by David S. Miller <[email protected]>

I like it.

Should I start sending patches to remove the checks from e1000/e1000e/ixgb/ixgbe already (to David, I assume?)?

How about:

static int eth_validate_addr(const struct net_device *dev)

{
	return is_valid_ether_addr(dev->dev_addr) ? 0 : -EINVAL;
}

Actually, looking over the code I see obvious bugs in the logic:

An invalid ethernet address should not cause device loading to fail, because the user is given the opportunity to supply a MAC address via userspace (ifconfig or whatever) before the interface goes up.

I just created the attached -bug fix- patch as illustration, though I have not committed it, waiting for comment.
...

Jeff

Quoted 5 lines · Jeff Garzik

Quoted 4 lines

In any case, this patch should not be merged. We often send it around to users to debug their issue in case it involves eeproms, but merging it will just conceal the real issue and all of a sudden a flood of people stop reporting *real* issues to us.

Quoted 3 lines

Sorry, I disagree. Just as with e100, if there is a clear way the user can recover their setup -- and Adam says his was effective -- I don't see why we should be denying users the ability to use their own hardware.

Indeed. This is a common enough problem that not including it causes more pain than its worth. I have two affected boxes myself that I actually thought the hardware was dead before I tried ajax's patch.

Quoted 7 lines · Dave Jones

Quoted 5 lines · Jeff Garzik

Quoted 4 lines

In any case, this patch should not be merged. We often send it around to users to debug their issue in case it involves eeproms, but merging it will just conceal the real issue and all of a sudden a flood of people stop reporting *real* issues to us.

Quoted 7 lines

Quoted 3 lines

Sorry, I disagree. Just as with e100, if there is a clear way the user can recover their setup -- and Adam says his was effective -- I don't see why we should be denying users the ability to use their own hardware.

Indeed. This is a common enough problem that not including it causes more pain than its worth. I have two affected boxes myself that I actually thought the hardware was dead before I tried ajax's patch.

look: You should have reported this to us and you didn't. Now you are using the fact that you did not report it as an argument which is out of place.

why do you say it is common? how often have you seen this and not reported it back to our support? are you willingly trying to frustrate this issue?

Auke

Quoted 11 lines · Dave Jones

Quoted 8 lines · Jeff Garzik

Quoted 4 lines

In any case, this patch should not be merged. We often send it around to users to debug their issue in case it involves eeproms, but merging it will just conceal the real issue and all of a sudden a flood of people stop reporting *real* issues to us.

Sorry, I disagree. Just as with e100, if there is a clear way the user can recover their setup -- and Adam says his was effective -- I don't see why we should be denying users the ability to use their own hardware.

Quoted 3 lines

Indeed. This is a common enough problem that not including it causes more pain than its worth. I have two affected boxes myself that I actually thought the hardware was dead before I tried ajax's patch.

look: You should have reported this to us and you didn't. Now you are using the fact that you did not report it as an argument which is out of place.

why do you say it is common? how often have you seen this and not reported it back to our support? are you willingly trying to frustrate this issue?

Sorry, I disagree. Just as with e100, if there is a clear way the user can recover their setup -- and Adam says his was effective -- I don't see why we should be denying users the ability to use their own hardware.