e1000: the power down when running ifdown command

PATCH NAK Naohiro Ooiwa 5 participants 12 messages Oct 31, 2009 – Nov 5, 2009

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Naohiro Ooiwa

author

2009-10-31 09:39 UTC #0 lore raw

PATCH 1/1 e1000: the power down when running ifdown command

Hi All

I resend my patch.
Sorry, my previous mail lacked an explanation.

The e1000 driver doesn't let the power down when running ifdown command. So, I set to the D3hot state of a PCI device at the end of e1000_close().

With this modification, e1000 driver reduces power by ifdown. It's about 6 watts when I measured a total power of one server machine in my environment.

I tested this patch. The result is good enough to me.

Could you please check my patch ?
If I should have other considerations, please tell me.

Thanks you.
Naohiro Ooiwa

Signed-off-by Naohiro Ooiwa <[email protected]>

---
 drivers/net/e1000/e1000_main.c |   32 ++++++++++++++++++++++++++++++++
 1 files changed, 32 insertions(+), 0 deletions(-)

drivers/net/e1000/e1000_main.c +32 -0

index bcd192c..12e1a42 100644
@@ -26,6 +26,11 @@
 
 *******************************************************************************/
 
+/*
+ * define this if you want pci save power while ifdown.
+ */
+#define E1000_PCI_POWER_SAVE
+
 #include "e1000.h"
 #include <net/ip6_checksum.h>

@@ -1248,6 +1253,23 @@ static int e1000_open(struct net_device *netdev)
 	struct e1000_hw *hw = &adapter->hw;
 	int err;
 
+#ifdef E1000_PCI_POWER_SAVE
+	struct pci_dev *pdev = adapter->pdev;
+
+	pci_set_power_state(pdev, PCI_D0);
+	pci_restore_state(pdev);
+
+	if (adapter->need_ioport)
+		err = pci_enable_device(pdev);
+	else
+		err = pci_enable_device_mem(pdev);
+	if (err) {
+		printk(KERN_ERR "e1000: Cannot enable PCI device from power-save\n");
+		return err;
+	}
+	pci_set_master(pdev);
+#endif
+
 	/* disallow open during test */
 	if (test_bit(__E1000_TESTING, &adapter->flags))
 		return -EBUSY;

@@ -1265,6 +1287,7 @@ static int e1000_open(struct net_device *netdev)
 		goto err_setup_rx;
 
 	e1000_power_up_phy(adapter);
+	e1000_reset(adapter);
 
 	adapter->mng_vlan_id = E1000_MNG_VLAN_NONE;
 	if ((hw->mng_cookie.status &

@@ -1341,6 +1364,15 @@ static int e1000_close(struct net_device *netdev)
 		e1000_vlan_rx_kill_vid(netdev, adapter->mng_vlan_id);
 	}
 
+#ifdef E1000_PCI_POWER_SAVE
+#ifdef CONFIG_PM
+	pci_save_state(adapter->pdev);
+#endif
+	pci_disable_device(adapter->pdev);
+	pci_wake_from_d3(adapter->pdev, true);
+	pci_set_power_state(adapter->pdev, PCI_D3hot);
+#endif
+
 	return 0;
 }

Signature · 11 lines

1.5.4.1

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

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

Stephen Hemminger

2009-10-31 17:58 UTC #1 lore raw

Quoted 9 lines · On Sat, 31 Oct 2009 18:39:52 +0900 Naohiro Ooiwa <[email protected]>…

Hi All
I resend my patch.
Sorry, my previous mail lacked an explanation.
The e1000 driver doesn't let the power down when running ifdown command. So, I set to the D3hot state of a PCI device at the end of e1000_close().

Quoted 9 lines

With this modification, e1000 driver reduces power by ifdown. It's about 6 watts when I measured a total power of one server machine in my environment.
I tested this patch. The result is good enough to me.
Could you please check my patch ?
If I should have other considerations, please tell me.

Does this work with Wake On Lan?

------------------------------------------------------------------------------
Come build with us! The BlackBerry(R) Developer Conference in SF, CA is the only developer event you need to attend this year. Jumpstart your developing skills, take BlackBerry mobile applications to market and stay ahead of the curve. Join us from November 9 - 12, 2009. Register now!
http://p.sf.net/sfu/devconference

Naohiro Ooiwa

author

2009-11-02 01:29 UTC #2 lore raw

Quoted 16 lines · Stephen Hemminger

Quoted 13 lines · On Sat, 31 Oct 2009 18:39:52 +0900 Naohiro Ooiwa <[email protected]>…
Hi All
I resend my patch.
Sorry, my previous mail lacked an explanation.
The e1000 driver doesn't let the power down when running ifdown command. So, I set to the D3hot state of a PCI device at the end of e1000_close().
With this modification, e1000 driver reduces power by ifdown. It's about 6 watts when I measured a total power of one server machine in my environment.

Quoted 5 lines

Quoted 5 lines
I tested this patch. The result is good enough to me.
Could you please check my patch ?
If I should have other considerations, please tell me.

Hi Stephen

Thank you so much for your reply.

Quoted 1 line

Does this work with Wake On Lan?

Yes, it works WOL.
But I worry that my test is enough.

They are following:
- simple data transmission after ifdown;ifup.
- enable wol, ifup network device, system shutdown, and make sure wol work.
- enable wol, ifdown network device, system shutdown, and make sure wol work.
- while [ 0 ] ; do ifdown eth0 ; ifup eth0 ; done
- while [ 0 ] ; do modprobe e1000 ; rmmod e1000 ; done

drivers/net/e1000/e1000_main.c static int e1000_open(struct net_device *netdev) · line 1293

Quoted 8 lines

drivers/net/e1000/e1000_main.c +1 -0

@@ -1265,6 +1287,7 @@ static int e1000_open(struct net_device *netdev)
 		goto err_setup_rx;
 
 	e1000_power_up_phy(adapter);
+	e1000_reset(adapter);
 
 	adapter->mng_vlan_id = E1000_MNG_VLAN_NONE;
 	if ((hw->mng_cookie.status &

This code fix problem that e1000 driver doesn't work to auto-negotiation once in a while.
Maybe, the cause is that set state to D0 just before it.
I found it by repeat of ifup and ifdown.

If you find out other points and any necessary tests from my patch, please tell me. I will make sure them.

Thanks you.
Naohiro Ooiwa

Naohiro Ooiwa

author

2009-11-04 10:23 UTC #3 lore raw

Quoted 7 lines · Naohiro Ooiwa

Quoted 4 lines · Stephen Hemminger
On Sat, 31 Oct 2009 18:39:52 +0900
Naohiro Ooiwa <[email protected]> wrote:
Does this work with Wake On Lan?
Yes, it works WOL.

Sorry, I made a mistake.
The WOL doesn't work when my patch applied to kernel. I wasn't myself.

I consider the WOL and I will resent the patch.
Thank you for your point.

thanks,
Naohiro Ooiwa

Quoted 35 lines

But I worry that my test is enough.
They are following:
- simple data transmission after ifdown;ifup.
- enable wol, ifup network device, system shutdown, and make sure wol work.
- enable wol, ifdown network device, system shutdown, and make sure wol work.
- while [ 0 ] ; do ifdown eth0 ; ifup eth0 ; done
- while [ 0 ] ; do modprobe e1000 ; rmmod e1000 ; done
drivers/net/e1000/e1000_main.c static int e1000_open(struct net_device *netdev) · line 1293
Quoted 8 lines
drivers/net/e1000/e1000_main.c +1 -0
@@ -1265,6 +1287,7 @@ static int e1000_open(struct net_device *netdev)
 		goto err_setup_rx;
 
 	e1000_power_up_phy(adapter);
+	e1000_reset(adapter);
 
 	adapter->mng_vlan_id = E1000_MNG_VLAN_NONE;
 	if ((hw->mng_cookie.status &
This code fix problem that e1000 driver doesn't work to auto-negotiation once in a while.
Maybe, the cause is that set state to D0 just before it.
I found it by repeat of ifup and ifdown.
If you find out other points and any necessary tests from my patch, please tell me. I will make sure them.
Thanks you.
Naohiro Ooiwa
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Stephen Hemminger

2009-11-04 16:09 UTC #4 lore raw

Quoted 13 lines · On Wed, 04 Nov 2009 19:23:43 +0900 Naohiro Ooiwa <[email protected]>…

Quoted 7 lines · Naohiro Ooiwa
Quoted 4 lines · Stephen Hemminger
On Sat, 31 Oct 2009 18:39:52 +0900
Naohiro Ooiwa <[email protected]> wrote:
Does this work with Wake On Lan?
Yes, it works WOL.
Sorry, I made a mistake.
The WOL doesn't work when my patch applied to kernel. I wasn't myself.

Quoted 6 lines

I consider the WOL and I will resent the patch.
Thank you for your point.
thanks,
Naohiro Ooiwa

Good, thank you for checking. I like the idea of powering down the PHY. Some of the drivers have shutdown hooks to power PHY back on during shutdown to enable WOL.

Allan, Bruce W

2009-11-04 17:57 UTC #5 lore raw

FWIW, in e1000e the PHY is already powered down when the interface is brought down if WoL and manageability are disabled. There is an issue where the PHY remains powered on when the driver is removed for which I have a patch already queued up here at Intel.

Quoted 53 lines

-----Original Message-----
From: Stephen Hemminger [mailto:[email protected]]
Sent: Wednesday, November 04, 2009 8:09 AM
To: Naohiro Ooiwa
Cc: [email protected]; [email protected]; Brandeburg, Jesse; Ronciak, John; Kirsher, Jeffrey T; Andrew Morton; [email protected]; [email protected]
Subject: Re: [E1000-devel] [PATCH] e1000: the power down when running ifdown command
Quoted 13 lines · On Wed, 04 Nov 2009 19:23:43 +0900 Naohiro Ooiwa <[email protected]>…
Quoted 7 lines · Naohiro Ooiwa
Quoted 4 lines · Stephen Hemminger
On Sat, 31 Oct 2009 18:39:52 +0900
Naohiro Ooiwa <[email protected]> wrote:
Does this work with Wake On Lan?
Yes, it works WOL.
Sorry, I made a mistake.
The WOL doesn't work when my patch applied to kernel. I wasn't myself.
Quoted 6 lines
I consider the WOL and I will resent the patch.
Thank you for your point.
thanks,
Naohiro Ooiwa
Good, thank you for checking. I like the idea of powering down the PHY. Some of the drivers have shutdown hooks to power PHY back on during shutdown to enable WOL.
--
--------------------------------------------------------------------------
----
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30- Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with Crystal Reports now. http://p.sf.net/sfu/bobj-july
_______________________________________________
E1000-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/e1000-devel

Naohiro Ooiwa

author

2009-11-05 14:58 UTC #6 lore raw

In reply to Stephen Hemminger

Quoted 18 lines · Stephen Hemminger

Quoted 15 lines · On Wed, 04 Nov 2009 19:23:43 +0900 Naohiro Ooiwa <[email protected]>…
Quoted 6 lines · Naohiro Ooiwa
Quoted 4 lines · Stephen Hemminger
On Sat, 31 Oct 2009 18:39:52 +0900
Naohiro Ooiwa <[email protected]> wrote:
Does this work with Wake On Lan?
Yes, it works WOL.
Sorry, I made a mistake.
The WOL doesn't work when my patch applied to kernel. I wasn't myself.
I consider the WOL and I will resent the patch.
Thank you for your point.

Quoted 8 lines

Quoted 2 lines
thanks,
Naohiro Ooiwa
Good, thank you for checking. I like the idea of powering down the PHY. Some of the drivers have shutdown hooks to power PHY back on during shutdown to enable WOL.

Thank you so much for your infomation.
Oh, The shutdown hooks is useful.

Thanks.
Naohiro Ooiwa

Jeff Kirsher

2009-11-03 00:26 UTC #7 lore raw

In reply to Naohiro Ooiwa

2009/10/31 Naohiro Ooiwa <[email protected]>:

Quoted 83 lines

Hi All

I resend my patch.
Sorry, my previous mail lacked an explanation.

The e1000 driver doesn't let the power down when running ifdown command. So, I set to the D3hot state of a PCI device at the end of e1000_close().

With this modification, e1000 driver reduces power by ifdown. It's about 6 watts when I measured a total power of one server machine in my environment.

I tested this patch. The result is good enough to me.

Could you please check my patch ?
If I should have other considerations, please tell me.

Thanks you.
Naohiro Ooiwa

Signed-off-by Naohiro Ooiwa <[email protected]>

---
drivers/net/e1000/e1000_main.c | 32 ++++++++++++++++++++++++++++++++

 1 files changed, 32 insertions(+), 0 deletions(-)

drivers/net/e1000/e1000_main.c +23 -0

index bcd192c..12e1a42 100644
@@ -26,6 +26,11 @@
 
  *******************************************************************************/
 
+/*
+ * define this if you want pci save power while ifdown.
+ */
+#define E1000_PCI_POWER_SAVE
+
  #include "e1000.h"
  #include <net/ip6_checksum.h>

@@ -1248,6 +1253,23 @@ static int e1000_open(struct net_device *netdev)
        struct e1000_hw *hw = &adapter->hw;
        int err;
 
+#ifdef E1000_PCI_POWER_SAVE
+       struct pci_dev *pdev = adapter->pdev;
+
+       pci_set_power_state(pdev, PCI_D0);
+       pci_restore_state(pdev);
+
+       if (adapter->need_ioport)
+               err = pci_enable_device(pdev);
+       else
+               err = pci_enable_device_mem(pdev);
+       if (err) {
+               printk(KERN_ERR "e1000: Cannot enable PCI device from power-save\n");
+               return err;
+       }
+       pci_set_master(pdev);
+#endif
+
        /* disallow open during test */
        if (test_bit(__E1000_TESTING, &adapter->flags))
                return -EBUSY;

@@ -1265,6 +1287,7 @@ static int e1000_open(struct net_device *netdev)
                goto err_setup_rx;
 
        e1000_power_up_phy(adapter);
+       e1000_reset(adapter);
 
        adapter->mng_vlan_id = E1000_MNG_VLAN_NONE;

if ((hw->mng_cookie.status &

drivers/net/e1000/e1000_main.c +2 -0

@@ -1341,6 +1364,15 @@ static int e1000_close(struct net_device *netdev)
                e1000_vlan_rx_kill_vid(netdev, adapter->mng_vlan_id);
        }
 
+#ifdef E1000_PCI_POWER_SAVE
+#ifdef CONFIG_PM

drivers/net/e1000/e1000_main.c static int e1000_close(struct net_device *netdev) · line 1378

Quoted 10 lines

drivers/net/e1000/e1000_main.c +7 -0

+       pci_save_state(adapter->pdev);
+#endif
+       pci_disable_device(adapter->pdev);
+       pci_wake_from_d3(adapter->pdev, true);
+       pci_set_power_state(adapter->pdev, PCI_D3hot);
+#endif
+
        return 0;
  }

I have added this patch to my tree for testing. This patch requires a fair amount of regression testing, so once its passed testing I will push the patch to David/netdev.

Signature · 2 lines

Cheers,
Jeff

Dan Williams

2009-11-03 00:56 UTC #8 lore raw

Quoted 82 lines · Jeff Kirsher

2009/10/31 Naohiro Ooiwa <[email protected]>:

Quoted 81 lines

Hi All

I resend my patch.
Sorry, my previous mail lacked an explanation.

The e1000 driver doesn't let the power down when running ifdown command. So, I set to the D3hot state of a PCI device at the end of e1000_close().

With this modification, e1000 driver reduces power by ifdown. It's about 6 watts when I measured a total power of one server machine in my environment.

I tested this patch. The result is good enough to me.

Could you please check my patch ?
If I should have other considerations, please tell me.

Thanks you.
Naohiro Ooiwa

Signed-off-by Naohiro Ooiwa <[email protected]>

---
 drivers/net/e1000/e1000_main.c |   32 ++++++++++++++++++++++++++++++++
 1 files changed, 32 insertions(+), 0 deletions(-)

drivers/net/e1000/e1000_main.c +23 -0

index bcd192c..12e1a42 100644
@@ -26,6 +26,11 @@
 
 *******************************************************************************/
 
+/*
+ * define this if you want pci save power while ifdown.
+ */
+#define E1000_PCI_POWER_SAVE
+
 #include "e1000.h"
 #include <net/ip6_checksum.h>

@@ -1248,6 +1253,23 @@ static int e1000_open(struct net_device *netdev)
       struct e1000_hw *hw = &adapter->hw;
       int err;
 
+#ifdef E1000_PCI_POWER_SAVE
+       struct pci_dev *pdev = adapter->pdev;
+
+       pci_set_power_state(pdev, PCI_D0);
+       pci_restore_state(pdev);
+
+       if (adapter->need_ioport)
+               err = pci_enable_device(pdev);
+       else
+               err = pci_enable_device_mem(pdev);
+       if (err) {
+               printk(KERN_ERR "e1000: Cannot enable PCI device from power-save\n");
+               return err;
+       }
+       pci_set_master(pdev);
+#endif
+
       /* disallow open during test */
       if (test_bit(__E1000_TESTING, &adapter->flags))
               return -EBUSY;

@@ -1265,6 +1287,7 @@ static int e1000_open(struct net_device *netdev)
               goto err_setup_rx;
 
       e1000_power_up_phy(adapter);
+       e1000_reset(adapter);
 
       adapter->mng_vlan_id = E1000_MNG_VLAN_NONE;
       if ((hw->mng_cookie.status &

@@ -1341,6 +1364,15 @@ static int e1000_close(struct net_device *netdev)
               e1000_vlan_rx_kill_vid(netdev, adapter->mng_vlan_id);
       }

Quoted 16 lines

drivers/net/e1000/e1000_main.c static int e1000_close(struct net_device *netdev) · line 1378
Quoted 12 lines
drivers/net/e1000/e1000_main.c +9 -0
+#ifdef E1000_PCI_POWER_SAVE
+#ifdef CONFIG_PM
+       pci_save_state(adapter->pdev);
+#endif
+       pci_disable_device(adapter->pdev);
+       pci_wake_from_d3(adapter->pdev, true);
+       pci_set_power_state(adapter->pdev, PCI_D3hot);
+#endif
+
       return 0;
 }
 
I have added this patch to my tree for testing. This patch requires a fair amount of regression testing, so once its passed testing I will push the patch to David/netdev.

Can we get rid of E1000_PCI_POWER_SAVE and just enable this unconditionally once it's baked? Having the define there is pretty superfluous right now. Either that, or make it CONFIG_E1000_PCI_POWER_SAVE and make a Kconfig option for it that depends on experimental or something. Or a module parameter. Or something other than a dangling #define :)

Dan

Naohiro Ooiwa

author

2009-11-03 10:06 UTC #9 lore raw

In reply to Jeff Kirsher

Quoted 5 lines · Jeff Kirsher

2009/10/31 Naohiro Ooiwa <[email protected]>:
I have added this patch to my tree for testing. This patch requires a fair amount of regression testing, so once its passed testing I will push the patch to David/netdev.

I appreciate the marge your tree.
If there is anything I can do, please let me know.

And I know this patch is good for e100 driver too. I would really like to create patch for it. How do you think about e100 driver.

Thanks,
Naohiro Ooiwa

Jeff Kirsher

NAK

2009-11-03 21:37 UTC #10 lore raw

2009/11/3 Naohiro Ooiwa <[email protected]>:

Quoted 10 lines

Quoted 5 lines · Jeff Kirsher
2009/10/31 Naohiro Ooiwa <[email protected]>:
I have added this patch to my tree for testing. This patch requires a fair amount of regression testing, so once its passed testing I will push the patch to David/netdev.
I appreciate the marge your tree.
If there is anything I can do, please let me know.

Quoted 8 lines

And I know this patch is good for e100 driver too. I would really like to create patch for it. How do you think about e100 driver.
Thanks,
Naohiro Ooiwa

Patches are always welcome (referring to a e100 patch).

As far as the e1000 patch goes, it has a number of issues which were found in testing. Here are just a few problems we saw:
1. ethtool -t - crashes the system
2. ethtool eth0 - always shows link/speed as 1000/Full even when there is no cable
3. ethtool -s eth0 autoneg on/off - system hang. Sometimes a copper interface will show up as fiber after this.
4. ethtool -d/-S/-g etc - will corrupt the stats of the interface while doing ifup/down

So it appears that more needs to be done to the driver to get this to work as expected.

NAK

Signature · 2 lines

Cheers,
Jeff

Naohiro Ooiwa

author

2009-11-04 10:27 UTC #11 lore raw

Quoted 15 lines · Jeff Kirsher

2009/11/3 Naohiro Ooiwa <[email protected]>:
Quoted 14 lines
Quoted 5 lines · Jeff Kirsher
2009/10/31 Naohiro Ooiwa <[email protected]>:
I have added this patch to my tree for testing. This patch requires a fair amount of regression testing, so once its passed testing I will push the patch to David/netdev.
I appreciate the marge your tree.
If there is anything I can do, please let me know.
And I know this patch is good for e100 driver too. I would really like to create patch for it. How do you think about e100 driver.

Quoted 5 lines

Quoted 3 lines
Thanks,
Naohiro Ooiwa
Patches are always welcome (referring to a e100 patch).

I am happy that you should say that.
I will try to create a patch for e100, e1000e, igb and ixgbe. Before that, I should fix the following problems.

Quoted 9 lines

As far as the e1000 patch goes, it has a number of issues which were found in testing. Here are just a few problems we saw:
1. ethtool -t - crashes the system
2. ethtool eth0 - always shows link/speed as 1000/Full even when there is no cable
3. ethtool -s eth0 autoneg on/off - system hang. Sometimes a copper interface will show up as fiber after this.
4. ethtool -d/-S/-g etc - will corrupt the stats of the interface while doing ifup/down

Thank you for your tests.
Oh, My patch is full of problem.
The rest is my work.

I will resend the patch after test of all ethtool's options. At that time, I will tell you contents of my tests.

And I sad WOL works on my patch in previous mail.
But WOL doesn't work. Sorry, I wasn't myself.
I will fix it too.

Quoted 6 lines

So it appears that more needs to be done to the driver to get this to work as expected.
NAK