diff mbox series

[v2,1/5] PM: ACPI/PCI: Resume all devices during hibernation

Message ID 6191578.xJk2HsE5MX@kreacher (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show
Series PM: PCI/ACPI: Hibernation handling fixes | expand

Commit Message

Rafael J. Wysocki July 1, 2019, 10:44 a.m. UTC
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Both the PCI bus type and the ACPI PM domain avoid resuming
runtime-suspended devices with DPM_FLAG_SMART_SUSPEND set during
hibernation (before creating the snapshot image of system memory),
but that turns out to be a mistake.  It leads to functional issues
and adds complexity that's hard to justify.

For this reason, resume all runtime-suspended PCI devices and all
devices in the ACPI PM domains before creating a snapshot image of
system memory during hibernation.

Fixes: 05087360fd7a (ACPI / PM: Take SMART_SUSPEND driver flag into account)
Fixes: c4b65157aeef (PCI / PM: Take SMART_SUSPEND driver flag into account)
Link: https://lore.kernel.org/linux-acpi/917d4399-2e22-67b1-9d54-808561f9083f@uwyo.edu/T/#maf065fe6e4974f2a9d79f332ab99dfaba635f64c
Reported-by: Robert R. Howell <RHowell@uwyo.edu>
Tested-by: Robert R. Howell <RHowell@uwyo.edu>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

-> v2: No changes.

---
 drivers/acpi/device_pm.c |   13 +++++++------
 drivers/pci/pci-driver.c |   16 ++++++++--------
 2 files changed, 15 insertions(+), 14 deletions(-)

Comments

Mika Westerberg July 1, 2019, 4:15 p.m. UTC | #1
On Mon, Jul 01, 2019 at 12:44:25PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Both the PCI bus type and the ACPI PM domain avoid resuming
> runtime-suspended devices with DPM_FLAG_SMART_SUSPEND set during
> hibernation (before creating the snapshot image of system memory),
> but that turns out to be a mistake.  It leads to functional issues
> and adds complexity that's hard to justify.
> 
> For this reason, resume all runtime-suspended PCI devices and all
> devices in the ACPI PM domains before creating a snapshot image of
> system memory during hibernation.
> 
> Fixes: 05087360fd7a (ACPI / PM: Take SMART_SUSPEND driver flag into account)
> Fixes: c4b65157aeef (PCI / PM: Take SMART_SUSPEND driver flag into account)
> Link: https://lore.kernel.org/linux-acpi/917d4399-2e22-67b1-9d54-808561f9083f@uwyo.edu/T/#maf065fe6e4974f2a9d79f332ab99dfaba635f64c
> Reported-by: Robert R. Howell <RHowell@uwyo.edu>
> Tested-by: Robert R. Howell <RHowell@uwyo.edu>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> -> v2: No changes.
> 
> ---
>  drivers/acpi/device_pm.c |   13 +++++++------
>  drivers/pci/pci-driver.c |   16 ++++++++--------
>  2 files changed, 15 insertions(+), 14 deletions(-)
> 
> Index: linux-pm/drivers/acpi/device_pm.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/device_pm.c
> +++ linux-pm/drivers/acpi/device_pm.c
> @@ -1155,13 +1155,14 @@ EXPORT_SYMBOL_GPL(acpi_subsys_resume_ear
>  int acpi_subsys_freeze(struct device *dev)
>  {
>  	/*
> -	 * This used to be done in acpi_subsys_prepare() for all devices and
> -	 * some drivers may depend on it, so do it here.  Ideally, however,
> -	 * runtime-suspended devices should not be touched during freeze/thaw
> -	 * transitions.
> +	 * Resume all runtime-suspended devices before creating a snapshot
> +	 * image of system memory, because the restore kernel generally cannot
> +	 * be expected to always handle them consistently and they need to be
> +	 * put into the runtime-active metastate during system resume anyway,
> +	 * so it is better to ensure that the state saved in the image will be
> +	 * alwyas consistent with that.

alwyas -> always

>  	 */
> -	if (!dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND))
> -		pm_runtime_resume(dev);
> +	pm_runtime_resume(dev);
>  
>  	return pm_generic_freeze(dev);
>  }
> Index: linux-pm/drivers/pci/pci-driver.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/pci-driver.c
> +++ linux-pm/drivers/pci/pci-driver.c
> @@ -1012,15 +1012,15 @@ static int pci_pm_freeze(struct device *
>  	}
>  
>  	/*
> -	 * This used to be done in pci_pm_prepare() for all devices and some
> -	 * drivers may depend on it, so do it here.  Ideally, runtime-suspended
> -	 * devices should not be touched during freeze/thaw transitions,
> -	 * however.
> +	 * Resume all runtime-suspended devices before creating a snapshot
> +	 * image of system memory, because the restore kernel generally cannot
> +	 * be expected to always handle them consistently and they need to be
> +	 * put into the runtime-active metastate during system resume anyway,
> +	 * so it is better to ensure that the state saved in the image will be
> +	 * alwyas consistent with that.

ditto

>  	 */
> -	if (!dev_pm_smart_suspend_and_suspended(dev)) {
> -		pm_runtime_resume(dev);
> -		pci_dev->state_saved = false;
> -	}
> +	pm_runtime_resume(dev);
> +	pci_dev->state_saved = false;
>  
>  	if (pm->freeze) {
>  		int error;
> 
> 
>
Robert R. Howell July 2, 2019, 4:24 a.m. UTC | #2
On 7/1/19 4:44 AM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Both the PCI bus type and the ACPI PM domain avoid resuming
> runtime-suspended devices with DPM_FLAG_SMART_SUSPEND set during
> hibernation (before creating the snapshot image of system memory),
> but that turns out to be a mistake.  It leads to functional issues
> and adds complexity that's hard to justify.
> 
> For this reason, resume all runtime-suspended PCI devices and all
> devices in the ACPI PM domains before creating a snapshot image of
> system memory during hibernation.
> 
> Fixes: 05087360fd7a (ACPI / PM: Take SMART_SUSPEND driver flag into account)
> Fixes: c4b65157aeef (PCI / PM: Take SMART_SUSPEND driver flag into account)
> Link: https://lore.kernel.org/linux-acpi/917d4399-2e22-67b1-9d54-808561f9083f@uwyo.edu/T/#maf065fe6e4974f2a9d79f332ab99dfaba635f64c
> Reported-by: Robert R. Howell <RHowell@uwyo.edu>
> Tested-by: Robert R. Howell <RHowell@uwyo.edu>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> -> v2: No changes.

I've tested your v2 patch set on my ASUS T100TA, applying just those 5 patches to the 
5.2-rc7 kernel, and they do fix the hibernation problem.  The i2c_designware 
timeout errors are now gone and sound does now work after resume from both 
suspend and hibernate.  

As before I still see the "i2c_designware 80860F41:00: Transfer while suspended" error 
on the first resume from either suspend or hibernate, but also as before that 
particular error doesn't seem to cause a persistent problem and the system works 
normally after the resume.

Bob Howell
Rafael J. Wysocki July 2, 2019, 3:59 p.m. UTC | #3
On Mon, Jul 1, 2019 at 6:15 PM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> On Mon, Jul 01, 2019 at 12:44:25PM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Both the PCI bus type and the ACPI PM domain avoid resuming
> > runtime-suspended devices with DPM_FLAG_SMART_SUSPEND set during
> > hibernation (before creating the snapshot image of system memory),
> > but that turns out to be a mistake.  It leads to functional issues
> > and adds complexity that's hard to justify.
> >
> > For this reason, resume all runtime-suspended PCI devices and all
> > devices in the ACPI PM domains before creating a snapshot image of
> > system memory during hibernation.
> >
> > Fixes: 05087360fd7a (ACPI / PM: Take SMART_SUSPEND driver flag into account)
> > Fixes: c4b65157aeef (PCI / PM: Take SMART_SUSPEND driver flag into account)
> > Link: https://lore.kernel.org/linux-acpi/917d4399-2e22-67b1-9d54-808561f9083f@uwyo.edu/T/#maf065fe6e4974f2a9d79f332ab99dfaba635f64c
> > Reported-by: Robert R. Howell <RHowell@uwyo.edu>
> > Tested-by: Robert R. Howell <RHowell@uwyo.edu>
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >
> > -> v2: No changes.
> >
> > ---
> >  drivers/acpi/device_pm.c |   13 +++++++------
> >  drivers/pci/pci-driver.c |   16 ++++++++--------
> >  2 files changed, 15 insertions(+), 14 deletions(-)
> >
> > Index: linux-pm/drivers/acpi/device_pm.c
> > ===================================================================
> > --- linux-pm.orig/drivers/acpi/device_pm.c
> > +++ linux-pm/drivers/acpi/device_pm.c
> > @@ -1155,13 +1155,14 @@ EXPORT_SYMBOL_GPL(acpi_subsys_resume_ear
> >  int acpi_subsys_freeze(struct device *dev)
> >  {
> >       /*
> > -      * This used to be done in acpi_subsys_prepare() for all devices and
> > -      * some drivers may depend on it, so do it here.  Ideally, however,
> > -      * runtime-suspended devices should not be touched during freeze/thaw
> > -      * transitions.
> > +      * Resume all runtime-suspended devices before creating a snapshot
> > +      * image of system memory, because the restore kernel generally cannot
> > +      * be expected to always handle them consistently and they need to be
> > +      * put into the runtime-active metastate during system resume anyway,
> > +      * so it is better to ensure that the state saved in the image will be
> > +      * alwyas consistent with that.
>
> alwyas -> always
>
> >        */
> > -     if (!dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND))
> > -             pm_runtime_resume(dev);
> > +     pm_runtime_resume(dev);
> >
> >       return pm_generic_freeze(dev);
> >  }
> > Index: linux-pm/drivers/pci/pci-driver.c
> > ===================================================================
> > --- linux-pm.orig/drivers/pci/pci-driver.c
> > +++ linux-pm/drivers/pci/pci-driver.c
> > @@ -1012,15 +1012,15 @@ static int pci_pm_freeze(struct device *
> >       }
> >
> >       /*
> > -      * This used to be done in pci_pm_prepare() for all devices and some
> > -      * drivers may depend on it, so do it here.  Ideally, runtime-suspended
> > -      * devices should not be touched during freeze/thaw transitions,
> > -      * however.
> > +      * Resume all runtime-suspended devices before creating a snapshot
> > +      * image of system memory, because the restore kernel generally cannot
> > +      * be expected to always handle them consistently and they need to be
> > +      * put into the runtime-active metastate during system resume anyway,
> > +      * so it is better to ensure that the state saved in the image will be
> > +      * alwyas consistent with that.
>
> ditto

Thanks, I'll fix these up when applying the patch.
diff mbox series

Patch

Index: linux-pm/drivers/acpi/device_pm.c
===================================================================
--- linux-pm.orig/drivers/acpi/device_pm.c
+++ linux-pm/drivers/acpi/device_pm.c
@@ -1155,13 +1155,14 @@  EXPORT_SYMBOL_GPL(acpi_subsys_resume_ear
 int acpi_subsys_freeze(struct device *dev)
 {
 	/*
-	 * This used to be done in acpi_subsys_prepare() for all devices and
-	 * some drivers may depend on it, so do it here.  Ideally, however,
-	 * runtime-suspended devices should not be touched during freeze/thaw
-	 * transitions.
+	 * Resume all runtime-suspended devices before creating a snapshot
+	 * image of system memory, because the restore kernel generally cannot
+	 * be expected to always handle them consistently and they need to be
+	 * put into the runtime-active metastate during system resume anyway,
+	 * so it is better to ensure that the state saved in the image will be
+	 * alwyas consistent with that.
 	 */
-	if (!dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND))
-		pm_runtime_resume(dev);
+	pm_runtime_resume(dev);
 
 	return pm_generic_freeze(dev);
 }
Index: linux-pm/drivers/pci/pci-driver.c
===================================================================
--- linux-pm.orig/drivers/pci/pci-driver.c
+++ linux-pm/drivers/pci/pci-driver.c
@@ -1012,15 +1012,15 @@  static int pci_pm_freeze(struct device *
 	}
 
 	/*
-	 * This used to be done in pci_pm_prepare() for all devices and some
-	 * drivers may depend on it, so do it here.  Ideally, runtime-suspended
-	 * devices should not be touched during freeze/thaw transitions,
-	 * however.
+	 * Resume all runtime-suspended devices before creating a snapshot
+	 * image of system memory, because the restore kernel generally cannot
+	 * be expected to always handle them consistently and they need to be
+	 * put into the runtime-active metastate during system resume anyway,
+	 * so it is better to ensure that the state saved in the image will be
+	 * alwyas consistent with that.
 	 */
-	if (!dev_pm_smart_suspend_and_suspended(dev)) {
-		pm_runtime_resume(dev);
-		pci_dev->state_saved = false;
-	}
+	pm_runtime_resume(dev);
+	pci_dev->state_saved = false;
 
 	if (pm->freeze) {
 		int error;