Patchwork e1000: power off PHY after reset when interface is down

login
register
mail settings
Submitter prasanna.panchamukhi@riverbed.com
Date Feb. 23, 2011, 1:25 a.m.
Message ID <1298424313-9840-1-git-send-email-prasanna.panchamukhi@riverbed.com>
Download mbox | patch
Permalink /patch/84040/
State Awaiting Upstream
Delegated to: David Miller
Headers show

Comments

prasanna.panchamukhi@riverbed.com - Feb. 23, 2011, 1:25 a.m.
From: Prasanna S. Panchamukhi <prasanna.panchamukhi@riverbed.com>

Some Phys supported by the e1000 driver do not remain powered off across
a reset of the device when the interface is down, e.g. on 82546.
This patch powers down (only when WoL is disabled) the PHY after reset if
the interface is down and ethtool diagnostics are not currently running.

Similar problem was see on 82571 controller and was fixed in e1000e driver
by Bruce Allan.
Please refer commit 31dbe5b4ac6fca72dec946e4d0fa7f0913f1d9b1 for details.

Signed-off-by: Prasanna S. Panchamukhi <prasanna.panchamukhi@riverbed.com>
---
 drivers/net/e1000/e1000_ethtool.c |   27 +++++++++++++++++++--------
 drivers/net/e1000/e1000_main.c    |    7 +++++++
 2 files changed, 26 insertions(+), 8 deletions(-)
Jeff Kirsher - Feb. 24, 2011, 12:02 a.m.
On Tue, 2011-02-22 at 17:25 -0800, prasanna.panchamukhi@riverbed.com
wrote:
> From: Prasanna S. Panchamukhi <prasanna.panchamukhi@riverbed.com>
> 
> Some Phys supported by the e1000 driver do not remain powered off
> across
> a reset of the device when the interface is down, e.g. on 82546.
> This patch powers down (only when WoL is disabled) the PHY after reset
> if
> the interface is down and ethtool diagnostics are not currently
> running.
> 
> Similar problem was see on 82571 controller and was fixed in e1000e
> driver
> by Bruce Allan.
> Please refer commit 31dbe5b4ac6fca72dec946e4d0fa7f0913f1d9b1 for
> details.
> 
> Signed-off-by: Prasanna S. Panchamukhi
> <prasanna.panchamukhi@riverbed.com>
> ---
>  drivers/net/e1000/e1000_ethtool.c |   27 +++++++++++++++++++--------
>  drivers/net/e1000/e1000_main.c    |    7 +++++++
>  2 files changed, 26 insertions(+), 8 deletions(-) 

Thanks Prasanna!  I have added the patch to my queue of e1000 patches.
Jeff Kirsher - March 4, 2011, 8:19 a.m.
On Wed, Feb 23, 2011 at 16:02, Jeff Kirsher <jeffrey.t.kirsher@intel.com> wrote:
> On Tue, 2011-02-22 at 17:25 -0800, prasanna.panchamukhi@riverbed.com
> wrote:
>> From: Prasanna S. Panchamukhi <prasanna.panchamukhi@riverbed.com>
>>
>> Some Phys supported by the e1000 driver do not remain powered off
>> across
>> a reset of the device when the interface is down, e.g. on 82546.
>> This patch powers down (only when WoL is disabled) the PHY after reset
>> if
>> the interface is down and ethtool diagnostics are not currently
>> running.
>>
>> Similar problem was see on 82571 controller and was fixed in e1000e
>> driver
>> by Bruce Allan.
>> Please refer commit 31dbe5b4ac6fca72dec946e4d0fa7f0913f1d9b1 for
>> details.
>>
>> Signed-off-by: Prasanna S. Panchamukhi
>> <prasanna.panchamukhi@riverbed.com>
>> ---
>>  drivers/net/e1000/e1000_ethtool.c |   27 +++++++++++++++++++--------
>>  drivers/net/e1000/e1000_main.c    |    7 +++++++
>>  2 files changed, 26 insertions(+), 8 deletions(-)
>
> Thanks Prasanna!  I have added the patch to my queue of e1000 patches.
>

Prasanna-
Here is what we found during validating your patch:

The behavior of 82546 device(s) seems to be identical with/without this patch
applied. 82546GB (LOM), dev_id 1079 powers down (with wol disabled) after
ifdown, but powers back up after approx. 10 seconds. 82546EB (NIC), dev_id
1010 powers down (with wol disabled) after ifdown. Both of the above
behaviors are the same with and without the patch applied. Also, if this patch
DID work as expected, it should print a message after a reset, such as "Cannot
restart autonegotiation: Resource temporarily unavailable", which would mirror
the behavior of e1000e.
Prasanna Panchamukhi - March 4, 2011, 6:03 p.m.
On 03/04/2011 12:19 AM, Jeff Kirsher wrote:
> On Wed, Feb 23, 2011 at 16:02, Jeff Kirsher<jeffrey.t.kirsher@intel.com>  wrote:
>> On Tue, 2011-02-22 at 17:25 -0800, prasanna.panchamukhi@riverbed.com
>> wrote:
>>> From: Prasanna S. Panchamukhi<prasanna.panchamukhi@riverbed.com>
>>>
>>> Some Phys supported by the e1000 driver do not remain powered off
>>> across
>>> a reset of the device when the interface is down, e.g. on 82546.
>>> This patch powers down (only when WoL is disabled) the PHY after reset
>>> if
>>> the interface is down and ethtool diagnostics are not currently
>>> running.
>>>
>>> Similar problem was see on 82571 controller and was fixed in e1000e
>>> driver
>>> by Bruce Allan.
>>> Please refer commit 31dbe5b4ac6fca72dec946e4d0fa7f0913f1d9b1 for
>>> details.
>>>
>>> Signed-off-by: Prasanna S. Panchamukhi
>>> <prasanna.panchamukhi@riverbed.com>
>>> ---
>>>   drivers/net/e1000/e1000_ethtool.c |   27 +++++++++++++++++++--------
>>>   drivers/net/e1000/e1000_main.c    |    7 +++++++
>>>   2 files changed, 26 insertions(+), 8 deletions(-)
>> Thanks Prasanna!  I have added the patch to my queue of e1000 patches.
>>
> Prasanna-
> Here is what we found during validating your patch:
>
> The behavior of 82546 device(s) seems to be identical with/without this patch
> applied. 82546GB (LOM), dev_id 1079 powers down (with wol disabled) after
> ifdown, but powers back up after approx. 10 seconds. 82546EB (NIC), dev_id
> 1010 powers down (with wol disabled) after ifdown. Both of the above
> behaviors are the same with and without the patch applied. Also, if this patch
> DID work as expected, it should print a message after a reset, such as "Cannot
> restart autonegotiation: Resource temporarily unavailable", which would mirror
> the behavior of e1000e.
>
Hi Jeff,

Below is the test case we run verify this fix:

$ ethtool -s eth0 wol d
$ ifconfig eth0 up
$ mii-tool eth0
eth0: negotiated 100baseTx-FD, link ok
$ ifconfig eth0 down
$ mii-tool eth0
eth0: no link
$ ethtool -s eth0 autoneg on  (doesn't really matter what we do here)
$ mii-tool eth0
eth0: negotiated 100baseTx-FD, link ok (this should be: eth0: no link)

I will re-run the test&  check if does not fix this.

Thanks
Prasanna



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Prasanna Panchamukhi - March 4, 2011, 10:41 p.m.
On 03/04/2011 10:03 AM, Prasanna Panchamukhi wrote:
> On 03/04/2011 12:19 AM, Jeff Kirsher wrote:
>> On Wed, Feb 23, 2011 at 16:02, Jeff Kirsher<jeffrey.t.kirsher@intel.com>   wrote:
>>> On Tue, 2011-02-22 at 17:25 -0800, prasanna.panchamukhi@riverbed.com
>>> wrote:
>>>> From: Prasanna S. Panchamukhi<prasanna.panchamukhi@riverbed.com>
>>>>
>>>> Some Phys supported by the e1000 driver do not remain powered off
>>>> across
>>>> a reset of the device when the interface is down, e.g. on 82546.
>>>> This patch powers down (only when WoL is disabled) the PHY after reset
>>>> if
>>>> the interface is down and ethtool diagnostics are not currently
>>>> running.
>>>>
>>>> Similar problem was see on 82571 controller and was fixed in e1000e
>>>> driver
>>>> by Bruce Allan.
>>>> Please refer commit 31dbe5b4ac6fca72dec946e4d0fa7f0913f1d9b1 for
>>>> details.
>>>>
>>>> Signed-off-by: Prasanna S. Panchamukhi
>>>> <prasanna.panchamukhi@riverbed.com>
>>>> ---
>>>>    drivers/net/e1000/e1000_ethtool.c |   27 +++++++++++++++++++--------
>>>>    drivers/net/e1000/e1000_main.c    |    7 +++++++
>>>>    2 files changed, 26 insertions(+), 8 deletions(-)
>>> Thanks Prasanna!  I have added the patch to my queue of e1000 patches.
>>>
>> Prasanna-
>> Here is what we found during validating your patch:
>>
>> The behavior of 82546 device(s) seems to be identical with/without this patch
>> applied. 82546GB (LOM), dev_id 1079 powers down (with wol disabled) after
>> ifdown, but powers back up after approx. 10 seconds. 82546EB (NIC), dev_id
>> 1010 powers down (with wol disabled) after ifdown. Both of the above
>> behaviors are the same with and without the patch applied. Also, if this patch
>> DID work as expected, it should print a message after a reset, such as "Cannot
>> restart autonegotiation: Resource temporarily unavailable", which would mirror
>> the behavior of e1000e.
>>
> Hi Jeff,
>
> Below is the test case we run verify this fix:
>
> $ ethtool -s eth0 wol d
> $ ifconfig eth0 up
> $ mii-tool eth0
> eth0: negotiated 100baseTx-FD, link ok
> $ ifconfig eth0 down
> $ mii-tool eth0
> eth0: no link
> $ ethtool -s eth0 autoneg on  (doesn't really matter what we do here)
> $ mii-tool eth0
> eth0: negotiated 100baseTx-FD, link ok (this should be: eth0: no link)
>
> I will re-run the test&   check if does not fix this.
Hi Jeff,

I re-verified it & confirm that, this patch fixes the problem.
Please let me know if you have any issues.

Thanks
Prasanna

> Thanks
> Prasanna
>
>
>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/net/e1000/e1000_ethtool.c b/drivers/net/e1000/e1000_ethtool.c
index f4d0922..d3f18f5 100644
--- a/drivers/net/e1000/e1000_ethtool.c
+++ b/drivers/net/e1000/e1000_ethtool.c
@@ -1554,6 +1554,15 @@  static void e1000_diag_test(struct net_device *netdev,
 	bool if_running = netif_running(netdev);
 
 	set_bit(__E1000_TESTING, &adapter->flags);
+
+	if (!if_running) {
+		e1000_power_up_phy(adapter);
+
+		adapter->hw.wait_autoneg_complete = 1;
+		e1000_reset(adapter);
+		adapter->hw.wait_autoneg_complete = 0;
+	}
+
 	if (eth_test->flags == ETH_TEST_FL_OFFLINE) {
 		/* Offline tests */
 
@@ -1572,8 +1581,6 @@  static void e1000_diag_test(struct net_device *netdev,
 		if (if_running)
 			/* indicate we're in test mode */
 			dev_close(netdev);
-		else
-			e1000_reset(adapter);
 
 		if (e1000_reg_test(adapter, &data[0]))
 			eth_test->flags |= ETH_TEST_FL_FAILED;
@@ -1587,8 +1594,6 @@  static void e1000_diag_test(struct net_device *netdev,
 			eth_test->flags |= ETH_TEST_FL_FAILED;
 
 		e1000_reset(adapter);
-		/* make sure the phy is powered up */
-		e1000_power_up_phy(adapter);
 		if (e1000_loopback_test(adapter, &data[3]))
 			eth_test->flags |= ETH_TEST_FL_FAILED;
 
@@ -1602,19 +1607,25 @@  static void e1000_diag_test(struct net_device *netdev,
 		if (if_running)
 			dev_open(netdev);
 	} else {
-		e_info(hw, "online testing starting\n");
 		/* Online tests */
-		if (e1000_link_test(adapter, &data[4]))
-			eth_test->flags |= ETH_TEST_FL_FAILED;
 
-		/* Online tests aren't run; pass by default */
+		e_info(hw, "online testing starting\n");
+
+		/* register, eeprom, intr and loopback tests not run online */
 		data[0] = 0;
 		data[1] = 0;
 		data[2] = 0;
 		data[3] = 0;
 
+		if (e1000_link_test(adapter, &data[4]))
+			eth_test->flags |= ETH_TEST_FL_FAILED;
+
 		clear_bit(__E1000_TESTING, &adapter->flags);
 	}
+
+	if (!if_running)
+		e1000_reset(adapter);
+
 	msleep_interruptible(4 * 1000);
 }
 
diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c
index bfab140..beec573 100644
--- a/drivers/net/e1000/e1000_main.c
+++ b/drivers/net/e1000/e1000_main.c
@@ -704,6 +704,13 @@  void e1000_reset(struct e1000_adapter *adapter)
 	ew32(VET, ETHERNET_IEEE_VLAN_TYPE);
 
 	e1000_reset_adaptive(hw);
+
+	if (!netif_running(adapter->netdev) &&
+	    !test_bit(__E1000_TESTING, &adapter->flags)) {
+		e1000_power_down_phy(adapter);
+		return;
+	}
+
 	e1000_phy_get_info(hw, &adapter->phy_info);
 
 	e1000_release_manageability(adapter);