[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

[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

[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

[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

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

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!

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.

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.

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

[David Miller]

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