Messages in this thread Patch in this message |  | | Subject | Re: [PATCH] e1000e: Assign DPM_FLAG_SMART_SUSPEND and DPM_FLAG_MAY_SKIP_RESUME to speed up s2ram | From | Kai-Heng Feng <> | Date | Fri, 27 Nov 2020 20:20:17 +0800 |
| |
> On Nov 26, 2020, at 22:45, Chen Yu <yu.c.chen@intel.com> wrote: > > On Thu, Nov 26, 2020 at 08:05:02PM +0800, Kai-Heng Feng wrote: >> >> >>> On Nov 26, 2020, at 19:10, Chen Yu <yu.c.chen@intel.com> wrote: >>> >>> On Thu, Nov 26, 2020 at 02:36:42PM +0800, Kai-Heng Feng wrote: >>>>>> >>>>>> What about plugging ethernet cable and using WoL after system is suspended? >>>>>> Commit "e1000e: Exclude device from suspend direct complete optimization" was to address that scenario. >>> [cut] >>>> >>>> I don't think this is right. >>>> Isn't E1000_WUFC_LNKC already set for runtime suspend? >>>> What if WoL doesn't have it set? >>>> >>> After re-taking a look at your description, please let me elaborate more about the scenario. >>> With this patch applied, and with sysfs wake up disabled, the expected behavior is: >>> >>> 1. If NIC is not runtime suspended: >>> 1.1 s2ram suspend -> wufc will be set to 0(no WoL settings), suspend(), suspend_late(), suspend_noirq() >>> 1.2 s2ram resume -> NIC resumes normaly >>> >>> 2. If NIC is runtime suspended: >>> 2.1 s2ram suspend -> wufc set to E1000_WUFC_LNKC, skip the subsequent suspend callbacks. >> >> Is it safe to keep E1000_WUFC_LNKC enabled here? >> >> From commit 6bf6be1127f7 ("e1000e: Do not wake up the system via WOL if device wakeup is disabled"): >> >> /* Runtime suspend should only enable wakeup for link changes */ >> if (runtime) >> wufc = E1000_WUFC_LNKC; >> else if (device_may_wakeup(&pdev->dev)) >> wufc = adapter->wol; >> else >> wufc = 0; >> >> So it has different wakeup settings for runtime suspend and system suspend, either device_may_wakeup() true or false. > Right. >> Or maybe e1000e devs can confirm E1000_WUFC_LNKC is a safe for system suspend? >> > Does 'safe' here mean waking up the system? > For s2ram, whether the NIC can wake up the system from S3 via cable plug is platform > (BIOS) specific. So the wufc settings here is not directly related to system wake up > via plug event.
Thanks for the confirmation. How about a different approach? Simply use direct-complete to let PM core handle the rest:
diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c index b30f00891c03..1d1424a20733 100644 --- a/drivers/net/ethernet/intel/e1000e/netdev.c +++ b/drivers/net/ethernet/intel/e1000e/netdev.c @@ -25,6 +25,7 @@ #include <linux/pm_runtime.h> #include <linux/aer.h> #include <linux/prefetch.h> +#include <linux/suspend.h> #include "e1000.h" @@ -6868,6 +6869,20 @@ static void e1000e_disable_aspm_locked(struct pci_dev *pdev, u16 state) __e1000e_disable_aspm(pdev, state, 1); } +static int e1000e_pm_prepare(struct device *dev) +{ + return pm_runtime_suspended(dev) && + pm_suspend_via_firmware() && + !device_may_wakeup(dev); +} + +static void e1000e_pm_complete(struct device *dev) +{ + /* Detect link change */ + if (pm_runtime_suspended(dev)) + pm_request_resume(dev); +} + static int e1000e_pm_thaw(struct device *dev) { struct net_device *netdev = dev_get_drvdata(dev); @@ -7665,9 +7680,7 @@ static int e1000_probe(struct pci_dev *pdev, const struct pci_device_id *ent) e1000_print_device_info(adapter); - dev_pm_set_driver_flags(&pdev->dev, DPM_FLAG_NO_DIRECT_COMPLETE); - - if (pci_dev_run_wake(pdev) && hw->mac.type < e1000_pch_cnp) + if (pci_dev_run_wake(pdev)) pm_runtime_put_noidle(&pdev->dev); return 0; @@ -7890,6 +7903,8 @@ MODULE_DEVICE_TABLE(pci, e1000_pci_tbl); static const struct dev_pm_ops e1000_pm_ops = { #ifdef CONFIG_PM_SLEEP + .prepare = e1000e_pm_prepare, + .complete = e1000e_pm_complete, .suspend = e1000e_pm_suspend, .resume = e1000e_pm_resume, .freeze = e1000e_pm_freeze,
> thanks, > Chenyu
|  |