| Message ID | 4AEC05E8.40600@miraclelinux.com |
|---|---|
| State | Awaiting Upstream, archived |
| Delegated to: | David Miller |
| Headers | show |
On Sat, 31 Oct 2009 18:39:52 +0900 Naohiro Ooiwa <nooiwa@miraclelinux.com> wrote: > 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. > Does this work with Wake On Lan? -- 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
Stephen Hemminger wrote: > On Sat, 31 Oct 2009 18:39:52 +0900 > Naohiro Ooiwa <nooiwa@miraclelinux.com> wrote: > >> 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. >> Hi Stephen Thank you so much for your reply. > 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 > @@ -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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2009/10/31 Naohiro Ooiwa <nooiwa@miraclelinux.com>: > 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 <nooiwa@miraclelinux.com> > --- > drivers/net/e1000/e1000_main.c | 32 ++++++++++++++++++++++++++++++++ > 1 files changed, 32 insertions(+), 0 deletions(-) > > diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c > index bcd192c..12e1a42 100644 > --- a/drivers/net/e1000/e1000_main.c > +++ b/drivers/net/e1000/e1000_main.c > @@ -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; > } > 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.
On Mon, 2009-11-02 at 16:26 -0800, Jeff Kirsher wrote: > 2009/10/31 Naohiro Ooiwa <nooiwa@miraclelinux.com>: > > 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 <nooiwa@miraclelinux.com> > > --- > > drivers/net/e1000/e1000_main.c | 32 ++++++++++++++++++++++++++++++++ > > 1 files changed, 32 insertions(+), 0 deletions(-) > > > > diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c > > index bcd192c..12e1a42 100644 > > --- a/drivers/net/e1000/e1000_main.c > > +++ b/drivers/net/e1000/e1000_main.c > > @@ -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; > > } > > > > 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 -- 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
Jeff Kirsher wrote: > 2009/10/31 Naohiro Ooiwa <nooiwa@miraclelinux.com>: > > 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 -- 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
2009/11/3 Naohiro Ooiwa <nooiwa@miraclelinux.com>: > Jeff Kirsher wrote: >> 2009/10/31 Naohiro Ooiwa <nooiwa@miraclelinux.com>: >> >> 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 > 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
Naohiro Ooiwa wrote: > Stephen Hemminger wrote: >> On Sat, 31 Oct 2009 18:39:52 +0900 >> Naohiro Ooiwa <nooiwa@miraclelinux.com> 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 > 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 > > >> @@ -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 majordomo@vger.kernel.org > 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Jeff Kirsher wrote: > 2009/11/3 Naohiro Ooiwa <nooiwa@miraclelinux.com>: >> Jeff Kirsher wrote: >>> 2009/10/31 Naohiro Ooiwa <nooiwa@miraclelinux.com>: >>> >>> 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 >> > > 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. > 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. > > So it appears that more needs to be done to the driver to get this to > work as expected. > > NAK > -- 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
On Wed, 04 Nov 2009 19:23:43 +0900 Naohiro Ooiwa <nooiwa@miraclelinux.com> wrote: > Naohiro Ooiwa wrote: > > Stephen Hemminger wrote: > >> On Sat, 31 Oct 2009 18:39:52 +0900 > >> Naohiro Ooiwa <nooiwa@miraclelinux.com> 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 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.
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. >-----Original Message----- >From: Stephen Hemminger [mailto:shemminger@vyatta.com] >Sent: Wednesday, November 04, 2009 8:09 AM >To: Naohiro Ooiwa >Cc: e1000-devel@lists.sourceforge.net; netdev@vger.kernel.org; Brandeburg, >Jesse; Ronciak, John; Kirsher, Jeffrey T; Andrew Morton; >svaidy@linux.vnet.ibm.com; davem@davemloft.net >Subject: Re: [E1000-devel] [PATCH] e1000: the power down when running >ifdown command > >On Wed, 04 Nov 2009 19:23:43 +0900 >Naohiro Ooiwa <nooiwa@miraclelinux.com> wrote: > >> Naohiro Ooiwa wrote: >> > Stephen Hemminger wrote: >> >> On Sat, 31 Oct 2009 18:39:52 +0900 >> >> Naohiro Ooiwa <nooiwa@miraclelinux.com> 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 > > >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 >E1000-devel@lists.sourceforge.net >https://lists.sourceforge.net/lists/listinfo/e1000-devel -- 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
Stephen Hemminger wrote: > On Wed, 04 Nov 2009 19:23:43 +0900 > Naohiro Ooiwa <nooiwa@miraclelinux.com> wrote: > >> Naohiro Ooiwa wrote: >>> Stephen Hemminger wrote: >>>> On Sat, 31 Oct 2009 18:39:52 +0900 >>>> Naohiro Ooiwa <nooiwa@miraclelinux.com> 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 > > > 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 -- 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
diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c index bcd192c..12e1a42 100644 --- a/drivers/net/e1000/e1000_main.c +++ b/drivers/net/e1000/e1000_main.c @@ -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; }
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 <nooiwa@miraclelinux.com> --- drivers/net/e1000/e1000_main.c | 32 ++++++++++++++++++++++++++++++++ 1 files changed, 32 insertions(+), 0 deletions(-)