[PATCHv1] e1000e: Allow ethtool to enable/disable loopback.

[PATCHv1] e1000e: Allow ethtool to enable/disable loopback.

[Mahesh Bandewar]

This patch adds e1000_set_features() to handle loopback mode. When loopback
is enabled, it enables internal-MAC loopback.

Signed-off-by: Mahesh Bandewar <maheshb@google.com>
---
 drivers/net/e1000e/e1000.h   |    1 +
 drivers/net/e1000e/ethtool.c |   34 ++++++++++++++++++++++++++++++++++
 drivers/net/e1000e/netdev.c  |   21 +++++++++++++++++++++
 drivers/net/e1000e/phy.c     |   19 +++++++++++++++----
 4 files changed, 71 insertions(+), 4 deletions(-)

diff --git a/drivers/net/e1000e/e1000.h b/drivers/net/e1000e/e1000.h
index 9549879..9db3037 100644
--- a/drivers/net/e1000e/e1000.h
+++ b/drivers/net/e1000e/e1000.h
@@ -640,6 +640,7 @@ extern s32 e1000_check_polarity_ife(struct e1000_hw *hw);
 extern s32 e1000_phy_force_speed_duplex_ife(struct e1000_hw *hw);
 extern s32 e1000_check_polarity_igp(struct e1000_hw *hw);
 extern bool e1000_check_phy_82574(struct e1000_hw *hw);
+extern int e1000_set_loopback(struct net_device *dev, u32 features);
 
 static inline s32 e1000_phy_hw_reset(struct e1000_hw *hw)
 {
diff --git a/drivers/net/e1000e/ethtool.c b/drivers/net/e1000e/ethtool.c
index 859d0d3..9c26b42 100644
--- a/drivers/net/e1000e/ethtool.c
+++ b/drivers/net/e1000e/ethtool.c
@@ -2027,6 +2027,40 @@ static int e1000e_set_flags(struct net_device *netdev, u32 data)
 	return 0;
 }
 
+int e1000_set_loopback(struct net_device *netdev, u32 features)
+{
+	struct e1000_adapter *adapter = netdev_priv(netdev);
+	struct e1000_hw *hw = &adapter->hw;
+	uint16_t reg = 0;
+	int retval = 0;
+
+	if ((retval = e1e_rphy(hw, PHY_CONTROL, &reg)) > 0)
+		return retval;
+
+	if (features & NETIF_F_LOOPBACK) {
+		if (reg & MII_CR_LOOPBACK)
+			return 0;
+
+		retval = e1000_setup_loopback_test(adapter);
+		if (retval)
+			return retval;
+
+		netdev_info(netdev,
+			"e1000e: Internal PHY loopback mode enabled.\n");
+	} else {
+		if (!(reg & MII_CR_LOOPBACK))
+			return 0;
+
+		e1000_loopback_cleanup(adapter);
+		e1000e_down(adapter);
+		e1000e_up(adapter);
+		netdev_info(netdev,
+			"e1000e: Internal PHY loopback mode disabled.\n");
+	}
+
+	return retval;
+}
+
 static const struct ethtool_ops e1000_ethtool_ops = {
 	.get_settings		= e1000_get_settings,
 	.set_settings		= e1000_set_settings,
diff --git a/drivers/net/e1000e/netdev.c b/drivers/net/e1000e/netdev.c
index 0939040..6aee9b19 100644
--- a/drivers/net/e1000e/netdev.c
+++ b/drivers/net/e1000e/netdev.c
@@ -3688,6 +3688,13 @@ static int e1000_open(struct net_device *netdev)
 	else
 		ew32(ICS, E1000_ICS_LSC);
 
+	/*
+	 * If loopback was enabled while the device was down,
+	 * make sure it gets installed properly now.
+	 */
+	if (netdev->features & NETIF_F_LOOPBACK)
+		e1000_set_loopback(netdev, netdev->features);
+
 	return 0;
 
 err_req_irq:
@@ -5623,6 +5630,16 @@ static void e1000_netpoll(struct net_device *netdev)
 }
 #endif
 
+static int e1000_set_features(struct net_device *dev, u32 features)
+{
+	u32 changed = dev->features ^ features;
+
+	if ((changed & NETIF_F_LOOPBACK) && netif_running(dev))
+		e1000_set_loopback(dev, features);
+
+	return 0;
+}
+
 /**
  * e1000_io_error_detected - called when PCI error is detected
  * @pdev: Pointer to PCI device
@@ -5789,6 +5806,7 @@ static const struct net_device_ops e1000e_netdev_ops = {
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	.ndo_poll_controller	= e1000_netpoll,
 #endif
+	.ndo_set_features	= e1000_set_features,
 };
 
 /**
@@ -6093,6 +6111,9 @@ static int __devinit e1000_probe(struct pci_dev *pdev,
 
 	e1000_print_device_info(adapter);
 
+	/* Add loopback capability to the device. */
+	netdev->hw_features |= NETIF_F_LOOPBACK;
+
 	if (pci_dev_run_wake(pdev))
 		pm_runtime_put_noidle(&pdev->dev);
 
diff --git a/drivers/net/e1000e/phy.c b/drivers/net/e1000e/phy.c
index 484774c..9bc5df3 100644
--- a/drivers/net/e1000e/phy.c
+++ b/drivers/net/e1000e/phy.c
@@ -1758,7 +1758,7 @@ s32 e1000e_phy_has_link_generic(struct e1000_hw *hw, u32 iterations,
 			       u32 usec_interval, bool *success)
 {
 	s32 ret_val = 0;
-	u16 i, phy_status;
+	u16 i, reg;
 
 	for (i = 0; i < iterations; i++) {
 		/*
@@ -1766,7 +1766,7 @@ s32 e1000e_phy_has_link_generic(struct e1000_hw *hw, u32 iterations,
 		 * twice due to the link bit being sticky.  No harm doing
 		 * it across the board.
 		 */
-		ret_val = e1e_rphy(hw, PHY_STATUS, &phy_status);
+		ret_val = e1e_rphy(hw, PHY_STATUS, &reg);
 		if (ret_val)
 			/*
 			 * If the first read fails, another entity may have
@@ -1774,10 +1774,10 @@ s32 e1000e_phy_has_link_generic(struct e1000_hw *hw, u32 iterations,
 			 * see if they have relinquished the resources yet.
 			 */
 			udelay(usec_interval);
-		ret_val = e1e_rphy(hw, PHY_STATUS, &phy_status);
+		ret_val = e1e_rphy(hw, PHY_STATUS, &reg);
 		if (ret_val)
 			break;
-		if (phy_status & MII_SR_LINK_STATUS)
+		if (reg & MII_SR_LINK_STATUS)
 			break;
 		if (usec_interval >= 1000)
 			mdelay(usec_interval/1000);
@@ -1787,6 +1787,17 @@ s32 e1000e_phy_has_link_generic(struct e1000_hw *hw, u32 iterations,
 
 	*success = (i < iterations);
 
+	/*
+	 * If the device is in loopback mode, we have to fake
+	 * link up. Try this only if link isn't present.
+	 */
+	if (!(*success) && !e1e_rphy(hw, PHY_CONTROL, &reg) &&
+		(reg & MII_CR_LOOPBACK)) {
+		/* Fake link up. */
+		*success = true;
+		ret_val = 0;
+	}
+
 	return ret_val;
 }
 
-- 
1.7.3.1

Re: [PATCHv1] e1000e: Allow ethtool to enable/disable loopback.

[Michał Mirosław]

2011/5/11 Mahesh Bandewar <maheshb@google.com>:
> This patch adds e1000_set_features() to handle loopback mode. When loopback
> is enabled, it enables internal-MAC loopback.

Please wait for this driver's conversion to hw_features. One comment
below, though.

[...]
> --- a/drivers/net/e1000e/netdev.c
> +++ b/drivers/net/e1000e/netdev.c
[...]
> +static int e1000_set_features(struct net_device *dev, u32 features)
> +{
> +       u32 changed = dev->features ^ features;
> +
> +       if ((changed & NETIF_F_LOOPBACK) && netif_running(dev))
> +               e1000_set_loopback(dev, features);
> +
> +       return 0;
> +}
[...]

If e1000_set_loopback() fails, this should set dev->features to passed
features (but keeping NETIF_F_LOOPBACK unchanged in dev->features) to
keep the state consistent.

Best Regards,
Michał Mirosław

Re: [PATCHv1] e1000e: Allow ethtool to enable/disable loopback.

[Mahesh Bandewar]

On Wed, May 11, 2011 at 12:15 PM, Michał Mirosław <mirqus@gmail.com> wrote:
> 2011/5/11 Mahesh Bandewar <maheshb@google.com>:
>> This patch adds e1000_set_features() to handle loopback mode. When loopback
>> is enabled, it enables internal-MAC loopback.
>
> Please wait for this driver's conversion to hw_features. One comment
> below, though.
>
This is not intrusive so should not create problems when that happens.

> [...]
>> --- a/drivers/net/e1000e/netdev.c
>> +++ b/drivers/net/e1000e/netdev.c
> [...]
>> +static int e1000_set_features(struct net_device *dev, u32 features)
>> +{
>> +       u32 changed = dev->features ^ features;
>> +
>> +       if ((changed & NETIF_F_LOOPBACK) && netif_running(dev))
>> +               e1000_set_loopback(dev, features);
>> +
>> +       return 0;
>> +}
> [...]
>
> If e1000_set_loopback() fails, this should set dev->features to passed
> features (but keeping NETIF_F_LOOPBACK unchanged in dev->features) to
> keep the state consistent.
>
set_features() can return the return code of set_loopback() instead of
0; this way the consistency will be maintained.

Thanks,
--mahesh..

> Best Regards,
> Michał Mirosław
>

------------------------------------------------------------------------------
Achieve unprecedented app performance and reliability
What every C/C++ and Fortran developer should know.
Learn how Intel has extended the reach of its next-generation tools
to help boost performance applications - inlcuding clusters.
http://p.sf.net/sfu/intel-dev2devmay
_______________________________________________
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: [PATCHv1] e1000e: Allow ethtool to enable/disable loopback.

[David Miller]

From: Mahesh Bandewar <maheshb@google.com>
Date: Wed, 11 May 2011 16:11:21 -0700

> On Wed, May 11, 2011 at 12:15 PM, Michał Mirosław <mirqus@gmail.com> wrote:
>> 2011/5/11 Mahesh Bandewar <maheshb@google.com>:
>>> This patch adds e1000_set_features() to handle loopback mode. When loopback
>>> is enabled, it enables internal-MAC loopback.
>>
>> Please wait for this driver's conversion to hw_features. One comment
>> below, though.
>>
> This is not intrusive so should not create problems when that happens.

Right and the Intel maintainer folks can resolve such conflicts
anyways.

Re: [PATCHv1] e1000e: Allow ethtool to enable/disable loopback.

[Michał Mirosław]

W dniu 12 maja 2011 01:11 użytkownik Mahesh Bandewar
<maheshb@google.com> napisał:
> On Wed, May 11, 2011 at 12:15 PM, Michał Mirosław <mirqus@gmail.com> wrote:
>> 2011/5/11 Mahesh Bandewar <maheshb@google.com>:
>>> This patch adds e1000_set_features() to handle loopback mode. When loopback
>>> is enabled, it enables internal-MAC loopback.
>> Please wait for this driver's conversion to hw_features. One comment
>> below, though.
> This is not intrusive so should not create problems when that happens.

Fine by me then.

>> [...]
>>> --- a/drivers/net/e1000e/netdev.c
>>> +++ b/drivers/net/e1000e/netdev.c
>> [...]
>>> +static int e1000_set_features(struct net_device *dev, u32 features)
>>> +{
>>> +       u32 changed = dev->features ^ features;
>>> +
>>> +       if ((changed & NETIF_F_LOOPBACK) && netif_running(dev))
>>> +               e1000_set_loopback(dev, features);
>>> +
>>> +       return 0;
>>> +}
>> [...]
>>
>> If e1000_set_loopback() fails, this should set dev->features to passed
>> features (but keeping NETIF_F_LOOPBACK unchanged in dev->features) to
>> keep the state consistent.
> set_features() can return the return code of set_loopback() instead of
> 0; this way the consistency will be maintained.

Only as long as NETIF_F_LOOPBACK is the only bit set in hw_features.
netdev_update_features() can't really know which features were changed
and which failed when ndo_set_features callback returns non-zero.

Best Regards,
Michał Mirosław

Re: [PATCHv1] e1000e: Allow ethtool to enable/disable loopback.

[Mahesh Bandewar]

On Wed, May 11, 2011 at 10:50 PM, Michał Mirosław <mirqus@gmail.com> wrote:
> W dniu 12 maja 2011 01:11 użytkownik Mahesh Bandewar
> <maheshb@google.com> napisał:
>> On Wed, May 11, 2011 at 12:15 PM, Michał Mirosław <mirqus@gmail.com> wrote:
>>> 2011/5/11 Mahesh Bandewar <maheshb@google.com>:
>>>> This patch adds e1000_set_features() to handle loopback mode. When loopback
>>>> is enabled, it enables internal-MAC loopback.
>>> Please wait for this driver's conversion to hw_features. One comment
>>> below, though.
>> This is not intrusive so should not create problems when that happens.
>
> Fine by me then.
>
>>> [...]
>>>> --- a/drivers/net/e1000e/netdev.c
>>>> +++ b/drivers/net/e1000e/netdev.c
>>> [...]
>>>> +static int e1000_set_features(struct net_device *dev, u32 features)
>>>> +{
>>>> +       u32 changed = dev->features ^ features;
>>>> +
>>>> +       if ((changed & NETIF_F_LOOPBACK) && netif_running(dev))
>>>> +               e1000_set_loopback(dev, features);
>>>> +
>>>> +       return 0;
>>>> +}
>>> [...]
>>>
>>> If e1000_set_loopback() fails, this should set dev->features to passed
>>> features (but keeping NETIF_F_LOOPBACK unchanged in dev->features) to
>>> keep the state consistent.
>> set_features() can return the return code of set_loopback() instead of
>> 0; this way the consistency will be maintained.
>
> Only as long as NETIF_F_LOOPBACK is the only bit set in hw_features.
> netdev_update_features() can't really know which features were changed
> and which failed when ndo_set_features callback returns non-zero.
>
This is more of an API shortcoming. Callback will have to revert
changes made (rollback) before returning non-zero value to keep it
consistent.

Thanks,
--mahesh..

> Best Regards,
> Michał Mirosław
>

------------------------------------------------------------------------------
Achieve unprecedented app performance and reliability
What every C/C++ and Fortran developer should know.
Learn how Intel has extended the reach of its next-generation tools
to help boost performance applications - inlcuding clusters.
http://p.sf.net/sfu/intel-dev2devmay
_______________________________________________
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: [PATCHv1] e1000e: Allow ethtool to enable/disable loopback.

[Michał Mirosław]

W dniu 12 maja 2011 23:41 użytkownik Mahesh Bandewar
<maheshb@google.com> napisał:
> On Wed, May 11, 2011 at 10:50 PM, Michał Mirosław <mirqus@gmail.com> wrote:
>> W dniu 12 maja 2011 01:11 użytkownik Mahesh Bandewar
>> <maheshb@google.com> napisał:
>>> On Wed, May 11, 2011 at 12:15 PM, Michał Mirosław <mirqus@gmail.com> wrote:
>>>> If e1000_set_loopback() fails, this should set dev->features to passed
>>>> features (but keeping NETIF_F_LOOPBACK unchanged in dev->features) to
>>>> keep the state consistent.
>>> set_features() can return the return code of set_loopback() instead of
>>> 0; this way the consistency will be maintained.
>> Only as long as NETIF_F_LOOPBACK is the only bit set in hw_features.
>> netdev_update_features() can't really know which features were changed
>> and which failed when ndo_set_features callback returns non-zero.
> This is more of an API shortcoming. Callback will have to revert
> changes made (rollback) before returning non-zero value to keep it
> consistent.

It might just update dev->features to match instead of rollback. It
could also start some recovery process that eventually calls
netdev_update_features() again to try the change again.

IOW, the information what changes failed are returned implicitly in
modified dev->features. When callback returns 0,
netdev_update_features() assumes that all were set correctly and
updates dev->features itself.

Best Regards,
Michał Mirosław