[PATCH net-next v1 0/4] e1000e: ethtool setfeatures fixes + loopback

[PATCH net-next v1 0/4] e1000e: ethtool setfeatures fixes + loopback

[David Decotigny]

This series fixes a bug in ethtool setfeatures and adds loopback
support through ethtool setfeatures.

I believe these patches could easily be adapted to e1000, but I don't
have the hardware to test.

############################################
# Patch Set Summary:

David Decotigny (2):
  net-e1000e: fix ethtool set_features taking new features into account
    too late
  net-e1000e: enable ethtool loopback support

Maciej Żenczykowski (2):
  net-e1000e: reworked carrier detection logic
  net-e1000e: Report carrier in loopback mode

 drivers/net/ethernet/intel/e1000e/e1000.h   |    2 +
 drivers/net/ethernet/intel/e1000e/ethtool.c |    6 +-
 drivers/net/ethernet/intel/e1000e/netdev.c  |   31 ++++++++++--
 drivers/net/ethernet/intel/e1000e/phy.c     |   69 +++++++++++++++++----------
 4 files changed, 76 insertions(+), 32 deletions(-)

-- 
1.7.3.1


------------------------------------------------------------------------------
All the data continuously generated in your IT infrastructure 
contains a definitive record of customers, application performance, 
security threats, fraudulent activity, and more. Splunk takes this 
data and makes sense of it. IT sense. And common sense.
http://p.sf.net/sfu/splunk-novd2d
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired

[PATCH net-next v1 1/4] net-e1000e: fix ethtool set_features taking new features into account too late

[David Decotigny]

Before this patch, the set_features() ethtool callback would not take
the new specified features into account when configuring the NIC. It
would only consider the current features, ie. the ones after previous
set_features(). A second ethtool set_features() with the same
specified features cannot work around that issue because it would boil
down to a nop (code checks wether specified features changed or not).

This patch solves this by propagating the requested features.

Tested:
  printk in code + custom ethtool (based on
  http://patchwork.ozlabs.org/patch/95836/)



Signed-off-by: David Decotigny <david.decotigny@google.com>
---
 drivers/net/ethernet/intel/e1000e/netdev.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index a5bd7a3..b63f316 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -5901,24 +5901,28 @@ static void e1000_eeprom_checks(struct e1000_adapter *adapter)
 }
 
 static int e1000_set_features(struct net_device *netdev,
-	netdev_features_t features)
+			      netdev_features_t features)
 {
 	struct e1000_adapter *adapter = netdev_priv(netdev);
 	netdev_features_t changed = features ^ netdev->features;
+	int retval = 1;  /* telling netdev that we are updating
+			  * netdev->features by ourselves */
+
+	netdev->features = features;
 
 	if (changed & (NETIF_F_TSO | NETIF_F_TSO6))
 		adapter->flags |= FLAG_TSO_FORCE;
 
 	if (!(changed & (NETIF_F_HW_VLAN_RX | NETIF_F_HW_VLAN_TX |
 			 NETIF_F_RXCSUM)))
-		return 0;
+		return retval;
 
 	if (netif_running(netdev))
 		e1000e_reinit_locked(adapter);
 	else
 		e1000e_reset(adapter);
 
-	return 0;
+	return retval;
 }
 
 static const struct net_device_ops e1000e_netdev_ops = {
-- 
1.7.3.1

[PATCH net-next v1 2/4] net-e1000e: enable ethtool loopback support

[David Decotigny]

This allows ethtool to activate loopback via the get/set_features API.

Tested:
  - HW = 80003ES2LAN (copper), x86_64
  - incoming external 200Mbps, loopback on/off, verified that incoming
    stream resumed
  - incoming external 200Mbps, ifconfig down, loopback on/off,
    ifconfig up, verified that incoming stream resumed
  - incoming external 200Mbps, loopback on, ifconfig down, loopback
    off, ifconfig up, verified that incoming stream resumed
  - the same with a pktgen when loopback is enabled: verifying that
    tx_packets = rx_packets + rx_missed_errors, and that tx_packets is
    very close to pktgen_packets (1 or 2 packets difference, not
    more). Note that rx_missed_errors >> 0
  - artificially simulated failure of enabling loopback -> reported by
    set_features and kept loopback feature marked as not configured.



Signed-off-by: David Decotigny <david.decotigny@google.com>
---
 drivers/net/ethernet/intel/e1000e/e1000.h   |    2 ++
 drivers/net/ethernet/intel/e1000e/ethtool.c |    6 +++---
 drivers/net/ethernet/intel/e1000e/netdev.c  |   21 ++++++++++++++++++++-
 3 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/e1000.h b/drivers/net/ethernet/intel/e1000e/e1000.h
index 9fe18d1..786cf31 100644
--- a/drivers/net/ethernet/intel/e1000e/e1000.h
+++ b/drivers/net/ethernet/intel/e1000e/e1000.h
@@ -485,6 +485,8 @@ extern const char e1000e_driver_version[];
 
 extern void e1000e_check_options(struct e1000_adapter *adapter);
 extern void e1000e_set_ethtool_ops(struct net_device *netdev);
+extern int e1000_loopback_setup(struct e1000_adapter *adapter);
+extern void e1000_loopback_cleanup(struct e1000_adapter *adapter);
 
 extern int e1000e_up(struct e1000_adapter *adapter);
 extern void e1000e_down(struct e1000_adapter *adapter);
diff --git a/drivers/net/ethernet/intel/e1000e/ethtool.c b/drivers/net/ethernet/intel/e1000e/ethtool.c
index fb2c28e..259add2 100644
--- a/drivers/net/ethernet/intel/e1000e/ethtool.c
+++ b/drivers/net/ethernet/intel/e1000e/ethtool.c
@@ -1410,7 +1410,7 @@ static int e1000_set_es2lan_mac_loopback(struct e1000_adapter *adapter)
 	return 0;
 }
 
-static int e1000_setup_loopback_test(struct e1000_adapter *adapter)
+int e1000_loopback_setup(struct e1000_adapter *adapter)
 {
 	struct e1000_hw *hw = &adapter->hw;
 	u32 rctl;
@@ -1438,7 +1438,7 @@ static int e1000_setup_loopback_test(struct e1000_adapter *adapter)
 	return 7;
 }
 
-static void e1000_loopback_cleanup(struct e1000_adapter *adapter)
+void e1000_loopback_cleanup(struct e1000_adapter *adapter)
 {
 	struct e1000_hw *hw = &adapter->hw;
 	u32 rctl;
@@ -1593,7 +1593,7 @@ static int e1000_loopback_test(struct e1000_adapter *adapter, u64 *data)
 	if (*data)
 		goto out;
 
-	*data = e1000_setup_loopback_test(adapter);
+	*data = e1000_loopback_setup(adapter);
 	if (*data)
 		goto err_loopback;
 
diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index b63f316..25102e6 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -3256,6 +3256,14 @@ static void e1000_configure(struct e1000_adapter *adapter)
 	e1000_configure_rx(adapter);
 	adapter->alloc_rx_buf(adapter, e1000_desc_unused(adapter->rx_ring),
 			      GFP_KERNEL);
+
+	if (adapter->netdev->features & NETIF_F_LOOPBACK) {
+		if (e1000_loopback_setup(adapter)) {
+			e_warn("Could not activate loopback mode\n");
+			adapter->netdev->features &= ~NETIF_F_LOOPBACK;
+		} else
+			e_info("Loopback mode activated\n");
+	}
 }
 
 /**
@@ -5914,9 +5922,17 @@ static int e1000_set_features(struct net_device *netdev,
 		adapter->flags |= FLAG_TSO_FORCE;
 
 	if (!(changed & (NETIF_F_HW_VLAN_RX | NETIF_F_HW_VLAN_TX |
-			 NETIF_F_RXCSUM)))
+			 NETIF_F_RXCSUM | NETIF_F_LOOPBACK)))
 		return retval;
 
+	/* disable loopback if requested */
+	if ((changed & NETIF_F_LOOPBACK) && (!(features & NETIF_F_LOOPBACK))) {
+		netdev_dbg(netdev, "Disabling loopback mode\n");
+		e1000_loopback_cleanup(adapter);
+	}
+	/* otherwise loopback is enabled via e1000e_reinit_locked
+	 * below and each time the device is up'ed */
+
 	if (netif_running(netdev))
 		e1000e_reinit_locked(adapter);
 	else
@@ -6113,6 +6129,9 @@ static int __devinit e1000_probe(struct pci_dev *pdev,
 	/* Set user-changeable features (subset of all device features) */
 	netdev->hw_features = netdev->features;
 
+	/* Add capabilities, disabled by default */
+	netdev->hw_features |= NETIF_F_LOOPBACK;
+
 	if (adapter->flags & FLAG_HAS_HW_VLAN_FILTER)
 		netdev->features |= NETIF_F_HW_VLAN_FILTER;
 
-- 
1.7.3.1


------------------------------------------------------------------------------
All the data continuously generated in your IT infrastructure 
contains a definitive record of customers, application performance, 
security threats, fraudulent activity, and more. Splunk takes this 
data and makes sense of it. IT sense. And common sense.
http://p.sf.net/sfu/splunk-novd2d
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

[PATCH net-next v1 3/4] net-e1000e: reworked carrier detection logic

[David Decotigny]

From: Maciej Żenczykowski <zenczykowski@gmail.com>

This change removes an un-needed final pause in case of timeout and
reworks the "link is up" detection logic.



Signed-off-by: David Decotigny <david.decotigny@google.com>
---
 drivers/net/ethernet/intel/e1000e/phy.c |   64 +++++++++++++++++++------------
 1 files changed, 39 insertions(+), 25 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/phy.c b/drivers/net/ethernet/intel/e1000e/phy.c
index 8666476..f487a7f 100644
--- a/drivers/net/ethernet/intel/e1000e/phy.c
+++ b/drivers/net/ethernet/intel/e1000e/phy.c
@@ -1775,39 +1775,53 @@ static s32 e1000_wait_autoneg(struct e1000_hw *hw)
  *  Polls the PHY status register for link, 'iterations' number of times.
  **/
 s32 e1000e_phy_has_link_generic(struct e1000_hw *hw, u32 iterations,
-			       u32 usec_interval, bool *success)
+				u32 usec_interval, bool *success)
 {
-	s32 ret_val = 0;
-	u16 i, phy_status;
+	u32 i;
+	u16 phy_reg;
+	int good_reads_phy_status = 0;
+
+	}
+
+	/*
+	 * Remember that e1e_rphy may fail because of another entity
+	 * (like the firmware) holding the lock, we need to handle
+	 * this gracefully - by waiting and trying again.
+	 *
+	 * Some PHYs require the PHY_STATUS register to be read twice
+	 * due to the link bit being sticky.  No harm doing it across
+	 * the board.
+	 */
+	for (i = 0; i < iterations; /* i incremented manually */) {
+		if (0 == e1e_rphy(hw, PHY_STATUS, &phy_reg)) {
+			if (++good_reads_phy_status < 2)
+				continue; /* Re-read once, to make sure */
 
-	for (i = 0; i < iterations; i++) {
-		/*
-		 * Some PHYs require the PHY_STATUS register to be read
-		 * twice due to the link bit being sticky.  No harm doing
-		 * it across the board.
-		 */
-		ret_val = e1e_rphy(hw, PHY_STATUS, &phy_status);
-		if (ret_val)
 			/*
-			 * If the first read fails, another entity may have
-			 * ownership of the resources, wait and try again to
-			 * see if they have relinquished the resources yet.
+			 * Great, we got at least 2 successfull reads
 			 */
-			udelay(usec_interval);
-		ret_val = e1e_rphy(hw, PHY_STATUS, &phy_status);
-		if (ret_val)
-			break;
-		if (phy_status & MII_SR_LINK_STATUS)
-			break;
-		if (usec_interval >= 1000)
-			mdelay(usec_interval/1000);
+
+			if (phy_reg & MII_SR_LINK_STATUS) {
+				/* success: link up */
+				*success = true;
+				return 0;
+			}
+		}
+
+		/* Pause now and re-iterate */
+		if (++i >= iterations)
+			break; /* Pause not needed after last iteration */
+		else if (usec_interval >= 1000)
+			mdelay(usec_interval / 1000);
 		else
 			udelay(usec_interval);
 	}
 
-	*success = (i < iterations);
-
-	return ret_val;
+	/* no success in for(;;) loop */
+	*success = false;
+	if (good_reads_phy_status < 2)
+		return -E1000_ERR_PHY;
+	return 0;
 }
 
 /**
-- 
1.7.3.1


------------------------------------------------------------------------------
All the data continuously generated in your IT infrastructure 
contains a definitive record of customers, application performance, 
security threats, fraudulent activity, and more. Splunk takes this 
data and makes sense of it. IT sense. And common sense.
http://p.sf.net/sfu/splunk-novd2d
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

[PATCH net-next v1 4/4] net-e1000e: Report carrier in loopback mode

[David Decotigny]

From: Maciej Żenczykowski <zenczykowski@gmail.com>

When interface is configured in loopback mode, force carrier check
positive.  This is useful when interface does not have carrier and
test puts the interface in loopback mode.

Tested: pktgen loopback on 80003ES2LAN


Signed-off-by: David Decotigny <david.decotigny@google.com>
---
 drivers/net/ethernet/intel/e1000e/phy.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/phy.c b/drivers/net/ethernet/intel/e1000e/phy.c
index f487a7f..9476c8f 100644
--- a/drivers/net/ethernet/intel/e1000e/phy.c
+++ b/drivers/net/ethernet/intel/e1000e/phy.c
@@ -1781,6 +1781,11 @@ s32 e1000e_phy_has_link_generic(struct e1000_hw *hw, u32 iterations,
 	u16 phy_reg;
 	int good_reads_phy_status = 0;
 
+	/* If loopback is enabled, we claim the link is up */
+	if ((0 == e1e_rphy(hw, PHY_CONTROL, &phy_reg))
+	    && (phy_reg & MII_CR_LOOPBACK)) {
+		*success = true;
+		return 0;
 	}
 
 	/*
-- 
1.7.3.1


------------------------------------------------------------------------------
All the data continuously generated in your IT infrastructure 
contains a definitive record of customers, application performance, 
security threats, fraudulent activity, and more. Splunk takes this 
data and makes sense of it. IT sense. And common sense.
http://p.sf.net/sfu/splunk-novd2d
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

Re: [PATCH net-next v1 1/4] net-e1000e: fix ethtool set_features taking new features into account too late

[Michał Mirosław]

2011/11/22 David Decotigny <david.decotigny@google.com>:
[...]
> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
> index a5bd7a3..b63f316 100644
> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> @@ -5901,24 +5901,28 @@ static void e1000_eeprom_checks(struct e1000_adapter *adapter)
>  }
>
>  static int e1000_set_features(struct net_device *netdev,
> -       netdev_features_t features)
> +                             netdev_features_t features)
>  {
>        struct e1000_adapter *adapter = netdev_priv(netdev);
>        netdev_features_t changed = features ^ netdev->features;
> +       int retval = 1;  /* telling netdev that we are updating
> +                         * netdev->features by ourselves */
> +
> +       netdev->features = features;
>
>        if (changed & (NETIF_F_TSO | NETIF_F_TSO6))
>                adapter->flags |= FLAG_TSO_FORCE;
>
>        if (!(changed & (NETIF_F_HW_VLAN_RX | NETIF_F_HW_VLAN_TX |
>                         NETIF_F_RXCSUM)))
> -               return 0;
> +               return retval;
>

Would be less code if you set netdev->features here...

>        if (netif_running(netdev))
>                e1000e_reinit_locked(adapter);
>        else
>                e1000e_reset(adapter);
>
> -       return 0;
> +       return retval;

... and return 1 here, noting in a comment that e1000e_reinit_locked()
might have changed netdev->features.

>  }

Best Regards,
Michał Mirosław

------------------------------------------------------------------------------
All the data continuously generated in your IT infrastructure 
contains a definitive record of customers, application performance, 
security threats, fraudulent activity, and more. Splunk takes this 
data and makes sense of it. IT sense. And common sense.
http://p.sf.net/sfu/splunk-novd2d
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

Re: [PATCH net-next v1 1/4] net-e1000e: fix ethtool set_features taking new features into account too late

[David Decotigny]

Hello,

2011/11/21 Michał Mirosław <mirqus@gmail.com>:
>>  static int e1000_set_features(struct net_device *netdev,
>> -       netdev_features_t features)
>> +                             netdev_features_t features)
>>  {
>>        struct e1000_adapter *adapter = netdev_priv(netdev);
>>        netdev_features_t changed = features ^ netdev->features;
>> +       int retval = 1;  /* telling netdev that we are updating
>> +                         * netdev->features by ourselves */
>> +
>> +       netdev->features = features;
>>
>>        if (changed & (NETIF_F_TSO | NETIF_F_TSO6))
>>                adapter->flags |= FLAG_TSO_FORCE;
>>
>>        if (!(changed & (NETIF_F_HW_VLAN_RX | NETIF_F_HW_VLAN_TX |
>>                         NETIF_F_RXCSUM)))
>> -               return 0;
>> +               return retval;
>>
>
> Would be less code if you set netdev->features here...
>
>>        if (netif_running(netdev))
>>                e1000e_reinit_locked(adapter);
>>        else
>>                e1000e_reset(adapter);
>>
>> -       return 0;
>> +       return retval;
>
> ... and return 1 here, noting in a comment that e1000e_reinit_locked()
> might have changed netdev->features.

This would work, although I preferred the systematic approach for code
management reasons. But I will follow your recommendations. Waiting a
little (review of other patches) before sending the updated version.

Thanks!

------------------------------------------------------------------------------
All the data continuously generated in your IT infrastructure 
contains a definitive record of customers, application performance, 
security threats, fraudulent activity, and more. Splunk takes this 
data and makes sense of it. IT sense. And common sense.
http://p.sf.net/sfu/splunk-novd2d
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

Re: [PATCH net-next v1 3/4] net-e1000e: reworked carrier detection logic

[Maciej Żenczykowski]

David,

I know you asked for a review of this on Friday, before you sent this
out to netdev, and I was planning on doing so, but I see you've sent
it out to a wider audience now.
So, I'll attach comments directly here.

You took my patch (which was for 2.6.34) and split it in two (it was
erroneously marked author zenczykowski@gmail.com instead of
maze@google.com, so that should be fixed) and while you seem to have
reduced the amount of lines-of-code-churn, I'm not sure you've
actually made the resulting code (as a whole) more readable.

I'm not sure how much of these changes were a result of changes
elsewhere in the driver between 2.6.34 and now.

This was very tricky code to get right, and I'm not sure whether the
patch as you posted it gets all the corner cases right...
Indeed I actually think the changes you've made reduce the robustness
wrt. timeouts caused by firmware concurrent accesses.  In particular
the first read access you make is very likely to fail.
I think renaming success to link and iterations to intervals was
worthwhile since it improved readability...

For reference, the original patch, cherrypicked onto net-next and
compile (but not anything else) tested will be a reply to this email.

------------------------------------------------------------------------------
All the data continuously generated in your IT infrastructure 
contains a definitive record of customers, application performance, 
security threats, fraudulent activity, and more. Splunk takes this 
data and makes sense of it. IT sense. And common sense.
http://p.sf.net/sfu/splunk-novd2d
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired

[PATCH] net-e1000e: Report carrier in loopback mode.

[Maciej Żenczykowski]

From: Maciej Żenczykowski <maze@google.com>

When loopback mode is forced on interface, and if the carrier check returns
negative, then force carrier check positive.  This is useful when interface
does not have carrier and test puts the interface in loopback mode.

While we're at it rework the code to fix other bugs in it.
---
 drivers/net/ethernet/intel/e1000e/e1000.h   |    4 +-
 drivers/net/ethernet/intel/e1000e/ich8lan.c |    4 +-
 drivers/net/ethernet/intel/e1000e/lib.c     |    2 +-
 drivers/net/ethernet/intel/e1000e/phy.c     |   98 +++++++++++++++++----------
 4 files changed, 68 insertions(+), 40 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/e1000.h b/drivers/net/ethernet/intel/e1000e/e1000.h
index 9fe18d1..93ffc39 100644
--- a/drivers/net/ethernet/intel/e1000e/e1000.h
+++ b/drivers/net/ethernet/intel/e1000e/e1000.h
@@ -620,8 +620,8 @@ extern s32 e1000e_write_kmrn_reg_locked(struct e1000_hw *hw, u32 offset,
 extern s32 e1000e_read_kmrn_reg(struct e1000_hw *hw, u32 offset, u16 *data);
 extern s32 e1000e_read_kmrn_reg_locked(struct e1000_hw *hw, u32 offset,
                                        u16 *data);
-extern s32 e1000e_phy_has_link_generic(struct e1000_hw *hw, u32 iterations,
-			       u32 usec_interval, bool *success);
+extern s32 e1000e_phy_has_link_generic(struct e1000_hw *hw, int intervals,
+			       u32 usec_interval, bool *link);
 extern s32 e1000e_phy_reset_dsp(struct e1000_hw *hw);
 extern void e1000_power_up_phy_copper(struct e1000_hw *hw);
 extern void e1000_power_down_phy_copper(struct e1000_hw *hw);
diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c
index e2a80a2..955cd8c 100644
--- a/drivers/net/ethernet/intel/e1000e/ich8lan.c
+++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c
@@ -681,7 +681,7 @@ static s32 e1000_check_for_copper_link_ich8lan(struct e1000_hw *hw)
 	 * link.  If so, then we want to get the current speed/duplex
 	 * of the PHY.
 	 */
-	ret_val = e1000e_phy_has_link_generic(hw, 1, 0, &link);
+	ret_val = e1000e_phy_has_link_generic(hw, 0, 0, &link);
 	if (ret_val)
 		goto out;
 
@@ -3527,7 +3527,7 @@ static s32 e1000_kmrn_lock_loss_workaround_ich8lan(struct e1000_hw *hw)
 	 * Attempting this while link is negotiating fouled up link
 	 * stability
 	 */
-	ret_val = e1000e_phy_has_link_generic(hw, 1, 0, &link);
+	ret_val = e1000e_phy_has_link_generic(hw, 0, 0, &link);
 	if (!link)
 		return 0;
 
diff --git a/drivers/net/ethernet/intel/e1000e/lib.c b/drivers/net/ethernet/intel/e1000e/lib.c
index 0893ab1..e9b9e52 100644
--- a/drivers/net/ethernet/intel/e1000e/lib.c
+++ b/drivers/net/ethernet/intel/e1000e/lib.c
@@ -456,7 +456,7 @@ s32 e1000e_check_for_copper_link(struct e1000_hw *hw)
 	 * link.  If so, then we want to get the current speed/duplex
 	 * of the PHY.
 	 */
-	ret_val = e1000e_phy_has_link_generic(hw, 1, 0, &link);
+	ret_val = e1000e_phy_has_link_generic(hw, 0, 0, &link);
 	if (ret_val)
 		return ret_val;
 
diff --git a/drivers/net/ethernet/intel/e1000e/phy.c b/drivers/net/ethernet/intel/e1000e/phy.c
index 8666476..f026cba 100644
--- a/drivers/net/ethernet/intel/e1000e/phy.c
+++ b/drivers/net/ethernet/intel/e1000e/phy.c
@@ -1768,46 +1768,74 @@ static s32 e1000_wait_autoneg(struct e1000_hw *hw)
 /**
  *  e1000e_phy_has_link_generic - Polls PHY for link
  *  @hw: pointer to the HW structure
- *  @iterations: number of times to poll for link
+ *  @intervals: number of times to delay between polls for link
  *  @usec_interval: delay between polling attempts
- *  @success: pointer to whether polling was successful or not
+ *  @link: pointer to whether link was present or not
  *
- *  Polls the PHY status register for link, 'iterations' number of times.
+ *  Polls the PHY status register for link, 'intervals + 1' number of times.
+ *  Max run time is approx 'intervals * usec_interval' microseconds.
  **/
-s32 e1000e_phy_has_link_generic(struct e1000_hw *hw, u32 iterations,
-			       u32 usec_interval, bool *success)
+s32 e1000e_phy_has_link_generic(struct e1000_hw *hw, int intervals,
+				u32 usec_interval, bool *link)
 {
-	s32 ret_val = 0;
-	u16 i, phy_status;
+	int good_reads_phy_status = 0;
+	bool loopback_checked = false;
+	s32 err;
+	u16 reg;
+
+	/* Remember that e1e_rphy may fail because of another entity
+	 * (like the firmware) holding the lock, we need to handle
+	 * this gracefully - by waiting and trying again.
+	 *
+	 * Some PHYs require the PHY_STATUS register to be
+	 * read twice due to the link bit being sticky.
+	 * No harm doing it across the board.
+	 *
+	 * This first initial read slightly improves the probability of
+	 * a successful double read of the PHY_STATUS on the first iteration.
+	 * (and thus also whenever this function is called with iterations == 0)
+	 */
+	err = e1e_rphy(hw, PHY_STATUS, &reg);
+	if (!err) ++good_reads_phy_status;
+
+	for (;;) {
+		err = e1e_rphy(hw, PHY_STATUS, &reg);
+		if (!err) {
+			if (++good_reads_phy_status < 2) continue;
+			if (reg & MII_SR_LINK_STATUS) {
+				*link = true;
+				return 0; /* success: link up */
+			}
+		}
+
+		if (!loopback_checked) {
+			/* If the interface is in loopback-mode... */
+			err = e1e_rphy(hw, PHY_CONTROL, &reg);
+			if (!err) {
+				loopback_checked = true;
+				if (reg & MII_CR_LOOPBACK) {
+					/* ... fake link up. */
+					*link = true;
+					return 0; /* success: link up */
+				}
+			}
+		}
+
+		if (--intervals < 0) {
+			/* timeout waiting for link to go up (or only errors) */
+			if (good_reads_phy_status < 2) {
+				if (!err) err = -E1000_ERR_PHY;
+				return err; /* failure */
+			}
+			*link = false;
+			return 0; /* success: link down */
+		}
 
-	for (i = 0; i < iterations; i++) {
-		/*
-		 * Some PHYs require the PHY_STATUS register to be read
-		 * twice due to the link bit being sticky.  No harm doing
-		 * it across the board.
-		 */
-		ret_val = e1e_rphy(hw, PHY_STATUS, &phy_status);
-		if (ret_val)
-			/*
-			 * If the first read fails, another entity may have
-			 * ownership of the resources, wait and try again to
-			 * see if they have relinquished the resources yet.
-			 */
-			udelay(usec_interval);
-		ret_val = e1e_rphy(hw, PHY_STATUS, &phy_status);
-		if (ret_val)
-			break;
-		if (phy_status & MII_SR_LINK_STATUS)
-			break;
 		if (usec_interval >= 1000)
-			mdelay(usec_interval/1000);
+			mdelay(usec_interval / 1000);
 		else
 			udelay(usec_interval);
 	}
-
-	*success = (i < iterations);
-
-	return ret_val;
 }
 
 /**
@@ -1943,7 +1971,7 @@ s32 e1000e_get_phy_info_m88(struct e1000_hw *hw)
 		return -E1000_ERR_CONFIG;
 	}
 
-	ret_val = e1000e_phy_has_link_generic(hw, 1, 0, &link);
+	ret_val = e1000e_phy_has_link_generic(hw, 0, 0, &link);
 	if (ret_val)
 		return ret_val;
 
@@ -2011,7 +2039,7 @@ s32 e1000e_get_phy_info_igp(struct e1000_hw *hw)
 	u16 data;
 	bool link;
 
-	ret_val = e1000e_phy_has_link_generic(hw, 1, 0, &link);
+	ret_val = e1000e_phy_has_link_generic(hw, 0, 0, &link);
 	if (ret_val)
 		return ret_val;
 
@@ -2071,7 +2099,7 @@ s32 e1000_get_phy_info_ife(struct e1000_hw *hw)
 	u16 data;
 	bool link;
 
-	ret_val = e1000e_phy_has_link_generic(hw, 1, 0, &link);
+	ret_val = e1000e_phy_has_link_generic(hw, 0, 0, &link);
 	if (ret_val)
 		goto out;
 
@@ -3298,7 +3326,7 @@ s32 e1000_get_phy_info_82577(struct e1000_hw *hw)
 	u16 data;
 	bool link;
 
-	ret_val = e1000e_phy_has_link_generic(hw, 1, 0, &link);
+	ret_val = e1000e_phy_has_link_generic(hw, 0, 0, &link);
 	if (ret_val)
 		goto out;
 
-- 
1.7.3.1

Re: [PATCH net-next v1 0/4] e1000e: ethtool setfeatures fixes + loopback

[David Miller]

I would like to see some discussion wrt. Jamal's feedback, which is that
a lot of the side-band functionality added by this code is either 1) already
doable with packet scheduler actions or 2) should be implemented there.

------------------------------------------------------------------------------
All the data continuously generated in your IT infrastructure 
contains a definitive record of customers, application performance, 
security threats, fraudulent activity, and more. Splunk takes this 
data and makes sense of it. IT sense. And common sense.
http://p.sf.net/sfu/splunk-novd2d
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired

Re: [PATCH net-next v1 0/4] e1000e: ethtool setfeatures fixes + loopback

[David Miller]

Sorry I replied to the wrong posting :-/  My bad.

------------------------------------------------------------------------------
All the data continuously generated in your IT infrastructure 
contains a definitive record of customers, application performance, 
security threats, fraudulent activity, and more. Splunk takes this 
data and makes sense of it. IT sense. And common sense.
http://p.sf.net/sfu/splunk-novd2d
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired