diff mbox

[v2] igb: Add I210 cable fault detection to self test

Message ID 27181503.100057.1449171821628.JavaMail.zimbra@xes-inc.com
State Superseded
Delegated to: Jeff Kirsher
Headers show

Commit Message

Aaron Sierra Dec. 3, 2015, 7:43 p.m. UTC
From: Joe Schultz <jschultz@xes-inc.com>

Add an offline diagnostic test for the I210 internal PHY which checks
for cable faults and reports the distance along the cable where the
fault was detected. Fault types detected include open, short, and
cross-pair short.

Signed-off-by: Joe Schultz <jschultz@xes-inc.com>
Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
---
 v2 - account for changes made by this patch in dev-queue:
   drivers/net: get rid of unnecessary initializations in .get_drvinfo()

 drivers/net/ethernet/intel/igb/e1000_defines.h |  13 +-
 drivers/net/ethernet/intel/igb/igb_ethtool.c   | 189 ++++++++++++++++++++++++-
 2 files changed, 197 insertions(+), 5 deletions(-)

Comments

Brown, Aaron F Dec. 11, 2015, 3:21 a.m. UTC | #1
> From: Intel-wired-lan [intel-wired-lan-bounces@lists.osuosl.org] on behalf of Aaron Sierra [asierra@xes-inc.com]
> Sent: Thursday, December 03, 2015 11:43 AM
> To: Kirsher, Jeffrey T; intel-wired-lan@lists.osuosl.org
> Cc: Matthew Vick; Joe Schultz
> Subject: [Intel-wired-lan] [PATCH v2] igb: Add I210 cable fault detection to    self test
> 
> From: Joe Schultz <jschultz@xes-inc.com>
> 
> Add an offline diagnostic test for the I210 internal PHY which checks
> for cable faults and reports the distance along the cable where the
> fault was detected. Fault types detected include open, short, and
> cross-pair short.
> 
> Signed-off-by: Joe Schultz <jschultz@xes-inc.com>
> Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
> ---
>  v2 - account for changes made by this patch in dev-queue:
>    drivers/net: get rid of unnecessary initializations in .get_drvinfo()
> 
>  drivers/net/ethernet/intel/igb/e1000_defines.h |  13 +-
>  drivers/net/ethernet/intel/igb/igb_ethtool.c   | 189 ++++++++++++++++++++++++-
>  2 files changed, 197 insertions(+), 5 deletions(-)
>

This introduces a compile warning:
--------------------------------------------------------------------------------------------------------------------------------
  CC [M]  drivers/net/ethernet/intel/igb/igb_ethtool.o
drivers/net/ethernet/intel/igb/igb_ethtool.c: In function ‘igb_cable_fault_test’:
drivers/net/ethernet/intel/igb/igb_ethtool.c:2074: warning: ‘pcdc’ may be used uninitialized in this function
  CC [M]  drivers/net/ethernet/intel/igb/e1000_82575.o
--------------------------------------------------------------------------------------------------------------------------------

I'm also wondering if the fault distance checks are getting run with diags in the offline mode and a cable is connected.  Whenever I run "ethtool -t ethX offline" while I have a valid link I get a "-1", which I believe is the default value, for the results on each pair.
--------------------------------------------------------------------------------------------------------------------------------
...
Pair D cable fault   (offline)   0
Pair A fault distance            -1This is true with an i210 and i211 (other parts I have tried do not show the fault checks, as expected.
Pair B fault distance            -1
Pair C fault distance            -1
Pair D fault distance            -1
Pair A fault open                0
...
--------------------------------------------------------------------------------------------------------------------------------
This is true with an i210 and i211 (other parts I have tried do not show the fault checks, as expected.

> ...
> ...
> +       /* Wait up to 1.5s for the results to be ready */
> +       while (pcdc & I347AT4_PCDC_CABLE_DIAG_STATUS) {
> +               ret_val = igb_read_phy_reg(hw, I347AT4_PCDC, &pcdc);
> +               if (ret_val || timeout == 1500)
> +                       break;
> +               udelay(1000);
> +               timeout++;
> +       }
> +
> +       if (timeout >= 1500)
> +               dev_warn(&adapter->pdev->dev,
> +                       "Cable fault test timed out. Results may be invalid");

How did you come up with this (1.5) second value?  I'm getting this message fair amount of the time with what I thought are good cables plugged into both ancient and modern switches.
Aaron Sierra Dec. 11, 2015, 7:39 p.m. UTC | #2
----- Original Message -----
> From: "Aaron F Brown" <aaron.f.brown@intel.com>
> Sent: Thursday, December 10, 2015 9:21:27 PM
> 
> > From: Intel-wired-lan [intel-wired-lan-bounces@lists.osuosl.org] on behalf
> > of Aaron Sierra [asierra@xes-inc.com]
> > Sent: Thursday, December 03, 2015 11:43 AM
> > To: Kirsher, Jeffrey T; intel-wired-lan@lists.osuosl.org
> > Cc: Matthew Vick; Joe Schultz
> > Subject: [Intel-wired-lan] [PATCH v2] igb: Add I210 cable fault detection
> > to    self test
> > 
> > From: Joe Schultz <jschultz@xes-inc.com>
> > 
> > Add an offline diagnostic test for the I210 internal PHY which checks
> > for cable faults and reports the distance along the cable where the
> > fault was detected. Fault types detected include open, short, and
> > cross-pair short.
> > 
> > Signed-off-by: Joe Schultz <jschultz@xes-inc.com>
> > Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
> > ---
> >  v2 - account for changes made by this patch in dev-queue:
> >    drivers/net: get rid of unnecessary initializations in .get_drvinfo()
> > 
> >  drivers/net/ethernet/intel/igb/e1000_defines.h |  13 +-
> >  drivers/net/ethernet/intel/igb/igb_ethtool.c   | 189
> >  ++++++++++++++++++++++++-
> >  2 files changed, 197 insertions(+), 5 deletions(-)
> >
> 
> This introduces a compile warning:
> --------------------------------------------------------------------------------------------------------------------------------
>   CC [M]  drivers/net/ethernet/intel/igb/igb_ethtool.o
> drivers/net/ethernet/intel/igb/igb_ethtool.c: In function
> ‘igb_cable_fault_test’:
> drivers/net/ethernet/intel/igb/igb_ethtool.c:2074: warning: ‘pcdc’ may be
> used uninitialized in this function
>   CC [M]  drivers/net/ethernet/intel/igb/e1000_82575.o
> --------------------------------------------------------------------------------------------------------------------------------
> 
> I'm also wondering if the fault distance checks are getting run with diags in
> the offline mode and a cable is connected.  Whenever I run "ethtool -t ethX
> offline" while I have a valid link I get a "-1", which I believe is the
> default value, for the results on each pair.
> --------------------------------------------------------------------------------------------------------------------------------
> ...
> Pair D cable fault   (offline)   0
> Pair A fault distance            -1This is true with an i210 and i211 (other
> parts I have tried do not show the fault checks, as expected.
> Pair B fault distance            -1
> Pair C fault distance            -1
> Pair D fault distance            -1
> Pair A fault open                0
> ...
> --------------------------------------------------------------------------------------------------------------------------------
> This is true with an i210 and i211 (other parts I have tried do not show the
> fault checks, as expected.

Aaron,
The test will not report real values unless the port is "up" when testing is
done. Perhaps, we need to modify the test to ensure that it is in the
necessary state via igb_power_up_link()?

> > ...
> > ...
> > +       /* Wait up to 1.5s for the results to be ready */
> > +       while (pcdc & I347AT4_PCDC_CABLE_DIAG_STATUS) {
> > +               ret_val = igb_read_phy_reg(hw, I347AT4_PCDC, &pcdc);
> > +               if (ret_val || timeout == 1500)
> > +                       break;
> > +               udelay(1000);
> > +               timeout++;
> > +       }
> > +
> > +       if (timeout >= 1500)
> > +               dev_warn(&adapter->pdev->dev,
> > +                       "Cable fault test timed out. Results may be
> > invalid");
> 
> How did you come up with this (1.5) second value?  I'm getting this message
> fair amount of the time with what I thought are good cables plugged into
> both ancient and modern switches.

The timeout value was chosen through testing. We don't know of any documented
value that defines the typical runtime.

-Aaron S.
Brown, Aaron F Dec. 11, 2015, 10:32 p.m. UTC | #3
> From: Aaron Sierra [asierra@xes-inc.com]
> Sent: Friday, December 11, 2015 11:39 AM
> To: Brown, Aaron F
> Cc: Kirsher, Jeffrey T; intel-wired-lan@lists.osuosl.org; Matthew Vick; Joe Schultz
> Subject: Re: [Intel-wired-lan] [PATCH v2] igb: Add I210 cable fault detection to        self test
> 
> > From: "Aaron F Brown" <aaron.f.brown@intel.com>
> > Sent: Thursday, December 10, 2015 9:21:27 PM
> >
> > > From: Intel-wired-lan [intel-wired-lan-bounces@lists.osuosl.org] on behalf
> > > of Aaron Sierra [asierra@xes-inc.com]
> > > Sent: Thursday, December 03, 2015 11:43 AM
> > > To: Kirsher, Jeffrey T; intel-wired-lan@lists.osuosl.org
> > > Cc: Matthew Vick; Joe Schultz
> > > Subject: [Intel-wired-lan] [PATCH v2] igb: Add I210 cable fault detection
> > > to    self test
> > >
> > > From: Joe Schultz <jschultz@xes-inc.com>
> > >
> > > Add an offline diagnostic test for the I210 internal PHY which checks
> > > ...
> > > ...
> > I'm also wondering if the fault distance checks are getting run with diags in
> > the offline mode and a cable is connected.  Whenever I run "ethtool -t ethX
> > offline" while I have a valid link I get a "-1", which I believe is the
> > default value, for the results on each pair.
> > --------------------------------------------------------------------------------------------------------------------------------
> > ...
> > Pair D cable fault   (offline)   0
> > Pair A fault distance            -1
> > Pair B fault distance            -1
> > Pair C fault distance            -1
> > Pair D fault distance            -1
> > Pair A fault open                0
> > ...
> --------------------------------------------------------------------------------------------------------------------------------
> > This is true with an i210 and i211 (other parts I have tried do not show the
> > fault checks, as expected.
> 
> Aaron,
> The test will not report real values unless the port is "up" when testing is
> done. Perhaps, we need to modify the test to ensure that it is in the
> necessary state via igb_power_up_link()?

Ah, that makes sense, even if the port is up when the test starts, the diags offline mode brings it down.  Yes, I could by into some way of bringing the link up so the test will get run properly.  Alternatively I guess you could bypass the test when in offline mode and throw a message stating the port needs to be up (run in online mode) for this set of checks.  I prefer bringing the link up and running it as offline is supposed to be a more inclusive set of tests than online.

> > > ...
> > > ...
> > > +       if (timeout >= 1500)
> > > +               dev_warn(&adapter->pdev->dev,
> > > +                       "Cable fault test timed out. Results may be
> > > invalid");
> >
> > How did you come up with this (1.5) second value?  I'm getting this message
> > fair amount of the time with what I thought are good cables plugged into
> > both ancient and modern switches.
> 
> The timeout value was chosen through testing. We don't know of any documented
> value that defines the typical runtime.
> 
> -Aaron S.

Nor do I.  Would the test being run with link down (offline mode) make the difference?  I only noticed the message when I was running in offline mode (but I also ran it a lot more often with offline as I was chasing those "-1"s.

Thanks, Aaron B.
Aaron Sierra Dec. 16, 2015, 12:10 a.m. UTC | #4
----- Original Message -----
> From: "Aaron F Brown" <aaron.f.brown@intel.com>
> Sent: Friday, December 11, 2015 4:32:58 PM
> 
> > From: Aaron Sierra [asierra@xes-inc.com]
> > Sent: Friday, December 11, 2015 11:39 AM
> > To: Brown, Aaron F
> > Cc: Kirsher, Jeffrey T; intel-wired-lan@lists.osuosl.org; Matthew Vick; Joe
> > Schultz
> > Subject: Re: [Intel-wired-lan] [PATCH v2] igb: Add I210 cable fault
> > detection to        self test
> > 
> > > From: "Aaron F Brown" <aaron.f.brown@intel.com>
> > > Sent: Thursday, December 10, 2015 9:21:27 PM
> > >
> > > > From: Intel-wired-lan [intel-wired-lan-bounces@lists.osuosl.org] on
> > > > behalf
> > > > of Aaron Sierra [asierra@xes-inc.com]
> > > > Sent: Thursday, December 03, 2015 11:43 AM
> > > > To: Kirsher, Jeffrey T; intel-wired-lan@lists.osuosl.org
> > > > Cc: Matthew Vick; Joe Schultz
> > > > Subject: [Intel-wired-lan] [PATCH v2] igb: Add I210 cable fault
> > > > detection
> > > > to    self test
> > > >
> > > > From: Joe Schultz <jschultz@xes-inc.com>
> > > >
> > > > Add an offline diagnostic test for the I210 internal PHY which checks
> > > > ...
> > > > ...
> > > I'm also wondering if the fault distance checks are getting run with
> > > diags in
> > > the offline mode and a cable is connected.  Whenever I run "ethtool -t
> > > ethX
> > > offline" while I have a valid link I get a "-1", which I believe is the
> > > default value, for the results on each pair.
> > > --------------------------------------------------------------------------------------------------------------------------------
> > > ...
> > > Pair D cable fault   (offline)   0
> > > Pair A fault distance            -1
> > > Pair B fault distance            -1
> > > Pair C fault distance            -1
> > > Pair D fault distance            -1
> > > Pair A fault open                0
> > > ...
> > --------------------------------------------------------------------------------------------------------------------------------
> > > This is true with an i210 and i211 (other parts I have tried do not show
> > > the
> > > fault checks, as expected.
> > 
> > Aaron,
> > The test will not report real values unless the port is "up" when testing
> > is
> > done. Perhaps, we need to modify the test to ensure that it is in the
> > necessary state via igb_power_up_link()?
> 
> Ah, that makes sense, even if the port is up when the test starts, the diags
> offline mode brings it down.  Yes, I could by into some way of bringing the
> link up so the test will get run properly.  Alternatively I guess you could
> bypass the test when in offline mode and throw a message stating the port
> needs to be up (run in online mode) for this set of checks.  I prefer
> bringing the link up and running it as offline is supposed to be a more
> inclusive set of tests than online.

Aaron,

Actually, this portion of the offline test is already structured so that the
port is automatically brought up before running the cable diagnostic (thanks
to the link test). See below for more.
 
> > > > ...
> > > > ...
> > > > +       if (timeout >= 1500)
> > > > +               dev_warn(&adapter->pdev->dev,
> > > > +                       "Cable fault test timed out. Results may be
> > > > invalid");
> > >
> > > How did you come up with this (1.5) second value?  I'm getting this
> > > message
> > > fair amount of the time with what I thought are good cables plugged into
> > > both ancient and modern switches.
> > 
> > The timeout value was chosen through testing. We don't know of any
> > documented
> > value that defines the typical runtime.
> > 
> > -Aaron S.
> 
> Nor do I.  Would the test being run with link down (offline mode) make the
> difference?  I only noticed the message when I was running in offline mode
> (but I also ran it a lot more often with offline as I was chasing those
> "-1"s.

I think the issue is that this v2 version of the patch runs the cable
diagnostic even if link is detected on the port. There's no real point in
testing for cable faults if link is present. The PHY seems to agree, since it
times out if link is present. I'll include a fix in the next version.

-Aaron S.
diff mbox

Patch

diff --git a/drivers/net/ethernet/intel/igb/e1000_defines.h b/drivers/net/ethernet/intel/igb/e1000_defines.h
index 240902e..d18a6d0 100644
--- a/drivers/net/ethernet/intel/igb/e1000_defines.h
+++ b/drivers/net/ethernet/intel/igb/e1000_defines.h
@@ -932,6 +932,7 @@ 
 #define I347AT4_PCDL1                  0x11 /* Pair 1 PHY Cable Diagnostics Length */
 #define I347AT4_PCDL2                  0x12 /* Pair 2 PHY Cable Diagnostics Length */
 #define I347AT4_PCDL3                  0x13 /* Pair 3 PHY Cable Diagnostics Length */
+#define I347AT4_PCDR                   0x14 /* PHY Cable Diagnostics Results */
 #define I347AT4_PCDC                   0x15 /* PHY Cable Diagnostics Control */
 #define I347AT4_PAGE_SELECT            0x16
 
@@ -952,7 +953,17 @@ 
 #define I347AT4_PSCR_DOWNSHIFT_8X     0x7000
 
 /* i347-AT4 PHY Cable Diagnostics Control */
-#define I347AT4_PCDC_CABLE_LENGTH_UNIT 0x0400 /* 0=cm 1=meters */
+#define I347AT4_PCDC_CABLE_LENGTH_UNIT	0x0400 /* 0=cm 1=meters */
+#define I347AT4_PCDC_CABLE_DIAG_STATUS	0x0800
+#define I347AT4_PCDC_DISABLE_CROSS_PAIR	0x2000
+#define I347AT4_PCDC_RUN_AT_AUTONEG	0x4000
+#define I347AT4_PCDC_RUN_TEST		0x8000
+
+/* i347-AT4 PHY Cable Diagnostics Results */
+#define I347AT4_PCDR_CABLE_OK		0x0001 /* No faults detected on pair */
+#define I347AT4_PCDR_CABLE_OPEN		0x0002 /* Open pair detected */
+#define I347AT4_PCDR_CABLE_SHORT	0x0003 /* Shorted pair detected */
+#define I347AT4_PCDR_CABLE_CROSS_SHORT	0x0004 /* Cross-pair short detected */
 
 /* Marvell 1112 only registers */
 #define M88E1112_VCT_DSP_DISTANCE       0x001A
diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c
index 1d329f1..066cc73 100644
--- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
+++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
@@ -132,7 +132,28 @@  enum igb_diagnostics_results {
 	TEST_EEP,
 	TEST_IRQ,
 	TEST_LOOP,
-	TEST_LINK
+	TEST_LINK,
+	/* I210 superset */
+	TEST_FAULT_A,
+	TEST_FAULT_B,
+	TEST_FAULT_C,
+	TEST_FAULT_D,
+	TEST_LENGTH_A,
+	TEST_LENGTH_B,
+	TEST_LENGTH_C,
+	TEST_LENGTH_D,
+	TEST_OPEN_A,
+	TEST_OPEN_B,
+	TEST_OPEN_C,
+	TEST_OPEN_D,
+	TEST_SHORT_A,
+	TEST_SHORT_B,
+	TEST_SHORT_C,
+	TEST_SHORT_D,
+	TEST_CROSS_A,
+	TEST_CROSS_B,
+	TEST_CROSS_C,
+	TEST_CROSS_D
 };
 
 static const char igb_gstrings_test[][ETH_GSTRING_LEN] = {
@@ -142,7 +163,50 @@  static const char igb_gstrings_test[][ETH_GSTRING_LEN] = {
 	[TEST_LOOP] = "Loopback test  (offline)",
 	[TEST_LINK] = "Link test   (on/offline)"
 };
+
+static const char igb_i210_gstrings_test[][ETH_GSTRING_LEN] = {
+	[TEST_REG]	= "Register test        (offline)",
+	[TEST_EEP]	= "Eeprom test          (offline)",
+	[TEST_IRQ]	= "Interrupt test       (offline)",
+	[TEST_LOOP]	= "Loopback test        (offline)",
+	[TEST_LINK]	= "Link test         (on/offline)",
+	[TEST_FAULT_A]  = "Pair A cable fault   (offline)",
+	[TEST_FAULT_B]  = "Pair B cable fault   (offline)",
+	[TEST_FAULT_C]  = "Pair C cable fault   (offline)",
+	[TEST_FAULT_D]  = "Pair D cable fault   (offline)",
+	[TEST_LENGTH_A] = "Pair A fault distance         ",
+	[TEST_LENGTH_B] = "Pair B fault distance         ",
+	[TEST_LENGTH_C] = "Pair C fault distance         ",
+	[TEST_LENGTH_D] = "Pair D fault distance         ",
+	[TEST_OPEN_A]   = "Pair A fault open             ",
+	[TEST_OPEN_B]   = "Pair B fault open             ",
+	[TEST_OPEN_C]   = "Pair C fault open             ",
+	[TEST_OPEN_D]   = "Pair D fault open             ",
+	[TEST_SHORT_A]  = "Pair A fault intra-pair short ",
+	[TEST_SHORT_B]  = "Pair B fault intra-pair short ",
+	[TEST_SHORT_C]  = "Pair C fault intra-pair short ",
+	[TEST_SHORT_D]  = "Pair D fault intra-pair short ",
+	[TEST_CROSS_A]  = "Pair A fault inter-pair short ",
+	[TEST_CROSS_B]  = "Pair B fault inter-pair short ",
+	[TEST_CROSS_C]  = "Pair C fault inter-pair short ",
+	[TEST_CROSS_D]  = "Pair D fault inter-pair short "
+};
+
 #define IGB_TEST_LEN (sizeof(igb_gstrings_test) / ETH_GSTRING_LEN)
+#define IGB_I210_TEST_LEN (sizeof(igb_i210_gstrings_test) / ETH_GSTRING_LEN)
+
+static inline bool igb_has_i210_cable_fault_test(struct igb_adapter *adapter)
+{
+	struct e1000_hw *hw = &adapter->hw;
+
+	if (hw->phy.media_type != e1000_media_type_copper)
+		return false;
+
+	if (hw->phy.id == I210_I_PHY_ID)
+		return true;
+
+	return false;
+}
 
 static int igb_get_settings(struct net_device *netdev, struct ethtool_cmd *ecmd)
 {
@@ -1983,6 +2047,104 @@  static int igb_link_test(struct igb_adapter *adapter, u64 *data)
 	return *data;
 }
 
+static int igb_cable_fault_test_prep(struct igb_adapter *adapter)
+{
+	struct e1000_hw *hw = &adapter->hw;
+	u16 pcdc;
+	s32 ret_val;
+
+	ret_val = igb_write_phy_reg(hw, I347AT4_PAGE_SELECT, 0x7);
+	if (ret_val)
+		goto done;
+
+	/* Initiate diagnostics at next auto-negotiation */
+	pcdc = I347AT4_PCDC_CABLE_LENGTH_UNIT |
+		I347AT4_PCDC_RUN_AT_AUTONEG;
+
+	ret_val = igb_write_phy_reg(hw, I347AT4_PCDC, pcdc);
+
+done:
+	igb_write_phy_reg(hw, I347AT4_PAGE_SELECT, 0);
+	return ret_val;
+}
+
+static int igb_cable_fault_test(struct igb_adapter *adapter,
+			   struct ethtool_test *eth_test, u64 *data) {
+	struct e1000_hw *hw = &adapter->hw;
+	u16 pcdc, pcdr;
+	u16 error_code = 0;
+	u32 timeout = 0;
+	s32 ret_val;
+	int i;
+
+	ret_val = igb_write_phy_reg(hw, I347AT4_PAGE_SELECT, 0x7);
+	if (ret_val)
+		goto done;
+
+	ret_val = igb_write_phy_reg(hw, I347AT4_PCDC, pcdc);
+	if (ret_val)
+		goto done;
+
+	/* Wait up to 1.5s for the results to be ready */
+	while (pcdc & I347AT4_PCDC_CABLE_DIAG_STATUS) {
+		ret_val = igb_read_phy_reg(hw, I347AT4_PCDC, &pcdc);
+		if (ret_val || timeout == 1500)
+			break;
+		udelay(1000);
+		timeout++;
+	}
+
+	if (timeout >= 1500)
+		dev_warn(&adapter->pdev->dev,
+			"Cable fault test timed out. Results may be invalid");
+
+	ret_val = igb_read_phy_reg(hw, I347AT4_PCDR, &pcdr);
+	if (ret_val)
+		goto done;
+
+	hw->phy.ops.get_cable_length(hw);
+
+	/* Iterate over each cable pair */
+	for (i = 0; i < 4; i++) {
+		data[TEST_LENGTH_A + i] = hw->phy.pair_length[i];
+
+		error_code = (pcdr >> (i * 4)) & 0xf;
+		switch (error_code) {
+		case I347AT4_PCDR_CABLE_OK:
+			data[TEST_FAULT_A + i] = 0;
+			data[TEST_LENGTH_A + i] = -1;
+			/* don't assign ret_val */
+			break;
+		case I347AT4_PCDR_CABLE_OPEN:
+			data[TEST_FAULT_A + i] = 1;
+			data[TEST_OPEN_A + i] = 1;
+			ret_val = -1;
+			break;
+		case I347AT4_PCDR_CABLE_SHORT:
+			data[TEST_FAULT_A + i] = 1;
+			data[TEST_SHORT_A + i] = 1;
+			ret_val = -1;
+			break;
+		case I347AT4_PCDR_CABLE_CROSS_SHORT:
+			data[TEST_FAULT_A + i] = 1;
+			data[TEST_CROSS_A + i] = 1;
+			ret_val = -1;
+			break;
+		default:
+			data[TEST_FAULT_A + i] = -1;
+			data[TEST_LENGTH_A + i] = -1;
+			data[TEST_OPEN_A + i] = -1;
+			data[TEST_SHORT_A + i] = -1;
+			data[TEST_CROSS_A + i] = -1;
+			ret_val = -1;
+		}
+	}
+
+done:
+	igb_write_phy_reg(hw, I347AT4_PAGE_SELECT, 0);
+	return ret_val;
+}
+
 static void igb_diag_test(struct net_device *netdev,
 			  struct ethtool_test *eth_test, u64 *data)
 {
@@ -1993,6 +2155,11 @@  static void igb_diag_test(struct net_device *netdev,
 
 	set_bit(__IGB_TESTING, &adapter->state);
 
+	if (igb_has_i210_cable_fault_test(adapter)) {
+		memset(&data[TEST_FAULT_A], 0x0,
+			sizeof(u64) * (IGB_I210_TEST_LEN - IGB_TEST_LEN));
+	}
+
 	/* can't do offline tests on media switching devices */
 	if (adapter->hw.dev_spec._82575.mas_capable)
 		eth_test->flags &= ~ETH_TEST_FL_OFFLINE;
@@ -2009,12 +2176,21 @@  static void igb_diag_test(struct net_device *netdev,
 		/* power up link for link test */
 		igb_power_up_link(adapter);
 
+		if (igb_has_i210_cable_fault_test(adapter))
+			igb_cable_fault_test_prep(adapter);
+
 		/* Link test performed before hardware reset so autoneg doesn't
 		 * interfere with test result
 		 */
 		if (igb_link_test(adapter, &data[TEST_LINK]))
 			eth_test->flags |= ETH_TEST_FL_FAILED;
 
+		/* Test for cable faults before the PHY gets shut off */
+		if (igb_has_i210_cable_fault_test(adapter)) {
+			if (igb_cable_fault_test(adapter, eth_test, data))
+				eth_test->flags |= ETH_TEST_FL_FAILED;
+		}
+
 		if (if_running)
 			/* indicate we're in test mode */
 			dev_close(netdev);
@@ -2270,7 +2446,8 @@  static int igb_get_sset_count(struct net_device *netdev, int sset)
 	case ETH_SS_STATS:
 		return IGB_STATS_LEN;
 	case ETH_SS_TEST:
-		return IGB_TEST_LEN;
+		return igb_has_i210_cable_fault_test(netdev_priv(netdev)) ?
+			IGB_I210_TEST_LEN : IGB_TEST_LEN;
 	default:
 		return -ENOTSUPP;
 	}
@@ -2340,8 +2517,12 @@  static void igb_get_strings(struct net_device *netdev, u32 stringset, u8 *data)
 
 	switch (stringset) {
 	case ETH_SS_TEST:
-		memcpy(data, *igb_gstrings_test,
-			IGB_TEST_LEN*ETH_GSTRING_LEN);
+		if (igb_has_i210_cable_fault_test(adapter))
+			memcpy(data, *igb_i210_gstrings_test,
+				IGB_I210_TEST_LEN*ETH_GSTRING_LEN);
+		else
+			memcpy(data, *igb_gstrings_test,
+				IGB_TEST_LEN*ETH_GSTRING_LEN);
 		break;
 	case ETH_SS_STATS:
 		for (i = 0; i < IGB_GLOBAL_STATS_LEN; i++) {