igb: Add I210 cable fault detection to self test
Subsystems (2)
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH v2 1/1 igb: Add I210 cable fault detection to self test
From: Joe Schultz <[email protected]>
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.
---
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(-)
drivers/net/ethernet/intel/igb/e1000_defines.h +12 -1
@@ -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
drivers/net/ethernet/intel/igb/igb_ethtool.c +185 -4
238 lines not on this page · raw message
Signature · 1 line
1.9.1
Brown
PATCH v2 1/1 igb: Add I210 cable fault detection to self test
Quoted 15 lines
From: Intel-wired-lan [intel-wired-lan-bounces at lists.osuosl.org] on behalf of Aaron Sierra [asierra at xes-inc.com]
Sent: Thursday, December 03, 2015 11:43 AM
To: Kirsher, Jeffrey T; intel-wired-lan at lists.osuosl.org
Cc: Matthew Vick; Joe Schultz
Subject: [Intel-wired-lan] [PATCH v2] igb: Add I210 cable fault detection to self testFrom: Joe Schultz <[email protected]>
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 <[email protected]>Signed-off-by Aaron Sierra <[email protected]>
Quoted 8 lines
---
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.odrivers/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.
Quoted 4 lines
...
...drivers/net/ethernet/intel/igb/igb_ethtool.c +2
-0+/* Wait up to 1.5s for the results to be ready */ +while (pcdc &I347AT4_PCDC_CABLE_DIAG_STATUS ) {
drivers/net/ethernet/intel/igb/igb_ethtool.c static int igb_link_test(struct igb_adapter *adapter, u64 *data) · line 2099
Quoted 10 lines
drivers/net/ethernet/intel/igb/igb_ethtool.c +10
-0+ 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.
PATCH v2 1/1 igb: Add I210 cable fault detection to self test
----- Original Message -----
Quoted 49 lines
From: "Aaron F Brown" <[email protected]>
Sent: Thursday, December 10, 2015 9:21:27 PMQuoted 17 lines
From: Intel-wired-lan [intel-wired-lan-bounces at lists.osuosl.org] on behalf of Aaron Sierra [asierra at xes-inc.com]
Sent: Thursday, December 03, 2015 11:43 AM
To: Kirsher, Jeffrey T; intel-wired-lan at lists.osuosl.org
Cc: Matthew Vick; Joe Schultz
Subject: [Intel-wired-lan] [PATCH v2] igb: Add I210 cable fault detection
to self testFrom: Joe Schultz <[email protected]>
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 <[email protected]>Signed-off-by Aaron Sierra <[email protected]>Quoted 9 lines
---
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.odrivers/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.
Quoted 8 lines
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()?
Quoted 16 lines
Quoted 14 lines
...
...drivers/net/ethernet/intel/igb/igb_ethtool.c +12
-0+/* 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 beQuoted 1 line
invalid");
Quoted 3 lines
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
PATCH v2 1/1 igb: Add I210 cable fault detection to self test
Quoted 35 lines
From: Aaron Sierra [asierra at xes-inc.com]
Sent: Friday, December 11, 2015 11:39 AM
To: Brown, Aaron F
Cc: Kirsher, Jeffrey T; intel-wired-lan at lists.osuosl.org; Matthew Vick; Joe Schultz
Subject: Re: [Intel-wired-lan] [PATCH v2] igb: Add I210 cable fault detection to self testQuoted 22 lines
From: "Aaron F Brown" <[email protected]>
Sent: Thursday, December 10, 2015 9:21:27 PMQuoted 8 lines
From: Intel-wired-lan [intel-wired-lan-bounces at lists.osuosl.org] on behalf of Aaron Sierra [asierra at xes-inc.com]
Sent: Thursday, December 03, 2015 11:43 AM
To: Kirsher, Jeffrey T; intel-wired-lan at lists.osuosl.org
Cc: Matthew Vick; Joe Schultz
Subject: [Intel-wired-lan] [PATCH v2] igb: Add I210 cable fault detection
to self testQuoted 5 lines
From: Joe Schultz <[email protected]>
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.
--------------------------------------------------------------------------------------------------------------------------------
...Quoted 7 lines
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...
Quoted 8 lines
--------------------------------------------------------------------------------------------------------------------------------
Quoted 2 lines
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 viaigb_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.
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
...
...drivers/net/ethernet/intel/igb/igb_ethtool.c +3
-0+if (timeout >=1500 ) + dev_warn(&adapter->pdev->dev, + "Cable fault test timed out. Results may beinvalid");
Quoted 8 lines
Quoted 3 lines
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.
PATCH v2 1/1 igb: Add I210 cable fault detection to self test
----- Original Message -----
Quoted 55 lines
From: "Aaron F Brown" <[email protected]>
Sent: Friday, December 11, 2015 4:32:58 PMQuoted 46 lines
From: Aaron Sierra [asierra at xes-inc.com]
Sent: Friday, December 11, 2015 11:39 AM
To: Brown, Aaron F
Cc: Kirsher, Jeffrey T; intel-wired-lan at lists.osuosl.org; Matthew Vick; Joe Schultz
Subject: Re: [Intel-wired-lan] [PATCH v2] igb: Add I210 cable fault
detection to self testQuoted 26 lines
From: "Aaron F Brown" <[email protected]>
Sent: Thursday, December 10, 2015 9:21:27 PMQuoted 10 lines
From: Intel-wired-lan [intel-wired-lan-bounces at lists.osuosl.org] on behalf
of Aaron Sierra [asierra at xes-inc.com]
Sent: Thursday, December 03, 2015 11:43 AM
To: Kirsher, Jeffrey T; intel-wired-lan at lists.osuosl.org
Cc: Matthew Vick; Joe Schultz
Subject: [Intel-wired-lan] [PATCH v2] igb: Add I210 cable fault detection
to self testQuoted 5 lines
From: Joe Schultz <[email protected]>
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.
--------------------------------------------------------------------------------------------------------------------------------
...Quoted 7 lines
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...
--------------------------------------------------------------------------------------------------------------------------------
Quoted 3 lines
This is true with an i210 and i211 (other parts I have tried do not show the
fault checks, as expected.Quoted 5 lines
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 viaigb_power_up_link()?
Quoted 7 lines
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.
Quoted 16 lines
Quoted 16 lines
Quoted 11 lines
Quoted 6 lines
...
...drivers/net/ethernet/intel/igb/igb_ethtool.c +3
-0+if (timeout >=1500 ) + dev_warn(&adapter->pdev->dev, + "Cable fault test timed out. Results may beinvalid");
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.
Quoted 6 lines
Quoted 1 line
-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.