Re: [PATCH] e1000e: Assign DPM_FLAG_SMART_SUSPEND and DPM_FLAG_MAY_SKIP_RESUME to speed up s2ram

From: Kai-Heng Feng
Date: Fri Nov 27 2020 - 07:20:45 EST




> On Nov 26, 2020, at 22:45, Chen Yu <yu.c.chen@xxxxxxxxx> 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@xxxxxxxxx> 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