On 12/21/2011 04:42 AM, Alexander Duyck wrote: > I am curious on if you have done any testing on an actual igb device with > this code enabled? My main concern is that the PHY is integrated into these > parts, thus putting the MAC into D3 will result in behaviour changes in the > PHY. Specifically in the case of D3 the PHY on many of the igb parts will go > into a low power link up mode, and may exclude a 1Gb/s link. If this occurs > I would expect to see a number of link issues on the device. > I tested the code on a 82576 dual ports adapter, didn't noticed any issue.
> From what I can tell it looks like if you were to perform any ethtool > operations while the link is down and this code is enabled it would likely > result in multiple errors. I am not sure what would happen if you were to > run an "ethtool -t" test while the device was in a powered down state. > This is not big problem because netif_device_detach() is called when suspending a device. "ethtool -t" reports "No such device" if the device is suspended. (There is a bug my patch, it doesn't calls netif_device_detach() if the device is down. Will send a new patch later) > Also have you done any tests to see what the actual power savings of this > patch might be? I'm concerned there may be none due to the fact that putting > a single function of an igb device into D3 will not result in any power > savings. In order to see any power savings you would need to put all of the > functions on a given MAC into D3hot before the device will actually switch > the PCIe bus to L1. On e1000e parts this made sense since most parts are > only single port, however since igb supports dual and quad port adapters I > don't know how effective this would be. > Yes, there is no power saving unless all functions are put into D3hot. I used a power meter to measure the power consumption, about 1 watt was saved if both ports of 82576 were put into D3hot. Regards Yan, Zheng > Thanks, > > Alex > > On 12/18/2011 09:07 PM, Yan, Zheng wrote: >> Use the runtime power management framework to add basic runtime PM support >> to the igb driver. Namely, make the driver suspend the device when the link >> is off and set it up for generating a wakeup event after the link has been >> detected again. Also make the driver suspend the device when the interface >> is being shut down. This feature is disabled by default. >> >> Based on e1000e's runtime PM code. >> >> Signed-off-by: Zheng Yan<[email protected]> >> --- >> drivers/net/ethernet/intel/igb/igb_main.c | 151 >> ++++++++++++++++++++++++----- >> 1 files changed, 126 insertions(+), 25 deletions(-) >> >> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c >> b/drivers/net/ethernet/intel/igb/igb_main.c >> index 89d576c..aac529b 100644 >> --- a/drivers/net/ethernet/intel/igb/igb_main.c >> +++ b/drivers/net/ethernet/intel/igb/igb_main.c >> @@ -53,6 +53,7 @@ >> #include<linux/if_ether.h> >> #include<linux/aer.h> >> #include<linux/prefetch.h> >> +#include<linux/pm_runtime.h> >> #ifdef CONFIG_IGB_DCA >> #include<linux/dca.h> >> #endif >> @@ -172,8 +173,18 @@ static int igb_check_vf_assignment(struct igb_adapter >> *adapter); >> #endif >> >> #ifdef CONFIG_PM >> -static int igb_suspend(struct pci_dev *, pm_message_t); >> -static int igb_resume(struct pci_dev *); >> +static int igb_suspend(struct device *); >> +static int igb_resume(struct device *); >> +#ifdef CONFIG_PM_RUNTIME >> +static int igb_runtime_suspend(struct device *dev); >> +static int igb_runtime_resume(struct device *dev); >> +static int igb_runtime_idle(struct device *dev); >> +#endif >> +static const struct dev_pm_ops igb_pm_ops = { >> + SET_SYSTEM_SLEEP_PM_OPS(igb_suspend, igb_resume) >> + SET_RUNTIME_PM_OPS(igb_runtime_suspend, igb_runtime_resume, >> + igb_runtime_idle) >> +}; >> #endif >> static void igb_shutdown(struct pci_dev *); >> #ifdef CONFIG_IGB_DCA >> @@ -214,9 +225,7 @@ static struct pci_driver igb_driver = { >> .probe = igb_probe, >> .remove = __devexit_p(igb_remove), >> #ifdef CONFIG_PM >> - /* Power Management Hooks */ >> - .suspend = igb_suspend, >> - .resume = igb_resume, >> + .driver.pm =&igb_pm_ops, >> #endif >> .shutdown = igb_shutdown, >> .err_handler =&igb_err_handler >> @@ -2111,6 +2120,8 @@ static int __devinit igb_probe(struct pci_dev *pdev, >> default: >> break; >> } >> + >> + pm_runtime_put_noidle(&pdev->dev); >> return 0; >> >> err_register: >> @@ -2150,6 +2161,8 @@ static void __devexit igb_remove(struct pci_dev *pdev) >> struct igb_adapter *adapter = netdev_priv(netdev); >> struct e1000_hw *hw =&adapter->hw; >> >> + pm_runtime_get_noresume(&pdev->dev); >> + >> /* >> * The watchdog timer may be rescheduled, so explicitly >> * disable watchdog from being rescheduled. >> @@ -2472,16 +2485,23 @@ static int __devinit igb_sw_init(struct igb_adapter >> *adapter) >> * handler is registered with the OS, the watchdog timer is started, >> * and the stack is notified that the interface is ready. >> **/ >> -static int igb_open(struct net_device *netdev) >> +static int __igb_open(struct net_device *netdev, bool resuming) >> { >> struct igb_adapter *adapter = netdev_priv(netdev); >> struct e1000_hw *hw =&adapter->hw; >> + struct pci_dev *pdev = adapter->pdev; >> int err; >> int i; >> >> - /* disallow open during test */ >> - if (test_bit(__IGB_TESTING,&adapter->state)) >> - return -EBUSY; >> + if (!resuming) { >> + /* disallow open during test */ >> + if (test_bit(__IGB_TESTING,&adapter->state)) >> + return -EBUSY; >> + pm_runtime_get_sync(&pdev->dev); >> + } else if (test_bit(__IGB_TESTING,&adapter->state)) { >> + while (test_bit(__IGB_TESTING,&adapter->state)) >> + msleep(100); >> + } >> >> netif_carrier_off(netdev); >> >> @@ -2527,6 +2547,9 @@ static int igb_open(struct net_device *netdev) >> >> netif_tx_start_all_queues(netdev); >> >> + if (!resuming) >> + pm_runtime_put(&pdev->dev); >> + >> /* start the watchdog. */ >> hw->mac.get_link_status = 1; >> schedule_work(&adapter->watchdog_task); >> @@ -2541,10 +2564,17 @@ err_setup_rx: >> igb_free_all_tx_resources(adapter); >> err_setup_tx: >> igb_reset(adapter); >> + if (!resuming) >> + pm_runtime_put(&pdev->dev); >> >> return err; >> } >> >> +static int igb_open(struct net_device *netdev) >> +{ >> + return __igb_open(netdev, false); >> +} >> + >> /** >> * igb_close - Disables a network interface >> * @netdev: network interface device structure >> @@ -2556,21 +2586,32 @@ err_setup_tx: >> * needs to be disabled. A global MAC reset is issued to stop the >> * hardware, and all transmit and receive resources are freed. >> **/ >> -static int igb_close(struct net_device *netdev) >> +static int __igb_close(struct net_device *netdev, bool suspending) >> { >> struct igb_adapter *adapter = netdev_priv(netdev); >> + struct pci_dev *pdev = adapter->pdev; >> >> WARN_ON(test_bit(__IGB_RESETTING,&adapter->state)); >> - igb_down(adapter); >> >> + if (!suspending) >> + pm_runtime_get_sync(&pdev->dev); >> + >> + igb_down(adapter); >> igb_free_irq(adapter); >> >> igb_free_all_tx_resources(adapter); >> igb_free_all_rx_resources(adapter); >> >> + if (!suspending) >> + pm_runtime_put_sync(&pdev->dev); >> return 0; >> } >> >> +static int igb_close(struct net_device *netdev) >> +{ >> + return __igb_close(netdev, false); >> +} >> + >> /** >> * igb_setup_tx_resources - allocate Tx resources (Descriptors) >> * @tx_ring: tx descriptor ring (for a specific queue) to setup >> @@ -3630,6 +3671,9 @@ static void igb_watchdog_task(struct work_struct *work) >> >> link = igb_has_link(adapter); >> if (link) { >> + /* Cancel scheduled suspend requests. */ >> + pm_runtime_resume(netdev->dev.parent); >> + >> if (!netif_carrier_ok(netdev)) { >> u32 ctrl; >> hw->mac.ops.get_speed_and_duplex(hw, >> @@ -3701,6 +3745,9 @@ static void igb_watchdog_task(struct work_struct *work) >> if (!test_bit(__IGB_DOWN,&adapter->state)) >> mod_timer(&adapter->phy_info_timer, >> round_jiffies(jiffies + 2 * HZ)); >> + >> + pm_schedule_suspend(netdev->dev.parent, >> + MSEC_PER_SEC * 5); >> } >> } >> >> @@ -6583,21 +6630,24 @@ err_inval: >> return -EINVAL; >> } >> >> -static int __igb_shutdown(struct pci_dev *pdev, bool *enable_wake) >> +static int __igb_shutdown(struct pci_dev *pdev, bool *enable_wake, >> + bool runtime) >> { >> struct net_device *netdev = pci_get_drvdata(pdev); >> struct igb_adapter *adapter = netdev_priv(netdev); >> struct e1000_hw *hw =&adapter->hw; >> u32 ctrl, rctl, status; >> - u32 wufc = adapter->wol; >> + u32 wufc = runtime ? E1000_WUFC_LNKC : adapter->wol; >> #ifdef CONFIG_PM >> int retval = 0; >> #endif >> >> - netif_device_detach(netdev); >> - >> - if (netif_running(netdev)) >> - igb_close(netdev); >> + if (netif_running(netdev)) { >> + netif_device_detach(netdev); >> + __igb_close(netdev, true); >> + } else { >> + wufc&= ~E1000_WUFC_LNKC; >> + } >> >> igb_clear_interrupt_scheme(adapter); >> >> @@ -6656,12 +6706,13 @@ static int __igb_shutdown(struct pci_dev *pdev, bool >> *enable_wake) >> } >> >> #ifdef CONFIG_PM >> -static int igb_suspend(struct pci_dev *pdev, pm_message_t state) >> +static int igb_suspend(struct device *dev) >> { >> int retval; >> bool wake; >> + struct pci_dev *pdev = to_pci_dev(dev); >> >> - retval = __igb_shutdown(pdev,&wake); >> + retval = __igb_shutdown(pdev,&wake, 0); >> if (retval) >> return retval; >> >> @@ -6675,8 +6726,9 @@ static int igb_suspend(struct pci_dev *pdev, >> pm_message_t state) >> return 0; >> } >> >> -static int igb_resume(struct pci_dev *pdev) >> +static int igb_resume(struct device *dev) >> { >> + struct pci_dev *pdev = to_pci_dev(dev); >> struct net_device *netdev = pci_get_drvdata(pdev); >> struct igb_adapter *adapter = netdev_priv(netdev); >> struct e1000_hw *hw =&adapter->hw; >> @@ -6697,7 +6749,18 @@ static int igb_resume(struct pci_dev *pdev) >> pci_enable_wake(pdev, PCI_D3hot, 0); >> pci_enable_wake(pdev, PCI_D3cold, 0); >> >> - if (igb_init_interrupt_scheme(adapter)) { >> + if (!rtnl_is_locked()) { >> + /* >> + * shut up ASSERT_RTNL() warning in >> + * netif_set_real_num_tx/rx_queues. >> + */ >> + rtnl_lock(); >> + err = igb_init_interrupt_scheme(adapter); >> + rtnl_unlock(); >> + } else { >> + err = igb_init_interrupt_scheme(adapter); >> + } >> + if (err) { >> dev_err(&pdev->dev, "Unable to allocate memory for queues\n"); >> return -ENOMEM; >> } >> @@ -6710,23 +6773,61 @@ static int igb_resume(struct pci_dev *pdev) >> >> wr32(E1000_WUS, ~0); >> >> - if (netif_running(netdev)) { >> - err = igb_open(netdev); >> + if (netdev->flags& IFF_UP) { >> + err = __igb_open(netdev, true); >> if (err) >> return err; >> + netif_device_attach(netdev); >> } >> >> - netif_device_attach(netdev); >> + return 0; >> +} >> + >> +#ifdef CONFIG_PM_RUNTIME >> +static int igb_runtime_idle(struct device *dev) >> +{ >> + struct pci_dev *pdev = to_pci_dev(dev); >> + struct net_device *netdev = pci_get_drvdata(pdev); >> + struct igb_adapter *adapter = netdev_priv(netdev); >> + >> + if (!netif_running(netdev) || !igb_has_link(adapter)) >> + pm_schedule_suspend(dev, MSEC_PER_SEC * 5); >> + >> + return -EBUSY; >> +} >> + >> +static int igb_runtime_suspend(struct device *dev) >> +{ >> + struct pci_dev *pdev = to_pci_dev(dev); >> + int retval; >> + bool wake; >> + >> + retval = __igb_shutdown(pdev,&wake, 1); >> + if (retval) >> + return retval; >> + >> + if (wake) { >> + pci_prepare_to_sleep(pdev); >> + } else { >> + pci_wake_from_d3(pdev, false); >> + pci_set_power_state(pdev, PCI_D3hot); >> + } >> >> return 0; >> } >> + >> +static int igb_runtime_resume(struct device *dev) >> +{ >> + return igb_resume(dev); >> +} >> +#endif /* CONFIG_PM_RUNTIME */ >> #endif >> >> static void igb_shutdown(struct pci_dev *pdev) >> { >> bool wake; >> >> - __igb_shutdown(pdev,&wake); >> + __igb_shutdown(pdev,&wake, 0); >> >> if (system_state == SYSTEM_POWER_OFF) { >> pci_wake_from_d3(pdev, wake); > ------------------------------------------------------------------------------ Write once. Port to many. Get the SDK and tools to simplify cross-platform app development. Create new or port existing apps to sell to consumers worldwide. Explore the Intel AppUpSM program developer opportunity. appdeveloper.intel.com/join http://p.sf.net/sfu/intel-appdev _______________________________________________ E1000-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/e1000-devel To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired
