| Message ID | 20201124153221.11265-1-yu.c.chen@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | e1000e: Assign DPM_FLAG_SMART_SUSPEND and DPM_FLAG_MAY_SKIP_RESUME to speed up s2ram | expand |
Hi Yu, > On Nov 24, 2020, at 23:32, Chen Yu <yu.c.chen@intel.com> wrote: > > The NIC is put in runtime suspend status when there is no wire connected. > As a result, it is safe to keep this NIC in runtime suspended during s2ram > because the system does not rely on the NIC plug event nor WOL to wake up > the system. Unlike the s2idle, s2ram does not need to manipulate S0ix settings > during suspend. Please see below for the reason why I explicitly disable direct-complete in the driver. > > This patch assigns DPM_FLAG_SMART_SUSPEND and DPM_FLAG_MAY_SKIP_RESUME > to the e1000e driver so that the s2ram could skip the .suspend_late(), > .suspend_noirq() and .resume_noirq() .resume_early() when possible. > Also skip .suspend() and .resume() if dev_pm_skip_suspend() and > dev_pm_skip_resume() return true, so as to speed up the s2ram. If we really want direct-complete here, maybe always set current WoL setting in runtime suspend routine? > > Signed-off-by: Chen Yu <yu.c.chen@intel.com> > --- > drivers/base/power/main.c | 2 ++ > drivers/net/ethernet/intel/e1000e/netdev.c | 14 +++++++++++++- > 2 files changed, 15 insertions(+), 1 deletion(-) > > diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c > index c7ac49042cee..9cd8abba8612 100644 > --- a/drivers/base/power/main.c > +++ b/drivers/base/power/main.c > @@ -580,6 +580,7 @@ bool dev_pm_skip_resume(struct device *dev) > > return !dev->power.must_resume; > } > +EXPORT_SYMBOL_GPL(dev_pm_skip_resume); I don't think it's a good idea to use this predicate out side of PM core, must_resume may change during suspend process. > > /** > * device_resume_noirq - Execute a "noirq resume" callback for given device. > @@ -2010,3 +2011,4 @@ bool dev_pm_skip_suspend(struct device *dev) > return dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND) && > pm_runtime_status_suspended(dev); > } > +EXPORT_SYMBOL_GPL(dev_pm_skip_suspend); > diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c > index b30f00891c03..d79fddabc553 100644 > --- a/drivers/net/ethernet/intel/e1000e/netdev.c > +++ b/drivers/net/ethernet/intel/e1000e/netdev.c > @@ -6965,6 +6965,14 @@ static __maybe_unused int e1000e_pm_suspend(struct device *dev) > struct e1000_hw *hw = &adapter->hw; > int rc; > > + /* Runtime suspended means that there is no wired connection > + * and it has nothing to do with WOL that, we don't need to > + * adjust the WOL settings. So it is safe to put NIC in > + * runtime suspend while doing system suspend. > + */ 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. > + if (dev_pm_skip_suspend(dev)) > + return 0; > + > e1000e_flush_lpic(pdev); > > e1000e_pm_freeze(dev); > @@ -6989,6 +6997,9 @@ static __maybe_unused int e1000e_pm_resume(struct device *dev) > struct e1000_hw *hw = &adapter->hw; > int rc; > > + if (dev_pm_skip_resume(dev)) > + return 0; > + > /* Introduce S0ix implementation */ > if (hw->mac.type >= e1000_pch_cnp && > !e1000e_check_me(hw->adapter->pdev->device)) > @@ -7665,7 +7676,8 @@ 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); > + dev_pm_set_driver_flags(&pdev->dev, DPM_FLAG_NO_DIRECT_COMPLETE | > + DPM_FLAG_SMART_SUSPEND | DPM_FLAG_MAY_SKIP_RESUME); > > if (pci_dev_run_wake(pdev) && hw->mac.type < e1000_pch_cnp) > pm_runtime_put_noidle(&pdev->dev); Also, most e1000e device on modern platforms doesn't runtime suspend at all after commit "e1000e: Disable runtime PM on CNP+". Kai-Heng > -- > 2.25.1 >
Hi Paul, On Tue, Nov 24, 2020 at 04:47:30PM +0100, Paul Menzel wrote: > Dear Chen, > > > Thank you for the patch. > Thanks for reviewing this change. > Am 24.11.20 um 16:32 schrieb Chen Yu: > > The NIC is put in runtime suspend status when there is no wire connected. > > As a result, it is safe to keep this NIC in runtime suspended during s2ram > > because the system does not rely on the NIC plug event nor WOL to wake up > > the system. Unlike the s2idle, s2ram does not need to manipulate S0ix settings > > during suspend. > > what happens, when I plug in a cable, when the suspend is in ACPI S3 state? > I guess it’s ignored? > I think it depends on the platform(or BIOS implementation). On my platform it is ignored. When the system is running, the plug event would generate a SCI, but if it is in S3, whether to generate wake up event or not depends on the BIOS and the sysfs whether the device is device_may_wakeup(). In summary, whether the NIC is in runtime_suspend() or system_suspended does not impact the wake up from S3 by plug event. > > This patch assigns DPM_FLAG_SMART_SUSPEND and DPM_FLAG_MAY_SKIP_RESUME > > to the e1000e driver so that the s2ram could skip the .suspend_late(), > > .suspend_noirq() and .resume_noirq() .resume_early() when possible. > > Also skip .suspend() and .resume() if dev_pm_skip_suspend() and > > dev_pm_skip_resume() return true, so as to speed up the s2ram. > > What is sped up? Suspend or resume? > Both suspend and resume. > Please also document, what system you tested this on, and what the numbers > before and after are. The platform I'm testing on a laptop with i5-8300H CPU and I219-LM NIC. Before this change: [ 203.391465] e1000e 0000:00:1f.6: pci_pm_suspend+0x0/0x170 returned 0 after 323186 usecs [ 203.598307] e1000e 0000:00:1f.6: pci_pm_suspend_late+0x0/0x40 returned 0 after 4 usecs [ 203.654026] e1000e 0000:00:1f.6: pci_pm_suspend_noirq+0x0/0x290 returned 0 after 20915 usecs [ 203.714464] e1000e 0000:00:1f.6: pci_pm_resume_noirq+0x0/0x120 returned 0 after 19952 usecs [ 203.716208] e1000e 0000:00:1f.6: pci_pm_resume_early+0x0/0x30 returned 0 after 0 usecs [ 203.934399] e1000e 0000:00:1f.6: pci_pm_resume+0x0/0x90 returned 0 after 211437 usecs After this change: [ 150.455612] e1000e 0000:00:1f.6: pci_pm_suspend+0x0/0x170 returned 0 after 14 usecs [ 150.987627] e1000e 0000:00:1f.6: pci_pm_suspend_late+0x0/0x40 returned 0 after 3 usecs [ 151.021659] e1000e 0000:00:1f.6: pci_pm_suspend_noirq+0x0/0x290 returned 0 after 1 usecs [ 151.087303] e1000e 0000:00:1f.6: pci_pm_resume_noirq+0x0/0x120 returned 0 after 0 usecs [ 151.112056] e1000e 0000:00:1f.6: pci_pm_resume_early+0x0/0x30 returned 0 after 0 usecs [ 151.136508] e1000e 0000:00:1f.6: pci_pm_resume+0x0/0x90 returned 0 after 3030 usecs > > If there is a bug report, please note it too. > This is an optimization for scenario when cable is unpluged, so there's no dedicated bug report on this. > > Signed-off-by: Chen Yu <yu.c.chen@intel.com> > > --- > > drivers/base/power/main.c | 2 ++ > > drivers/net/ethernet/intel/e1000e/netdev.c | 14 +++++++++++++- > > 2 files changed, 15 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c > > index c7ac49042cee..9cd8abba8612 100644 > > --- a/drivers/base/power/main.c > > +++ b/drivers/base/power/main.c > > @@ -580,6 +580,7 @@ bool dev_pm_skip_resume(struct device *dev) > > return !dev->power.must_resume; > > } > > +EXPORT_SYMBOL_GPL(dev_pm_skip_resume); > > /** > > * device_resume_noirq - Execute a "noirq resume" callback for given device. > > @@ -2010,3 +2011,4 @@ bool dev_pm_skip_suspend(struct device *dev) > > return dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND) && > > pm_runtime_status_suspended(dev); > > } > > +EXPORT_SYMBOL_GPL(dev_pm_skip_suspend); > > diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c > > index b30f00891c03..d79fddabc553 100644 > > --- a/drivers/net/ethernet/intel/e1000e/netdev.c > > +++ b/drivers/net/ethernet/intel/e1000e/netdev.c > > @@ -6965,6 +6965,14 @@ static __maybe_unused int e1000e_pm_suspend(struct device *dev) > > struct e1000_hw *hw = &adapter->hw; > > int rc; > > + /* Runtime suspended means that there is no wired connection > > Maybe explicitly use *cable* in here to avoid confusion? > Okay. > > + * and it has nothing to do with WOL that, we don't need to > > Move the comma before *that*? > Okay. > > + * adjust the WOL settings. So it is safe to put NIC in > > + * runtime suspend while doing system suspend. > > I understood, that the NIC is already in runtime suspend? Could you please > clarify the comment? > Yes, it is already runtime suspended, I'll revise the comment. Thanks, Chenyu > > + */ > > + if (dev_pm_skip_suspend(dev)) > > + return 0; > > + > > e1000e_flush_lpic(pdev); > > e1000e_pm_freeze(dev); > > @@ -6989,6 +6997,9 @@ static __maybe_unused int e1000e_pm_resume(struct device *dev) > > struct e1000_hw *hw = &adapter->hw; > > int rc; > > + if (dev_pm_skip_resume(dev)) > > + return 0; > > + > > /* Introduce S0ix implementation */ > > if (hw->mac.type >= e1000_pch_cnp && > > !e1000e_check_me(hw->adapter->pdev->device)) > > @@ -7665,7 +7676,8 @@ 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); > > + dev_pm_set_driver_flags(&pdev->dev, DPM_FLAG_NO_DIRECT_COMPLETE | > > + DPM_FLAG_SMART_SUSPEND | DPM_FLAG_MAY_SKIP_RESUME); > > if (pci_dev_run_wake(pdev) && hw->mac.type < e1000_pch_cnp) > > pm_runtime_put_noidle(&pdev->dev); > > > Kind regards, > > Paul
Hi Kai-Heng, On Wed, Nov 25, 2020 at 01:17:28AM +0800, Kai-Heng Feng wrote: > Hi Yu, > > > On Nov 24, 2020, at 23:32, Chen Yu <yu.c.chen@intel.com> wrote: > > > > The NIC is put in runtime suspend status when there is no wire connected. > > As a result, it is safe to keep this NIC in runtime suspended during s2ram > > because the system does not rely on the NIC plug event nor WOL to wake up > > the system. Unlike the s2idle, s2ram does not need to manipulate S0ix settings > > during suspend. > > Please see below for the reason why I explicitly disable direct-complete in the driver. > Okay. > > > > This patch assigns DPM_FLAG_SMART_SUSPEND and DPM_FLAG_MAY_SKIP_RESUME > > to the e1000e driver so that the s2ram could skip the .suspend_late(), > > .suspend_noirq() and .resume_noirq() .resume_early() when possible. > > Also skip .suspend() and .resume() if dev_pm_skip_suspend() and > > dev_pm_skip_resume() return true, so as to speed up the s2ram. > > If we really want direct-complete here, maybe always set current WoL setting in runtime suspend routine? > Indeed, that would be a choice. > > > > Signed-off-by: Chen Yu <yu.c.chen@intel.com> > > --- > > drivers/base/power/main.c | 2 ++ > > drivers/net/ethernet/intel/e1000e/netdev.c | 14 +++++++++++++- > > 2 files changed, 15 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c > > index c7ac49042cee..9cd8abba8612 100644 > > --- a/drivers/base/power/main.c > > +++ b/drivers/base/power/main.c > > @@ -580,6 +580,7 @@ bool dev_pm_skip_resume(struct device *dev) > > > > return !dev->power.must_resume; > > } > > +EXPORT_SYMBOL_GPL(dev_pm_skip_resume); > > I don't think it's a good idea to use this predicate out side of PM core, must_resume may change during suspend process. > The dev_pm_skip_resume() is used during system resume, not during suspend, so there would be no race condition I suppose? > > > > /** > > * device_resume_noirq - Execute a "noirq resume" callback for given device. > > @@ -2010,3 +2011,4 @@ bool dev_pm_skip_suspend(struct device *dev) > > return dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND) && > > pm_runtime_status_suspended(dev); > > } > > +EXPORT_SYMBOL_GPL(dev_pm_skip_suspend); > > diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c > > index b30f00891c03..d79fddabc553 100644 > > --- a/drivers/net/ethernet/intel/e1000e/netdev.c > > +++ b/drivers/net/ethernet/intel/e1000e/netdev.c > > @@ -6965,6 +6965,14 @@ static __maybe_unused int e1000e_pm_suspend(struct device *dev) > > struct e1000_hw *hw = &adapter->hw; > > int rc; > > > > + /* Runtime suspended means that there is no wired connection > > + * and it has nothing to do with WOL that, we don't need to > > + * adjust the WOL settings. So it is safe to put NIC in > > + * runtime suspend while doing system suspend. > > + */ > > 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. > Yes, this is what I concerned previously. So in order to support this case, let's adjust this by checking if (device_may_wakeup() && dev_pm_skip_suspend()) so that if the user has disabled WOL via sysfs then we do not fall into this optimization commit 6bf6be1127f7 ("e1000e: Do not wake up the system via WOL if device wakeup is disabled") > > + if (dev_pm_skip_suspend(dev)) > > + return 0; > > + > > e1000e_flush_lpic(pdev); > > > > e1000e_pm_freeze(dev); > > @@ -6989,6 +6997,9 @@ static __maybe_unused int e1000e_pm_resume(struct device *dev) > > struct e1000_hw *hw = &adapter->hw; > > int rc; > > > > + if (dev_pm_skip_resume(dev)) > > + return 0; > > + > > /* Introduce S0ix implementation */ > > if (hw->mac.type >= e1000_pch_cnp && > > !e1000e_check_me(hw->adapter->pdev->device)) > > @@ -7665,7 +7676,8 @@ 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); > > + dev_pm_set_driver_flags(&pdev->dev, DPM_FLAG_NO_DIRECT_COMPLETE | > > + DPM_FLAG_SMART_SUSPEND | DPM_FLAG_MAY_SKIP_RESUME); > > > > if (pci_dev_run_wake(pdev) && hw->mac.type < e1000_pch_cnp) > > pm_runtime_put_noidle(&pdev->dev); > > Also, most e1000e device on modern platforms doesn't runtime suspend at all after commit "e1000e: Disable runtime PM on CNP+". > Yes, I did some hack on this to make runtime suspend work. As we do have more newer NICs to come, how about removing the restriction of runtime suspend and let the user determine whether to enable the runtime suspend via echo 'on' or 'auto' via sysfs's control. thanks, Chenyu > Kai-Heng > > > -- > > 2.25.1 > > >
> On Nov 25, 2020, at 18:36, Chen Yu <yu.c.chen@intel.com> wrote: > > Hi Kai-Heng, > On Wed, Nov 25, 2020 at 01:17:28AM +0800, Kai-Heng Feng wrote: >> Hi Yu, >> >>> On Nov 24, 2020, at 23:32, Chen Yu <yu.c.chen@intel.com> wrote: >>> >>> The NIC is put in runtime suspend status when there is no wire connected. >>> As a result, it is safe to keep this NIC in runtime suspended during s2ram >>> because the system does not rely on the NIC plug event nor WOL to wake up >>> the system. Unlike the s2idle, s2ram does not need to manipulate S0ix settings >>> during suspend. >> >> Please see below for the reason why I explicitly disable direct-complete in the driver. >> > Okay. >>> >>> This patch assigns DPM_FLAG_SMART_SUSPEND and DPM_FLAG_MAY_SKIP_RESUME >>> to the e1000e driver so that the s2ram could skip the .suspend_late(), >>> .suspend_noirq() and .resume_noirq() .resume_early() when possible. >>> Also skip .suspend() and .resume() if dev_pm_skip_suspend() and >>> dev_pm_skip_resume() return true, so as to speed up the s2ram. >> >> If we really want direct-complete here, maybe always set current WoL setting in runtime suspend routine? >> > Indeed, that would be a choice. >>> >>> Signed-off-by: Chen Yu <yu.c.chen@intel.com> >>> --- >>> drivers/base/power/main.c | 2 ++ >>> drivers/net/ethernet/intel/e1000e/netdev.c | 14 +++++++++++++- >>> 2 files changed, 15 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c >>> index c7ac49042cee..9cd8abba8612 100644 >>> --- a/drivers/base/power/main.c >>> +++ b/drivers/base/power/main.c >>> @@ -580,6 +580,7 @@ bool dev_pm_skip_resume(struct device *dev) >>> >>> return !dev->power.must_resume; >>> } >>> +EXPORT_SYMBOL_GPL(dev_pm_skip_resume); >> >> I don't think it's a good idea to use this predicate out side of PM core, must_resume may change during suspend process. >> > The dev_pm_skip_resume() is used during system resume, not during suspend, so > there would be no race condition I suppose? I think it's better to let PM core to decide. >>> >>> /** >>> * device_resume_noirq - Execute a "noirq resume" callback for given device. >>> @@ -2010,3 +2011,4 @@ bool dev_pm_skip_suspend(struct device *dev) >>> return dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND) && >>> pm_runtime_status_suspended(dev); >>> } >>> +EXPORT_SYMBOL_GPL(dev_pm_skip_suspend); >>> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c >>> index b30f00891c03..d79fddabc553 100644 >>> --- a/drivers/net/ethernet/intel/e1000e/netdev.c >>> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c >>> @@ -6965,6 +6965,14 @@ static __maybe_unused int e1000e_pm_suspend(struct device *dev) >>> struct e1000_hw *hw = &adapter->hw; >>> int rc; >>> >>> + /* Runtime suspended means that there is no wired connection >>> + * and it has nothing to do with WOL that, we don't need to >>> + * adjust the WOL settings. So it is safe to put NIC in >>> + * runtime suspend while doing system suspend. >>> + */ >> >> 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. >> > Yes, this is what I concerned previously. So in order to support this case, > let's adjust this by checking > if (device_may_wakeup() && dev_pm_skip_suspend()) > > so that if the user has disabled WOL via sysfs then we do not fall > into this optimization > commit 6bf6be1127f7 ("e1000e: Do not wake up the system via WOL if > device wakeup is disabled") 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? >>> + if (dev_pm_skip_suspend(dev)) >>> + return 0; >>> + >>> e1000e_flush_lpic(pdev); >>> >>> e1000e_pm_freeze(dev); >>> @@ -6989,6 +6997,9 @@ static __maybe_unused int e1000e_pm_resume(struct device *dev) >>> struct e1000_hw *hw = &adapter->hw; >>> int rc; >>> >>> + if (dev_pm_skip_resume(dev)) >>> + return 0; >>> + >>> /* Introduce S0ix implementation */ >>> if (hw->mac.type >= e1000_pch_cnp && >>> !e1000e_check_me(hw->adapter->pdev->device)) >>> @@ -7665,7 +7676,8 @@ 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); >>> + dev_pm_set_driver_flags(&pdev->dev, DPM_FLAG_NO_DIRECT_COMPLETE | >>> + DPM_FLAG_SMART_SUSPEND | DPM_FLAG_MAY_SKIP_RESUME); >>> >>> if (pci_dev_run_wake(pdev) && hw->mac.type < e1000_pch_cnp) >>> pm_runtime_put_noidle(&pdev->dev); >> >> Also, most e1000e device on modern platforms doesn't runtime suspend at all after commit "e1000e: Disable runtime PM on CNP+". >> > Yes, I did some hack on this to make runtime suspend work. > As we do have more newer NICs to come, how about removing the > restriction of runtime suspend and let the user determine whether > to enable the runtime suspend via echo 'on' or 'auto' via > sysfs's control. There's a discussion on enable runtime PM by default for all PCI devices. So removing this workaround will expose the bug for users. Let me get the system with the bug (Latitude 5500) and see if latest ACPI code can fix the GPE bug. Kai-Heng > > thanks, > Chenyu >> Kai-Heng >> >>> -- >>> 2.25.1
On Thu, Nov 26, 2020 at 02:36:42PM +0800, Kai-Heng Feng wrote: > > > > On Nov 25, 2020, at 18:36, Chen Yu <yu.c.chen@intel.com> wrote: > > > > Hi Kai-Heng, > > On Wed, Nov 25, 2020 at 01:17:28AM +0800, Kai-Heng Feng wrote: > >> Hi Yu, > >> > >>> On Nov 24, 2020, at 23:32, Chen Yu <yu.c.chen@intel.com> wrote: > >>> > >>> The NIC is put in runtime suspend status when there is no wire connected. > >>> As a result, it is safe to keep this NIC in runtime suspended during s2ram > >>> because the system does not rely on the NIC plug event nor WOL to wake up > >>> the system. Unlike the s2idle, s2ram does not need to manipulate S0ix settings > >>> during suspend. > >> > >> Please see below for the reason why I explicitly disable direct-complete in the driver. > >> > > Okay. > >>> > >>> This patch assigns DPM_FLAG_SMART_SUSPEND and DPM_FLAG_MAY_SKIP_RESUME > >>> to the e1000e driver so that the s2ram could skip the .suspend_late(), > >>> .suspend_noirq() and .resume_noirq() .resume_early() when possible. > >>> Also skip .suspend() and .resume() if dev_pm_skip_suspend() and > >>> dev_pm_skip_resume() return true, so as to speed up the s2ram. > >> > >> If we really want direct-complete here, maybe always set current WoL setting in runtime suspend routine? > >> > > Indeed, that would be a choice. > >>> > >>> Signed-off-by: Chen Yu <yu.c.chen@intel.com> > >>> --- > >>> drivers/base/power/main.c | 2 ++ > >>> drivers/net/ethernet/intel/e1000e/netdev.c | 14 +++++++++++++- > >>> 2 files changed, 15 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c > >>> index c7ac49042cee..9cd8abba8612 100644 > >>> --- a/drivers/base/power/main.c > >>> +++ b/drivers/base/power/main.c > >>> @@ -580,6 +580,7 @@ bool dev_pm_skip_resume(struct device *dev) > >>> > >>> return !dev->power.must_resume; > >>> } > >>> +EXPORT_SYMBOL_GPL(dev_pm_skip_resume); > >> > >> I don't think it's a good idea to use this predicate out side of PM core, must_resume may change during suspend process. > >> > > The dev_pm_skip_resume() is used during system resume, not during suspend, so > > there would be no race condition I suppose? > > I think it's better to let PM core to decide. > Humm, drivers/acpi/acpi_lpss.c alread used it in acpi_lpss_resume_early(), so e1000e is not the only one that wants to leverage this interface : ) > >>> > >>> /** > >>> * device_resume_noirq - Execute a "noirq resume" callback for given device. > >>> @@ -2010,3 +2011,4 @@ bool dev_pm_skip_suspend(struct device *dev) > >>> return dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND) && > >>> pm_runtime_status_suspended(dev); > >>> } > >>> +EXPORT_SYMBOL_GPL(dev_pm_skip_suspend); > >>> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c > >>> index b30f00891c03..d79fddabc553 100644 > >>> --- a/drivers/net/ethernet/intel/e1000e/netdev.c > >>> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c > >>> @@ -6965,6 +6965,14 @@ static __maybe_unused int e1000e_pm_suspend(struct device *dev) > >>> struct e1000_hw *hw = &adapter->hw; > >>> int rc; > >>> > >>> + /* Runtime suspended means that there is no wired connection > >>> + * and it has nothing to do with WOL that, we don't need to > >>> + * adjust the WOL settings. So it is safe to put NIC in > >>> + * runtime suspend while doing system suspend. > >>> + */ > >> > >> 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. > >> > > Yes, this is what I concerned previously. So in order to support this case, > > let's adjust this by checking > > if (device_may_wakeup() && dev_pm_skip_suspend()) > > > > so that if the user has disabled WOL via sysfs then we do not fall > > into this optimization > > commit 6bf6be1127f7 ("e1000e: Do not wake up the system via WOL if > > device wakeup is disabled") > > 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? > I did not quite get what your meaning is. First, it was a typo, please check v2 patch set, it is: if (dev_pm_skip_suspend() && !device_may_wakeup(dev)) if the NIC is runtime suspended, it means that, device_may_wakeup() return true, the code will continue to execute. In summary, if the NIC is a wake up device, we don't fall into the optimization. > >>> + if (dev_pm_skip_suspend(dev)) > >>> + return 0; > >>> + > >>> e1000e_flush_lpic(pdev); > >>> > >>> e1000e_pm_freeze(dev); > >>> @@ -6989,6 +6997,9 @@ static __maybe_unused int e1000e_pm_resume(struct device *dev) > >>> struct e1000_hw *hw = &adapter->hw; > >>> int rc; > >>> > >>> + if (dev_pm_skip_resume(dev)) > >>> + return 0; > >>> + > >>> /* Introduce S0ix implementation */ > >>> if (hw->mac.type >= e1000_pch_cnp && > >>> !e1000e_check_me(hw->adapter->pdev->device)) > >>> @@ -7665,7 +7676,8 @@ 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); > >>> + dev_pm_set_driver_flags(&pdev->dev, DPM_FLAG_NO_DIRECT_COMPLETE | > >>> + DPM_FLAG_SMART_SUSPEND | DPM_FLAG_MAY_SKIP_RESUME); > >>> > >>> if (pci_dev_run_wake(pdev) && hw->mac.type < e1000_pch_cnp) > >>> pm_runtime_put_noidle(&pdev->dev); > >> > >> Also, most e1000e device on modern platforms doesn't runtime suspend at all after commit "e1000e: Disable runtime PM on CNP+". > >> > > Yes, I did some hack on this to make runtime suspend work. > > As we do have more newer NICs to come, how about removing the > > restriction of runtime suspend and let the user determine whether > > to enable the runtime suspend via echo 'on' or 'auto' via > > sysfs's control. > > There's a discussion on enable runtime PM by default for all PCI devices. > So removing this workaround will expose the bug for users. > > Let me get the system with the bug (Latitude 5500) and see if latest ACPI code can fix the GPE bug. > There is sysfs for user to disable runtime suspend. If there is an issue on that platform and if we don't want to break the user space(because the blacklist is already there), I think we should only disable the runtime suspend on that platform, but not blocking other platforms IMO. thanks, Chenyu > Kai-Hengs > > > > > thanks, > > Chenyu > >> Kai-Heng > >> > >>> -- > >>> 2.25.1 >
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. 2.2 s2ram resume -> skip the subsequent resume callbacks. If between 2.1 and 2.2, the cable is plugged, the user is unable to use WoL to wake up the system. But if the sysfs wake up is enabled, the code logic falls into the old path, and the user can wake up the system via WoL by plugging the cable, and send packages to the system. Or do I miss something? thanks, Chenyu
> 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. Or maybe e1000e devs can confirm E1000_WUFC_LNKC is a safe for system suspend? Kai-Heng > 2.2 s2ram resume -> skip the subsequent resume callbacks. > > If between 2.1 and 2.2, the cable is plugged, the user is unable to use WoL to wake up > the system. > > But if the sysfs wake up is enabled, the code logic falls into the old path, and > the user can wake up the system via WoL by plugging the cable, and send packages to the > system. Or do I miss something? > > thanks, > Chenyu > >
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, Chenyu
> 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
On Fri, Nov 27, 2020 at 08:20:17PM +0800, Kai-Heng Feng wrote: > Thanks for the confirmation. How about a different approach? > Simply use direct-complete to let PM core handle the rest: > Thanks for your suggestion and sorry about replying too late. Yes, using direct-complete could leverage pm core to do that, although this is a little different than my original thought to mainly skip the resume process. But anyway, I'll use direct complete and send a v3 out. > 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); > +} device_may_wakeup() is not needed as pm core will check it anyway. > + > +static void e1000e_pm_complete(struct device *dev) > +{ > + /* Detect link change */ > + if (pm_runtime_suspended(dev)) > + pm_request_resume(dev); > +} There is no need to force resume the device, just keep it runtime suspended would be okay. Besides the pm core's complete() will restore runtime usage_count which is increased by pm core's prepare(), so there is no need to do that here again. > + > 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); > - DPM_FLAG_SMART_PREPARE must be set otherwise pci subsystem will ignore the .prepare() from the driver. > - if (pci_dev_run_wake(pdev) && hw->mac.type < e1000_pch_cnp) > + if (pci_dev_run_wake(pdev)) > pm_runtime_put_noidle(&pdev->dev); > I would prefer to only disable runtime for cnp, in case of any user regressions. thanks, Chenyu
diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c index c7ac49042cee..9cd8abba8612 100644 --- a/drivers/base/power/main.c +++ b/drivers/base/power/main.c @@ -580,6 +580,7 @@ bool dev_pm_skip_resume(struct device *dev) return !dev->power.must_resume; } +EXPORT_SYMBOL_GPL(dev_pm_skip_resume); /** * device_resume_noirq - Execute a "noirq resume" callback for given device. @@ -2010,3 +2011,4 @@ bool dev_pm_skip_suspend(struct device *dev) return dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND) && pm_runtime_status_suspended(dev); } +EXPORT_SYMBOL_GPL(dev_pm_skip_suspend); diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c index b30f00891c03..d79fddabc553 100644 --- a/drivers/net/ethernet/intel/e1000e/netdev.c +++ b/drivers/net/ethernet/intel/e1000e/netdev.c @@ -6965,6 +6965,14 @@ static __maybe_unused int e1000e_pm_suspend(struct device *dev) struct e1000_hw *hw = &adapter->hw; int rc; + /* Runtime suspended means that there is no wired connection + * and it has nothing to do with WOL that, we don't need to + * adjust the WOL settings. So it is safe to put NIC in + * runtime suspend while doing system suspend. + */ + if (dev_pm_skip_suspend(dev)) + return 0; + e1000e_flush_lpic(pdev); e1000e_pm_freeze(dev); @@ -6989,6 +6997,9 @@ static __maybe_unused int e1000e_pm_resume(struct device *dev) struct e1000_hw *hw = &adapter->hw; int rc; + if (dev_pm_skip_resume(dev)) + return 0; + /* Introduce S0ix implementation */ if (hw->mac.type >= e1000_pch_cnp && !e1000e_check_me(hw->adapter->pdev->device)) @@ -7665,7 +7676,8 @@ 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); + dev_pm_set_driver_flags(&pdev->dev, DPM_FLAG_NO_DIRECT_COMPLETE | + DPM_FLAG_SMART_SUSPEND | DPM_FLAG_MAY_SKIP_RESUME); if (pci_dev_run_wake(pdev) && hw->mac.type < e1000_pch_cnp) pm_runtime_put_noidle(&pdev->dev);
The NIC is put in runtime suspend status when there is no wire connected. As a result, it is safe to keep this NIC in runtime suspended during s2ram because the system does not rely on the NIC plug event nor WOL to wake up the system. Unlike the s2idle, s2ram does not need to manipulate S0ix settings during suspend. This patch assigns DPM_FLAG_SMART_SUSPEND and DPM_FLAG_MAY_SKIP_RESUME to the e1000e driver so that the s2ram could skip the .suspend_late(), .suspend_noirq() and .resume_noirq() .resume_early() when possible. Also skip .suspend() and .resume() if dev_pm_skip_suspend() and dev_pm_skip_resume() return true, so as to speed up the s2ram. Signed-off-by: Chen Yu <yu.c.chen@intel.com> --- drivers/base/power/main.c | 2 ++ drivers/net/ethernet/intel/e1000e/netdev.c | 14 +++++++++++++- 2 files changed, 15 insertions(+), 1 deletion(-)