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

[PATCH net-next v2 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.

Changes since v1:
  - simplification in patch 1/4 (from Michał Mirosław)

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

David Decotigny (2):
  net-e1000e: fix ethtool set_features taking new features into account
    too late
  net-e1000e: 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  |   32 +++++++++++-
 drivers/net/ethernet/intel/e1000e/phy.c     |   69 +++++++++++++++++----------
 4 files changed, 78 insertions(+), 31 deletions(-)

-- 
1.7.3.1

[PATCH net-next v2 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 |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index a5bd7a3..a83a108 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -5901,7 +5901,7 @@ 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;
@@ -5913,12 +5913,19 @@ static int e1000_set_features(struct net_device *netdev,
 			 NETIF_F_RXCSUM)))
 		return 0;
 
+	/* We will make sure to return 1 below so that we can change
+	 * netdev->features on the fly from within the functions
+	 * called below in case of error, without core/dev.c
+	 * overriding it. */
+	netdev->features = features;
+
 	if (netif_running(netdev))
 		e1000e_reinit_locked(adapter);
 	else
 		e1000e_reset(adapter);
 
-	return 0;
+	return 1; /* tell net/core/dev.c not to override
+		   * netdev->features */
 }
 
 static const struct net_device_ops e1000e_netdev_ops = {
-- 
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 v2 2/4] net-e1000e: 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
  - in each case, ifconfig rx&tx errors = 0 but rx_dropped can be != 0
  - 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 a83a108..ca45a12 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");
+	}
 }
 
 /**
@@ -5910,7 +5918,7 @@ 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 0;
 
 	/* We will make sure to return 1 below so that we can change
@@ -5919,6 +5927,14 @@ static int e1000_set_features(struct net_device *netdev,
 	 * overriding it. */
 	netdev->features = features;
 
+	/* 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
@@ -6116,6 +6132,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 v2 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 v2 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 v2 0/4] e1000e: ethtool setfeatures fixes + loopback

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 1235 bytes --]

On Tue, 2011-11-22 at 11:23 -0800, David Decotigny wrote:
> 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.
> 
> Changes since v1:
>   - simplification in patch 1/4 (from Michał Mirosław)
> 
> ############################################
> # Patch Set Summary:
> 
> David Decotigny (2):
>   net-e1000e: fix ethtool set_features taking new features into account
>     too late
>   net-e1000e: 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  |   32 +++++++++++-
>  drivers/net/ethernet/intel/e1000e/phy.c     |   69 +++++++++++++++++----------
>  4 files changed, 78 insertions(+), 31 deletions(-)
> 

Thanks David for the patch set.  I have added the e1000e patches to my
queue.  As an fyi, I want Bruce Allan to look over these patches and he
is out this week on vacation.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[David Decotigny]

Jeff, Bruce,

Please drop this patch series as I was informed that:
 - http://patchwork.ozlabs.org/patch/95175/ already implemented
loopback for ethtool and is under review. Please note, though, that
the version (patch 2) I submitted builds on top of newer net-next
changes
 - Maciej Żenczykowski would prefer not to use the version I sent for
patches 3 and 4, and will provide an improved patch

I will mark the series as rejected on patchwork and will resend patch
1 (fix) as "v3".

My apologies for this hick-up.

Regards,

--
David Decotigny



On Tue, Nov 22, 2011 at 12:46 PM, Jeff Kirsher
<jeffrey.t.kirsher@intel.com> wrote:
> On Tue, 2011-11-22 at 11:23 -0800, David Decotigny wrote:
>> 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.
>>
>> Changes since v1:
>>   - simplification in patch 1/4 (from Michał Mirosław)
>>
>> ############################################
>> # Patch Set Summary:
>>
>> David Decotigny (2):
>>   net-e1000e: fix ethtool set_features taking new features into account
>>     too late
>>   net-e1000e: 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  |   32 +++++++++++-
>>  drivers/net/ethernet/intel/e1000e/phy.c     |   69 +++++++++++++++++----------
>>  4 files changed, 78 insertions(+), 31 deletions(-)
>>
>
> Thanks David for the patch set.  I have added the e1000e patches to my
> queue.  As an fyi, I want Bruce Allan to look over these patches and he
> is out this week on vacation.

------------------------------------------------------------------------------
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 v2 0/4] e1000e: ethtool setfeatures fixes + loopback

[Maciej Żenczykowski]

David, could you test the trivial-forward-port patch I sent out and
verify that it continues to work correctly (was there anything you had
to fix in my patch?), a quick glance at the driver doesn't seem to
show any changes between 2.6.34 and now that would cause it to break
in unexpected ways... but who knows...?

There's a few features of it that I prefer to your version of the patch:
- it should be more robust to e1e_rphy timeout failures
- it checks loopback later, which makes that more robust, and can
check it multiple times.
- fixed up comments, variable renames, etc. better readability (IMHO)

Obviously it should probably be split into a separate patch to fix
bugs, and a separate one to implement loopback carrier faking.

The basic problem is that normally e1e_rphy() succeeds very quickly,
but in actuality the code is kind of more like:
  e1e_rphy() {
    for (i = 0; i < MAX_ITER; ++i) {
      try to read register
      on success return value
      pause
    }
    return failure;
  }

There is potential for the read to fail due to the firmware having
locked you out, hence the delay loop.  Unfortunately in certain cases
this fails for longer then MAX_ITER * pause_time, causing e1e_rphy()
as a whole to fail.

However, once it succeeds, subsequent reads are also very likely to
succeed and be very fast as well (ie. failures are _very_
time-correlated).

Hence, you cannot check for anything only once and expect it to
reliably behave.  This is the reason why in my patch loopback
detection was deeper in the loop instead of the first thing one does
(this also causes loopback detection code to not trigger on the normal
case of link up, so it's faster in the normal case).

There's also a lot of places where intervals == 0 on call, which is
why my patch has the extra read before the loop - it makes the
intervals == 0 case just slightly more likely to succeed.

Remember e1e_rphy() has 3 cases:
  timeout
  success after time < timeout
  immediate success.

And in practice, failed reads are more likely to be back-to-back than random.

A relatively likely scenario is 3 reads behaving like so:
  timeout, success after time < timeout, immediate success.