* [PATCH] PCI: PM: Avoid possible suspend-to-idle issue @ 2019-05-17 9:08 Rafael J. Wysocki 2019-05-17 15:57 ` Keith Busch ` (2 more replies) 0 siblings, 3 replies; 8+ messages in thread From: Rafael J. Wysocki @ 2019-05-17 9:08 UTC (permalink / raw) To: Linux PCI Cc: Linux PM, Linux ACPI, LKML, Bjorn Helgaas, Mika Westerberg, Keith Busch From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> If a PCI driver leaves the device handled by it in D0 and calls pci_save_state() on the device in its ->suspend() or ->suspend_late() callback, it can expect the device to stay in D0 over the whole s2idle cycle. However, that may not be the case if there is a spurious wakeup while the system is suspended, because in that case pci_pm_suspend_noirq() will run again after pci_pm_resume_noirq() which calls pci_restore_state(), via pci_pm_default_resume_early(), so state_saved is cleared and the second iteration of pci_pm_suspend_noirq() will invoke pci_prepare_to_sleep() which may change the power state of the device. To avoid that, add a new internal flag, skip_bus_pm, that will be set by pci_pm_suspend_noirq() when it runs for the first time during the given system suspend-resume cycle if the state of the device has been saved already and the device is still in D0. Setting that flag will cause the next iterations of pci_pm_suspend_noirq() to set state_saved for pci_pm_resume_noirq(), so that it always restores the device state from the originally saved data, and avoid calling pci_prepare_to_sleep() for the device. Fixes: 33e4f80ee69b ("ACPI / PM: Ignore spurious SCI wakeups from suspend-to-idle") Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> --- drivers/pci/pci-driver.c | 17 ++++++++++++++++- include/linux/pci.h | 1 + 2 files changed, 17 insertions(+), 1 deletion(-) Index: linux-pm/drivers/pci/pci-driver.c =================================================================== --- linux-pm.orig/drivers/pci/pci-driver.c +++ linux-pm/drivers/pci/pci-driver.c @@ -734,6 +734,8 @@ static int pci_pm_suspend(struct device struct pci_dev *pci_dev = to_pci_dev(dev); const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL; + pci_dev->skip_bus_pm = false; + if (pci_has_legacy_pm_support(pci_dev)) return pci_legacy_suspend(dev, PMSG_SUSPEND); @@ -827,7 +829,20 @@ static int pci_pm_suspend_noirq(struct d } } - if (!pci_dev->state_saved) { + if (pci_dev->skip_bus_pm) { + /* + * The function is running for the second time in a row without + * going through full resume, which is possible only during + * suspend-to-idle in a spurious wakeup case. Moreover, the + * device was originally left in D0, so its power state should + * not be changed here and the device register values saved + * originally should be restored on resume again. + */ + pci_dev->state_saved = true; + } else if (pci_dev->state_saved) { + if (pci_dev->current_state == PCI_D0) + pci_dev->skip_bus_pm = true; + } else { pci_save_state(pci_dev); if (pci_power_manageable(pci_dev)) pci_prepare_to_sleep(pci_dev); Index: linux-pm/include/linux/pci.h =================================================================== --- linux-pm.orig/include/linux/pci.h +++ linux-pm/include/linux/pci.h @@ -344,6 +344,7 @@ struct pci_dev { D3cold, not set for devices powered on/off by the corresponding bridge */ + unsigned int skip_bus_pm:1; /* Internal: Skip bus-level PM */ unsigned int ignore_hotplug:1; /* Ignore hotplug events */ unsigned int hotplug_user_indicators:1; /* SlotCtl indicators controlled exclusively by ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] PCI: PM: Avoid possible suspend-to-idle issue 2019-05-17 9:08 [PATCH] PCI: PM: Avoid possible suspend-to-idle issue Rafael J. Wysocki @ 2019-05-17 15:57 ` Keith Busch 2019-05-20 10:39 ` Mika Westerberg 2019-05-27 11:02 ` Rafael J. Wysocki 2 siblings, 0 replies; 8+ messages in thread From: Keith Busch @ 2019-05-17 15:57 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Linux PCI, Linux PM, Linux ACPI, LKML, Bjorn Helgaas, Mika Westerberg On Fri, May 17, 2019 at 11:08:50AM +0200, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > If a PCI driver leaves the device handled by it in D0 and calls > pci_save_state() on the device in its ->suspend() or ->suspend_late() > callback, it can expect the device to stay in D0 over the whole > s2idle cycle. However, that may not be the case if there is a > spurious wakeup while the system is suspended, because in that case > pci_pm_suspend_noirq() will run again after pci_pm_resume_noirq() > which calls pci_restore_state(), via pci_pm_default_resume_early(), > so state_saved is cleared and the second iteration of > pci_pm_suspend_noirq() will invoke pci_prepare_to_sleep() which > may change the power state of the device. > > To avoid that, add a new internal flag, skip_bus_pm, that will be set > by pci_pm_suspend_noirq() when it runs for the first time during the > given system suspend-resume cycle if the state of the device has > been saved already and the device is still in D0. Setting that flag > will cause the next iterations of pci_pm_suspend_noirq() to set > state_saved for pci_pm_resume_noirq(), so that it always restores the > device state from the originally saved data, and avoid calling > pci_prepare_to_sleep() for the device. > > Fixes: 33e4f80ee69b ("ACPI / PM: Ignore spurious SCI wakeups from suspend-to-idle") > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> LGTM Reviewed-by: Keith Busch <keith.busch@intel.com> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] PCI: PM: Avoid possible suspend-to-idle issue 2019-05-17 9:08 [PATCH] PCI: PM: Avoid possible suspend-to-idle issue Rafael J. Wysocki 2019-05-17 15:57 ` Keith Busch @ 2019-05-20 10:39 ` Mika Westerberg 2019-05-27 11:02 ` Rafael J. Wysocki 2 siblings, 0 replies; 8+ messages in thread From: Mika Westerberg @ 2019-05-20 10:39 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Linux PCI, Linux PM, Linux ACPI, LKML, Bjorn Helgaas, Keith Busch On Fri, May 17, 2019 at 11:08:50AM +0200, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > If a PCI driver leaves the device handled by it in D0 and calls > pci_save_state() on the device in its ->suspend() or ->suspend_late() > callback, it can expect the device to stay in D0 over the whole > s2idle cycle. However, that may not be the case if there is a > spurious wakeup while the system is suspended, because in that case > pci_pm_suspend_noirq() will run again after pci_pm_resume_noirq() > which calls pci_restore_state(), via pci_pm_default_resume_early(), > so state_saved is cleared and the second iteration of > pci_pm_suspend_noirq() will invoke pci_prepare_to_sleep() which > may change the power state of the device. > > To avoid that, add a new internal flag, skip_bus_pm, that will be set > by pci_pm_suspend_noirq() when it runs for the first time during the > given system suspend-resume cycle if the state of the device has > been saved already and the device is still in D0. Setting that flag > will cause the next iterations of pci_pm_suspend_noirq() to set > state_saved for pci_pm_resume_noirq(), so that it always restores the > device state from the originally saved data, and avoid calling > pci_prepare_to_sleep() for the device. > > Fixes: 33e4f80ee69b ("ACPI / PM: Ignore spurious SCI wakeups from suspend-to-idle") > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] PCI: PM: Avoid possible suspend-to-idle issue 2019-05-17 9:08 [PATCH] PCI: PM: Avoid possible suspend-to-idle issue Rafael J. Wysocki 2019-05-17 15:57 ` Keith Busch 2019-05-20 10:39 ` Mika Westerberg @ 2019-05-27 11:02 ` Rafael J. Wysocki 2019-06-11 8:39 ` Kai-Heng Feng 2 siblings, 1 reply; 8+ messages in thread From: Rafael J. Wysocki @ 2019-05-27 11:02 UTC (permalink / raw) To: Rafael J. Wysocki, Bjorn Helgaas Cc: Linux PCI, Linux PM, Linux ACPI, LKML, Mika Westerberg, Keith Busch On Friday, May 17, 2019 11:08:50 AM CEST Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > If a PCI driver leaves the device handled by it in D0 and calls > pci_save_state() on the device in its ->suspend() or ->suspend_late() > callback, it can expect the device to stay in D0 over the whole > s2idle cycle. However, that may not be the case if there is a > spurious wakeup while the system is suspended, because in that case > pci_pm_suspend_noirq() will run again after pci_pm_resume_noirq() > which calls pci_restore_state(), via pci_pm_default_resume_early(), > so state_saved is cleared and the second iteration of > pci_pm_suspend_noirq() will invoke pci_prepare_to_sleep() which > may change the power state of the device. > > To avoid that, add a new internal flag, skip_bus_pm, that will be set > by pci_pm_suspend_noirq() when it runs for the first time during the > given system suspend-resume cycle if the state of the device has > been saved already and the device is still in D0. Setting that flag > will cause the next iterations of pci_pm_suspend_noirq() to set > state_saved for pci_pm_resume_noirq(), so that it always restores the > device state from the originally saved data, and avoid calling > pci_prepare_to_sleep() for the device. > > Fixes: 33e4f80ee69b ("ACPI / PM: Ignore spurious SCI wakeups from suspend-to-idle") > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > drivers/pci/pci-driver.c | 17 ++++++++++++++++- > include/linux/pci.h | 1 + > 2 files changed, 17 insertions(+), 1 deletion(-) > > Index: linux-pm/drivers/pci/pci-driver.c > =================================================================== > --- linux-pm.orig/drivers/pci/pci-driver.c > +++ linux-pm/drivers/pci/pci-driver.c > @@ -734,6 +734,8 @@ static int pci_pm_suspend(struct device > struct pci_dev *pci_dev = to_pci_dev(dev); > const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL; > > + pci_dev->skip_bus_pm = false; > + > if (pci_has_legacy_pm_support(pci_dev)) > return pci_legacy_suspend(dev, PMSG_SUSPEND); > > @@ -827,7 +829,20 @@ static int pci_pm_suspend_noirq(struct d > } > } > > - if (!pci_dev->state_saved) { > + if (pci_dev->skip_bus_pm) { > + /* > + * The function is running for the second time in a row without > + * going through full resume, which is possible only during > + * suspend-to-idle in a spurious wakeup case. Moreover, the > + * device was originally left in D0, so its power state should > + * not be changed here and the device register values saved > + * originally should be restored on resume again. > + */ > + pci_dev->state_saved = true; > + } else if (pci_dev->state_saved) { > + if (pci_dev->current_state == PCI_D0) > + pci_dev->skip_bus_pm = true; > + } else { > pci_save_state(pci_dev); > if (pci_power_manageable(pci_dev)) > pci_prepare_to_sleep(pci_dev); > Index: linux-pm/include/linux/pci.h > =================================================================== > --- linux-pm.orig/include/linux/pci.h > +++ linux-pm/include/linux/pci.h > @@ -344,6 +344,7 @@ struct pci_dev { > D3cold, not set for devices > powered on/off by the > corresponding bridge */ > + unsigned int skip_bus_pm:1; /* Internal: Skip bus-level PM */ > unsigned int ignore_hotplug:1; /* Ignore hotplug events */ > unsigned int hotplug_user_indicators:1; /* SlotCtl indicators > controlled exclusively by > Bjorn, I've assumed no concerns or objections from you regarding this one and queued it up. If that assumption is incorrect, please let me know. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] PCI: PM: Avoid possible suspend-to-idle issue 2019-05-27 11:02 ` Rafael J. Wysocki @ 2019-06-11 8:39 ` Kai-Heng Feng 2019-06-11 9:03 ` Rafael J. Wysocki 2019-06-11 21:34 ` Rafael J. Wysocki 0 siblings, 2 replies; 8+ messages in thread From: Kai-Heng Feng @ 2019-06-11 8:39 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Bjorn Helgaas, Linux PCI, Linux PM, Linux ACPI, LKML, Mika Westerberg, Keith Busch Hi Rafael, at 19:02, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > On Friday, May 17, 2019 11:08:50 AM CEST Rafael J. Wysocki wrote: >> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> >> If a PCI driver leaves the device handled by it in D0 and calls >> pci_save_state() on the device in its ->suspend() or ->suspend_late() >> callback, it can expect the device to stay in D0 over the whole >> s2idle cycle. However, that may not be the case if there is a >> spurious wakeup while the system is suspended, because in that case >> pci_pm_suspend_noirq() will run again after pci_pm_resume_noirq() >> which calls pci_restore_state(), via pci_pm_default_resume_early(), >> so state_saved is cleared and the second iteration of >> pci_pm_suspend_noirq() will invoke pci_prepare_to_sleep() which >> may change the power state of the device. >> >> To avoid that, add a new internal flag, skip_bus_pm, that will be set >> by pci_pm_suspend_noirq() when it runs for the first time during the >> given system suspend-resume cycle if the state of the device has >> been saved already and the device is still in D0. Setting that flag >> will cause the next iterations of pci_pm_suspend_noirq() to set >> state_saved for pci_pm_resume_noirq(), so that it always restores the >> device state from the originally saved data, and avoid calling >> pci_prepare_to_sleep() for the device. >> >> Fixes: 33e4f80ee69b ("ACPI / PM: Ignore spurious SCI wakeups from >> suspend-to-idle") >> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> I just found out this patch has a chance to freeze or reboot the system during suspend cycles. What information do you need to debug? Kai-Heng >> --- >> drivers/pci/pci-driver.c | 17 ++++++++++++++++- >> include/linux/pci.h | 1 + >> 2 files changed, 17 insertions(+), 1 deletion(-) >> >> Index: linux-pm/drivers/pci/pci-driver.c >> =================================================================== >> --- linux-pm.orig/drivers/pci/pci-driver.c >> +++ linux-pm/drivers/pci/pci-driver.c >> @@ -734,6 +734,8 @@ static int pci_pm_suspend(struct device >> struct pci_dev *pci_dev = to_pci_dev(dev); >> const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL; >> >> + pci_dev->skip_bus_pm = false; >> + >> if (pci_has_legacy_pm_support(pci_dev)) >> return pci_legacy_suspend(dev, PMSG_SUSPEND); >> >> @@ -827,7 +829,20 @@ static int pci_pm_suspend_noirq(struct d >> } >> } >> >> - if (!pci_dev->state_saved) { >> + if (pci_dev->skip_bus_pm) { >> + /* >> + * The function is running for the second time in a row without >> + * going through full resume, which is possible only during >> + * suspend-to-idle in a spurious wakeup case. Moreover, the >> + * device was originally left in D0, so its power state should >> + * not be changed here and the device register values saved >> + * originally should be restored on resume again. >> + */ >> + pci_dev->state_saved = true; >> + } else if (pci_dev->state_saved) { >> + if (pci_dev->current_state == PCI_D0) >> + pci_dev->skip_bus_pm = true; >> + } else { >> pci_save_state(pci_dev); >> if (pci_power_manageable(pci_dev)) >> pci_prepare_to_sleep(pci_dev); >> Index: linux-pm/include/linux/pci.h >> =================================================================== >> --- linux-pm.orig/include/linux/pci.h >> +++ linux-pm/include/linux/pci.h >> @@ -344,6 +344,7 @@ struct pci_dev { >> D3cold, not set for devices >> powered on/off by the >> corresponding bridge */ >> + unsigned int skip_bus_pm:1; /* Internal: Skip bus-level PM */ >> unsigned int ignore_hotplug:1; /* Ignore hotplug events */ >> unsigned int hotplug_user_indicators:1; /* SlotCtl indicators >> controlled exclusively by > > Bjorn, I've assumed no concerns or objections from you regarding this one > and > queued it up. > > If that assumption is incorrect, please let me know. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] PCI: PM: Avoid possible suspend-to-idle issue 2019-06-11 8:39 ` Kai-Heng Feng @ 2019-06-11 9:03 ` Rafael J. Wysocki 2019-06-11 21:34 ` Rafael J. Wysocki 1 sibling, 0 replies; 8+ messages in thread From: Rafael J. Wysocki @ 2019-06-11 9:03 UTC (permalink / raw) To: Kai-Heng Feng Cc: Rafael J. Wysocki, Bjorn Helgaas, Linux PCI, Linux PM, Linux ACPI, LKML, Mika Westerberg, Keith Busch On Tue, Jun 11, 2019 at 10:39 AM Kai-Heng Feng <kai.heng.feng@canonical.com> wrote: > > Hi Rafael, > > at 19:02, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > > On Friday, May 17, 2019 11:08:50 AM CEST Rafael J. Wysocki wrote: > >> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > >> > >> If a PCI driver leaves the device handled by it in D0 and calls > >> pci_save_state() on the device in its ->suspend() or ->suspend_late() > >> callback, it can expect the device to stay in D0 over the whole > >> s2idle cycle. However, that may not be the case if there is a > >> spurious wakeup while the system is suspended, because in that case > >> pci_pm_suspend_noirq() will run again after pci_pm_resume_noirq() > >> which calls pci_restore_state(), via pci_pm_default_resume_early(), > >> so state_saved is cleared and the second iteration of > >> pci_pm_suspend_noirq() will invoke pci_prepare_to_sleep() which > >> may change the power state of the device. > >> > >> To avoid that, add a new internal flag, skip_bus_pm, that will be set > >> by pci_pm_suspend_noirq() when it runs for the first time during the > >> given system suspend-resume cycle if the state of the device has > >> been saved already and the device is still in D0. Setting that flag > >> will cause the next iterations of pci_pm_suspend_noirq() to set > >> state_saved for pci_pm_resume_noirq(), so that it always restores the > >> device state from the originally saved data, and avoid calling > >> pci_prepare_to_sleep() for the device. > >> > >> Fixes: 33e4f80ee69b ("ACPI / PM: Ignore spurious SCI wakeups from > >> suspend-to-idle") > >> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > I just found out this patch has a chance to freeze or reboot the system > during suspend cycles. > What information do you need to debug? It would be good to narrow down the failure to a particular transition, for example. In particular, if that happens during the dpm_noirq_resume_devices() called from s2idle_loop(), it may be necessary to also skip pci_pm_default_resume_early() for the devices with skip_bus_pm set. How many devices on the affected system end up with skip_bus_pm set, for that matter? ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] PCI: PM: Avoid possible suspend-to-idle issue 2019-06-11 8:39 ` Kai-Heng Feng 2019-06-11 9:03 ` Rafael J. Wysocki @ 2019-06-11 21:34 ` Rafael J. Wysocki 2019-06-11 21:37 ` Rafael J. Wysocki 1 sibling, 1 reply; 8+ messages in thread From: Rafael J. Wysocki @ 2019-06-11 21:34 UTC (permalink / raw) To: Kai-Heng Feng Cc: Bjorn Helgaas, Linux PCI, Linux PM, Linux ACPI, LKML, Mika Westerberg, Keith Busch On Tuesday, June 11, 2019 10:39:44 AM CEST Kai-Heng Feng wrote: > Hi Rafael, > > at 19:02, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > > On Friday, May 17, 2019 11:08:50 AM CEST Rafael J. Wysocki wrote: > >> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > >> > >> If a PCI driver leaves the device handled by it in D0 and calls > >> pci_save_state() on the device in its ->suspend() or ->suspend_late() > >> callback, it can expect the device to stay in D0 over the whole > >> s2idle cycle. However, that may not be the case if there is a > >> spurious wakeup while the system is suspended, because in that case > >> pci_pm_suspend_noirq() will run again after pci_pm_resume_noirq() > >> which calls pci_restore_state(), via pci_pm_default_resume_early(), > >> so state_saved is cleared and the second iteration of > >> pci_pm_suspend_noirq() will invoke pci_prepare_to_sleep() which > >> may change the power state of the device. > >> > >> To avoid that, add a new internal flag, skip_bus_pm, that will be set > >> by pci_pm_suspend_noirq() when it runs for the first time during the > >> given system suspend-resume cycle if the state of the device has > >> been saved already and the device is still in D0. Setting that flag > >> will cause the next iterations of pci_pm_suspend_noirq() to set > >> state_saved for pci_pm_resume_noirq(), so that it always restores the > >> device state from the originally saved data, and avoid calling > >> pci_prepare_to_sleep() for the device. > >> > >> Fixes: 33e4f80ee69b ("ACPI / PM: Ignore spurious SCI wakeups from > >> suspend-to-idle") > >> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > I just found out this patch has a chance to freeze or reboot the system > during suspend cycles. > > What information do you need to debug? A few things are missing from your report, like which kernel you have tested and how exactly you have arrived at the conclusion that this particular commit is the source of the problem. Care to provide some details on the above? Anyway, there are a couple of things that can be done to improve the code on top of 5.2-rc4. The appended patch is one of them, so can you please test it and let me know if it makes any difference? The rationale here is that firmware in some devices may be confused by attempts to put the device into D0 if it already is in that power state, so it is better to avoid doing so. --- drivers/pci/pci-driver.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) Index: linux-pm/drivers/pci/pci-driver.c =================================================================== --- linux-pm.orig/drivers/pci/pci-driver.c +++ linux-pm/drivers/pci/pci-driver.c @@ -524,7 +524,6 @@ static void pci_pm_default_resume_early( pci_power_up(pci_dev); pci_restore_state(pci_dev); pci_pme_restore(pci_dev); - pci_fixup_device(pci_fixup_resume_early, pci_dev); } /* @@ -844,14 +843,12 @@ static int pci_pm_suspend_noirq(struct d /* * The function is running for the second time in a row without * going through full resume, which is possible only during - * suspend-to-idle in a spurious wakeup case. Moreover, the - * device was originally left in D0, so its power state should - * not be changed here and the device register values saved - * originally should be restored on resume again. + * suspend-to-idle in a spurious wakeup case. The device should + * be in D0 at this point. */ - pci_dev->state_saved = true; + ; } else if (pci_dev->state_saved) { - if (pci_dev->current_state == PCI_D0) + if (pci_dev->current_state == PCI_D0 && !pm_suspend_via_firmware()) pci_dev->skip_bus_pm = true; } else { pci_save_state(pci_dev); @@ -862,6 +859,9 @@ static int pci_pm_suspend_noirq(struct d dev_dbg(dev, "PCI PM: Suspend power state: %s\n", pci_power_name(pci_dev->current_state)); + if (pci_dev->skip_bus_pm) + goto Fixup; + pci_pm_set_unknown_state(pci_dev); /* @@ -909,7 +909,10 @@ static int pci_pm_resume_noirq(struct de if (dev_pm_smart_suspend_and_suspended(dev)) pm_runtime_set_active(dev); - pci_pm_default_resume_early(pci_dev); + if (!pci_dev->skip_bus_pm) + pci_pm_default_resume_early(pci_dev); + + pci_fixup_device(pci_fixup_resume_early, pci_dev); if (pci_has_legacy_pm_support(pci_dev)) return pci_legacy_resume_early(dev); @@ -1200,6 +1203,7 @@ static int pci_pm_restore_noirq(struct d } pci_pm_default_resume_early(pci_dev); + pci_fixup_device(pci_fixup_resume_early, pci_dev); if (pci_has_legacy_pm_support(pci_dev)) return pci_legacy_resume_early(dev); ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] PCI: PM: Avoid possible suspend-to-idle issue 2019-06-11 21:34 ` Rafael J. Wysocki @ 2019-06-11 21:37 ` Rafael J. Wysocki 0 siblings, 0 replies; 8+ messages in thread From: Rafael J. Wysocki @ 2019-06-11 21:37 UTC (permalink / raw) To: Kai-Heng Feng Cc: Bjorn Helgaas, Linux PCI, Linux PM, Linux ACPI, LKML, Mika Westerberg, Keith Busch On Tuesday, June 11, 2019 11:34:36 PM CEST Rafael J. Wysocki wrote: > On Tuesday, June 11, 2019 10:39:44 AM CEST Kai-Heng Feng wrote: > > Hi Rafael, > > > > at 19:02, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > > > > On Friday, May 17, 2019 11:08:50 AM CEST Rafael J. Wysocki wrote: > > >> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > >> > > >> If a PCI driver leaves the device handled by it in D0 and calls > > >> pci_save_state() on the device in its ->suspend() or ->suspend_late() > > >> callback, it can expect the device to stay in D0 over the whole > > >> s2idle cycle. However, that may not be the case if there is a > > >> spurious wakeup while the system is suspended, because in that case > > >> pci_pm_suspend_noirq() will run again after pci_pm_resume_noirq() > > >> which calls pci_restore_state(), via pci_pm_default_resume_early(), > > >> so state_saved is cleared and the second iteration of > > >> pci_pm_suspend_noirq() will invoke pci_prepare_to_sleep() which > > >> may change the power state of the device. > > >> > > >> To avoid that, add a new internal flag, skip_bus_pm, that will be set > > >> by pci_pm_suspend_noirq() when it runs for the first time during the > > >> given system suspend-resume cycle if the state of the device has > > >> been saved already and the device is still in D0. Setting that flag > > >> will cause the next iterations of pci_pm_suspend_noirq() to set > > >> state_saved for pci_pm_resume_noirq(), so that it always restores the > > >> device state from the originally saved data, and avoid calling > > >> pci_prepare_to_sleep() for the device. > > >> > > >> Fixes: 33e4f80ee69b ("ACPI / PM: Ignore spurious SCI wakeups from > > >> suspend-to-idle") > > >> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > I just found out this patch has a chance to freeze or reboot the system > > during suspend cycles. > > > > What information do you need to debug? > > A few things are missing from your report, like which kernel you have tested > and how exactly you have arrived at the conclusion that this particular commit > is the source of the problem. > > Care to provide some details on the above? > > Anyway, there are a couple of things that can be done to improve the code > on top of 5.2-rc4. The appended patch is one of them, so can you please test > it and let me know if it makes any difference? > > The rationale here is that firmware in some devices may be confused by attempts > to put the device into D0 if it already is in that power state, so it is better to avoid > doing so. > > --- > drivers/pci/pci-driver.c | 20 ++++++++++++-------- > 1 file changed, 12 insertions(+), 8 deletions(-) > > Index: linux-pm/drivers/pci/pci-driver.c > =================================================================== > --- linux-pm.orig/drivers/pci/pci-driver.c > +++ linux-pm/drivers/pci/pci-driver.c > @@ -524,7 +524,6 @@ static void pci_pm_default_resume_early( > pci_power_up(pci_dev); > pci_restore_state(pci_dev); > pci_pme_restore(pci_dev); > - pci_fixup_device(pci_fixup_resume_early, pci_dev); > } > > /* > @@ -844,14 +843,12 @@ static int pci_pm_suspend_noirq(struct d > /* > * The function is running for the second time in a row without > * going through full resume, which is possible only during > - * suspend-to-idle in a spurious wakeup case. Moreover, the > - * device was originally left in D0, so its power state should > - * not be changed here and the device register values saved > - * originally should be restored on resume again. > + * suspend-to-idle in a spurious wakeup case. The device should > + * be in D0 at this point. > */ > - pci_dev->state_saved = true; > + ; > } else if (pci_dev->state_saved) { > - if (pci_dev->current_state == PCI_D0) > + if (pci_dev->current_state == PCI_D0 && !pm_suspend_via_firmware()) > pci_dev->skip_bus_pm = true; > } else { > pci_save_state(pci_dev); > @@ -862,6 +859,9 @@ static int pci_pm_suspend_noirq(struct d > dev_dbg(dev, "PCI PM: Suspend power state: %s\n", > pci_power_name(pci_dev->current_state)); > > + if (pci_dev->skip_bus_pm) > + goto Fixup; > + > pci_pm_set_unknown_state(pci_dev); > > /* > @@ -909,7 +909,10 @@ static int pci_pm_resume_noirq(struct de > if (dev_pm_smart_suspend_and_suspended(dev)) > pm_runtime_set_active(dev); > > - pci_pm_default_resume_early(pci_dev); > + if (!pci_dev->skip_bus_pm) > + pci_pm_default_resume_early(pci_dev); > + > + pci_fixup_device(pci_fixup_resume_early, pci_dev); > > if (pci_has_legacy_pm_support(pci_dev)) > return pci_legacy_resume_early(dev); > @@ -1200,6 +1203,7 @@ static int pci_pm_restore_noirq(struct d > } > > pci_pm_default_resume_early(pci_dev); > + pci_fixup_device(pci_fixup_resume_early, pci_dev); > > if (pci_has_legacy_pm_support(pci_dev)) > return pci_legacy_resume_early(dev); > And on top of the patch above, it may be a good idea to leave bridges above the devices left in D0 alone, which is taken care of by the appended patch. Please test it (on top of the above one) and let me know how that goes. --- drivers/pci/pci-driver.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) Index: linux-pm/drivers/pci/pci-driver.c =================================================================== --- linux-pm.orig/drivers/pci/pci-driver.c +++ linux-pm/drivers/pci/pci-driver.c @@ -841,15 +841,24 @@ static int pci_pm_suspend_noirq(struct d if (pci_dev->skip_bus_pm) { /* - * The function is running for the second time in a row without + * Either the device is a bridge with a child in D0 below it, or + * the function is running for the second time in a row without * going through full resume, which is possible only during * suspend-to-idle in a spurious wakeup case. The device should * be in D0 at this point. */ ; } else if (pci_dev->state_saved) { - if (pci_dev->current_state == PCI_D0 && !pm_suspend_via_firmware()) + if (pci_dev->current_state == PCI_D0 && !pm_suspend_via_firmware()) { + struct pci_dev *bridge = pci_dev->bus->self; + pci_dev->skip_bus_pm = true; + + while (bridge) { + bridge->skip_bus_pm = true; + bridge = bridge->bus->self; + } + } } else { pci_save_state(pci_dev); if (pci_power_manageable(pci_dev)) ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-06-11 21:37 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-05-17 9:08 [PATCH] PCI: PM: Avoid possible suspend-to-idle issue Rafael J. Wysocki 2019-05-17 15:57 ` Keith Busch 2019-05-20 10:39 ` Mika Westerberg 2019-05-27 11:02 ` Rafael J. Wysocki 2019-06-11 8:39 ` Kai-Heng Feng 2019-06-11 9:03 ` Rafael J. Wysocki 2019-06-11 21:34 ` Rafael J. Wysocki 2019-06-11 21:37 ` Rafael J. Wysocki
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).