Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1235653 > unrolled thread

[RFC][PATCH 2/2] PM / sleep: Kick devices that might have been reset by firmware

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [RFC][PATCH 2/2] PM / sleep: Kick devices that might have been reset by firmware "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-09-30 02:30 +0200
    Re: [RFC][PATCH 2/2] PM / sleep: Kick devices that might have been  reset by firmware kbuild test robot <lkp@intel.com> - 2015-09-30 02:40 +0200
    Re: [RFC][PATCH 2/2] PM / sleep: Kick devices that might have been  reset by firmware Alan Stern <stern@rowland.harvard.edu> - 2015-09-30 16:50 +0200
      Re: [RFC][PATCH 2/2] PM / sleep: Kick devices that might have been reset by firmware "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-09-30 23:30 +0200
        Re: [RFC][PATCH 2/2] PM / sleep: Kick devices that might have been  reset by firmware Alan Stern <stern@rowland.harvard.edu> - 2015-10-01 16:50 +0200
          Re: [RFC][PATCH 2/2] PM / sleep: Kick devices that might have been reset by firmware "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-10-01 23:50 +0200

#1235653 — [RFC][PATCH 2/2] PM / sleep: Kick devices that might have been reset by firmware

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

If the platform firmware was involved in the system resume that's
being completed, there is a concern that some devices might have
been reset by it and if those devices had the power.direct_complete
flag set during the preceding suspend transition, they may stay
in a reset-power-on state indefinitely (until they are runtime-resumed
and then suspended again).  That may not be a big deal from the
individual device's perspective, but if the system is an SoC, it may
be prevented from entering deep SoC-wide low-power states on idle
because of that.

To prevent that from happening, force a runtime resume for devices
with power.direct_complete set if the platform firmware was involved
in the resume transition currently in progress.

Something similar was done by the ACPI PM domain, but regardless of
the platform firmware involvement, and the new mechanism should be
sufficient to replace that code, so drop it.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/device_pm.c  |    7 -------
 drivers/base/power/main.c |    7 +++++++
 2 files changed, 7 insertions(+), 7 deletions(-)

Index: linux-pm/drivers/acpi/device_pm.c
===================================================================
--- linux-pm.orig/drivers/acpi/device_pm.c
+++ linux-pm/drivers/acpi/device_pm.c
@@ -969,13 +969,6 @@ EXPORT_SYMBOL_GPL(acpi_subsys_prepare);
 void acpi_subsys_complete(struct device *dev)
 {
 	pm_generic_complete(dev);
-	/*
-	 * If the device had been runtime-suspended before the system went into
-	 * the sleep state it is going out of and it has never been resumed till
-	 * now, resume it in case the firmware powered it up.
-	 */
-	if (dev->power.direct_complete)
-		pm_request_resume(dev);
 }
 EXPORT_SYMBOL_GPL(acpi_subsys_complete);
 
Index: linux-pm/drivers/base/power/main.c
===================================================================
--- linux-pm.orig/drivers/base/power/main.c
+++ linux-pm/drivers/base/power/main.c
@@ -928,6 +928,13 @@ static void device_complete(struct devic
 	device_unlock(dev);
 
 	pm_runtime_put(dev);
+	/*
+	 * If the device had been runtime-suspended before the system went into
+	 * the sleep state it is going out of and it has never been resumed till
+	 * now, resume it in case the firmware powered it up.
+	 */
+	if (dev->power.direct_complete && pm_resume_via_firmware())
+		pm_request_resume(dev);
 }
 
 /**

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1235660 — Re: [RFC][PATCH 2/2] PM / sleep: Kick devices that might have been reset by firmware

[Multipart message — attachments visible in raw view] — view raw

Hi Rafael,

[auto build test results on v4.3-rc3 -- if it's inappropriate base, please ignore]

config: i386-randconfig-a0-201539 (attached as .config)
reproduce:
  git checkout 270cc782e8ef117ccad348c9a8330501dcb55f95
  # save the attached .config to linux build tree
  make ARCH=i386 

All error/warnings (new ones prefixed by >>):

   drivers/base/power/main.c: In function 'device_complete':
>> drivers/base/power/main.c:936:36: error: implicit declaration of function 'pm_resume_via_firmware' [-Werror=implicit-function-declaration]
     if (dev->power.direct_complete && pm_resume_via_firmware())
                                       ^
   cc1: some warnings being treated as errors

vim +/pm_resume_via_firmware +936 drivers/base/power/main.c

   930		pm_runtime_put(dev);
   931		/*
   932		 * If the device had been runtime-suspended before the system went into
   933		 * the sleep state it is going out of and it has never been resumed till
   934		 * now, resume it in case the firmware powered it up.
   935		 */
 > 936		if (dev->power.direct_complete && pm_resume_via_firmware())
   937			pm_request_resume(dev);
   938	}
   939	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[toc] | [prev] | [next] | [standalone]

#1236407 — Re: [RFC][PATCH 2/2] PM / sleep: Kick devices that might have been reset by firmware

On Wed, 30 Sep 2015, Rafael J. Wysocki wrote:

> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> If the platform firmware was involved in the system resume that's
> being completed, there is a concern that some devices might have
> been reset by it and if those devices had the power.direct_complete
> flag set during the preceding suspend transition, they may stay
> in a reset-power-on state indefinitely (until they are runtime-resumed
> and then suspended again).  That may not be a big deal from the
> individual device's perspective, but if the system is an SoC, it may
> be prevented from entering deep SoC-wide low-power states on idle
> because of that.
> 
> To prevent that from happening, force a runtime resume for devices
> with power.direct_complete set if the platform firmware was involved
> in the resume transition currently in progress.
> 
> Something similar was done by the ACPI PM domain, but regardless of
> the platform firmware involvement, and the new mechanism should be
> sufficient to replace that code, so drop it.

Maybe I'm not reading patch 1/2 correctly, but it looks like an
ordinary ACPI-based desktop PC will always believe the firmware was
involved in an S3 sleep transition.  If that's so then won't this
change defeat all the work being done by people trying to prevent
unneeded runtime resumes during system resume?  direct_complete would 
be useful only on non-ACPI systems.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1236773

On Wednesday, September 30, 2015 10:46:03 AM Alan Stern wrote:
> On Wed, 30 Sep 2015, Rafael J. Wysocki wrote:
> 
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > If the platform firmware was involved in the system resume that's
> > being completed, there is a concern that some devices might have
> > been reset by it and if those devices had the power.direct_complete
> > flag set during the preceding suspend transition, they may stay
> > in a reset-power-on state indefinitely (until they are runtime-resumed
> > and then suspended again).  That may not be a big deal from the
> > individual device's perspective, but if the system is an SoC, it may
> > be prevented from entering deep SoC-wide low-power states on idle
> > because of that.
> > 
> > To prevent that from happening, force a runtime resume for devices
> > with power.direct_complete set if the platform firmware was involved
> > in the resume transition currently in progress.
> > 
> > Something similar was done by the ACPI PM domain, but regardless of
> > the platform firmware involvement, and the new mechanism should be
> > sufficient to replace that code, so drop it.
> 
> Maybe I'm not reading patch 1/2 correctly, but it looks like an
> ordinary ACPI-based desktop PC will always believe the firmware was
> involved in an S3 sleep transition.

That's correct and it reflects the reality.

> If that's so then won't this change defeat all the work being done by
> people trying to prevent unneeded runtime resumes during system resume?
> direct_complete would be useful only on non-ACPI systems.

To me at least the main motivation for direct_complete was to avoid unneeded
runtime resumes during system suspend and this patch doesn't change that.

Moreover, there is a difference between scheduling an asynchronous runtime
resume during system resume and doing a synchrouous runtime resume at that
point.  In the latter case the resume process has to wait for the runtime
resume to complete, while in the former case we can get to thawing user
space while the scheduled runtime resume is in progress (or even still
waiting for that matter).

This means that direct_complete would be useful even for S3 transitions
with this patch applied.  Not to mention the suspend-to-idle case in which
the resume really doesn't go through the firmware.

That said, perhaps the check as proposed is too coarse-grained.  We need to
do something like this in the ACPI PM domain code (and we do it already) and
for PCI devices that are likely to be affected by the issue at hand.  So
what about the appended patch (on top of https://patchwork.kernel.org/patch/7291711/)
instead?

Rafael


---
 drivers/acpi/acpi_lpss.c         |    2 +-
 drivers/acpi/device_pm.c         |   19 +------------------
 drivers/base/power/generic_ops.c |   23 +++++++++++++++++++++++
 drivers/pci/pci-driver.c         |    2 +-
 include/linux/pm.h               |    2 +-
 5 files changed, 27 insertions(+), 21 deletions(-)

Index: linux-pm/drivers/acpi/device_pm.c
===================================================================
--- linux-pm.orig/drivers/acpi/device_pm.c
+++ linux-pm/drivers/acpi/device_pm.c
@@ -963,23 +963,6 @@ int acpi_subsys_prepare(struct device *d
 EXPORT_SYMBOL_GPL(acpi_subsys_prepare);
 
 /**
- * acpi_subsys_complete - Finalize device's resume during system resume.
- * @dev: Device to handle.
- */
-void acpi_subsys_complete(struct device *dev)
-{
-	pm_generic_complete(dev);
-	/*
-	 * If the device had been runtime-suspended before the system went into
-	 * the sleep state it is going out of and it has never been resumed till
-	 * now, resume it in case the firmware powered it up.
-	 */
-	if (dev->power.direct_complete)
-		pm_request_resume(dev);
-}
-EXPORT_SYMBOL_GPL(acpi_subsys_complete);
-
-/**
  * acpi_subsys_suspend - Run the device driver's suspend callback.
  * @dev: Device to handle.
  *
@@ -1047,7 +1030,7 @@ static struct dev_pm_domain acpi_general
 		.runtime_resume = acpi_subsys_runtime_resume,
 #ifdef CONFIG_PM_SLEEP
 		.prepare = acpi_subsys_prepare,
-		.complete = acpi_subsys_complete,
+		.complete = pm_complete_with_resume_check,
 		.suspend = acpi_subsys_suspend,
 		.suspend_late = acpi_subsys_suspend_late,
 		.resume_early = acpi_subsys_resume_early,
Index: linux-pm/drivers/acpi/acpi_lpss.c
===================================================================
--- linux-pm.orig/drivers/acpi/acpi_lpss.c
+++ linux-pm/drivers/acpi/acpi_lpss.c
@@ -664,7 +664,7 @@ static struct dev_pm_domain acpi_lpss_pm
 #ifdef CONFIG_PM
 #ifdef CONFIG_PM_SLEEP
 		.prepare = acpi_subsys_prepare,
-		.complete = acpi_subsys_complete,
+		.complete = pm_complete_with_resume_check,
 		.suspend = acpi_subsys_suspend,
 		.suspend_late = acpi_lpss_suspend_late,
 		.resume_early = acpi_lpss_resume_early,
Index: linux-pm/drivers/base/power/generic_ops.c
===================================================================
--- linux-pm.orig/drivers/base/power/generic_ops.c
+++ linux-pm/drivers/base/power/generic_ops.c
@@ -9,6 +9,7 @@
 #include <linux/pm.h>
 #include <linux/pm_runtime.h>
 #include <linux/export.h>
+#include <linux/suspend.h>
 
 #ifdef CONFIG_PM
 /**
@@ -297,4 +298,26 @@ void pm_generic_complete(struct device *
 	if (drv && drv->pm && drv->pm->complete)
 		drv->pm->complete(dev);
 }
+
+/**
+ * pm_complete_with_resume_check - Complete a device power transition.
+ * @dev: Device to handle.
+ *
+ * Complete a device power transition during a system-wide power transition and
+ * optionally schedule a runtime resume of the device if the system resume in
+ * progress has been initated by the platform firmware and the device had its
+ * power.direct_complete flag set.
+ */
+void pm_complete_with_resume_check(struct device *dev)
+{
+	pm_generic_complete(dev);
+	/*
+	 * If the device had been runtime-suspended before the system went into
+	 * the sleep state it is going out of and it has never been resumed till
+	 * now, resume it in case the firmware powered it up.
+	 */
+	if (dev->power.direct_complete && pm_resume_via_firmware())
+		pm_request_resume(dev);
+}
+EXPORT_SYMBOL_GPL(pm_complete_with_resume_check);
 #endif /* CONFIG_PM_SLEEP */
Index: linux-pm/drivers/pci/pci-driver.c
===================================================================
--- linux-pm.orig/drivers/pci/pci-driver.c
+++ linux-pm/drivers/pci/pci-driver.c
@@ -687,7 +687,7 @@ static int pci_pm_prepare(struct device
 static void pci_pm_complete(struct device *dev)
 {
 	pci_dev_complete_resume(to_pci_dev(dev));
-	pm_generic_complete(dev);
+	pm_complete_with_resume_check(dev);
 }
 
 #else /* !CONFIG_PM_SLEEP */
Index: linux-pm/include/linux/pm.h
===================================================================
--- linux-pm.orig/include/linux/pm.h
+++ linux-pm/include/linux/pm.h
@@ -731,7 +731,7 @@ extern int pm_generic_restore(struct dev
 extern int pm_generic_poweroff_noirq(struct device *dev);
 extern int pm_generic_poweroff_late(struct device *dev);
 extern int pm_generic_poweroff(struct device *dev);
-extern void pm_generic_complete(struct device *dev);
+extern void pm_complete_with_resume_check(struct device *dev);
 
 #else /* !CONFIG_PM_SLEEP */
 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1237464 — Re: [RFC][PATCH 2/2] PM / sleep: Kick devices that might have been reset by firmware

On Wed, 30 Sep 2015, Rafael J. Wysocki wrote:

> > If that's so then won't this change defeat all the work being done by
> > people trying to prevent unneeded runtime resumes during system resume?
> > direct_complete would be useful only on non-ACPI systems.
> 
> To me at least the main motivation for direct_complete was to avoid unneeded
> runtime resumes during system suspend and this patch doesn't change that.

I always thought that at least part of the motivation was to allow 
devices to remain in runtime suspend throughout an entire system sleep 
transition: The device starts out in runtime suspend before the 
system goes to sleep and it is still in runtime suspend after the 
system wakes up.

> Moreover, there is a difference between scheduling an asynchronous runtime
> resume during system resume and doing a synchrouous runtime resume at that
> point.  In the latter case the resume process has to wait for the runtime
> resume to complete, while in the former case we can get to thawing user
> space while the scheduled runtime resume is in progress (or even still
> waiting for that matter).

True.  However, in practice what generally happened (before you
introduced direct_complete) is that the device would get set back to
full power during the system resume, just as though it had not been in
runtime suspend before the system went to sleep.

If the device uses async suspend/resume then this is not quite as bad
as doing a synchronous runtime resume.  But as you say, it's still not
as good as doing an async runtime resume, so I guess the effects of
your patch aren't quite as bad as I thought at first.

> This means that direct_complete would be useful even for S3 transitions
> with this patch applied.  Not to mention the suspend-to-idle case in which
> the resume really doesn't go through the firmware.

Certainly.

> That said, perhaps the check as proposed is too coarse-grained.  We need to
> do something like this in the ACPI PM domain code (and we do it already) and
> for PCI devices that are likely to be affected by the issue at hand.  So
> what about the appended patch (on top of https://patchwork.kernel.org/patch/7291711/)
> instead?

It seems reasonable.  If it turns out that more drivers need to check 
for firmware interference, we can add them in later on.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1237765

On Thursday, October 01, 2015 10:47:26 AM Alan Stern wrote:
> On Wed, 30 Sep 2015, Rafael J. Wysocki wrote:
> 
> > > If that's so then won't this change defeat all the work being done by
> > > people trying to prevent unneeded runtime resumes during system resume?
> > > direct_complete would be useful only on non-ACPI systems.
> > 
> > To me at least the main motivation for direct_complete was to avoid unneeded
> > runtime resumes during system suspend and this patch doesn't change that.
> 
> I always thought that at least part of the motivation was to allow 
> devices to remain in runtime suspend throughout an entire system sleep 
> transition: The device starts out in runtime suspend before the 
> system goes to sleep and it is still in runtime suspend after the 
> system wakes up.

Well, that was part of it, but the real issue was that we were resuming
all PCI devices unconditionally during system suspend.  When we tried to
address this, we noticed that the suspended device actually need not be
resumed at all throughout the whole cycle.

> > Moreover, there is a difference between scheduling an asynchronous runtime
> > resume during system resume and doing a synchrouous runtime resume at that
> > point.  In the latter case the resume process has to wait for the runtime
> > resume to complete, while in the former case we can get to thawing user
> > space while the scheduled runtime resume is in progress (or even still
> > waiting for that matter).
> 
> True.  However, in practice what generally happened (before you
> introduced direct_complete) is that the device would get set back to
> full power during the system resume, just as though it had not been in
> runtime suspend before the system went to sleep.

Right.

> If the device uses async suspend/resume then this is not quite as bad
> as doing a synchronous runtime resume.  But as you say, it's still not
> as good as doing an async runtime resume, so I guess the effects of
> your patch aren't quite as bad as I thought at first.

Plus even if the device uses async suspend/resume, but it happens to be the
slowest device in at least one stage of resume (eg. the "resume" stage), then
the whole resume process needs to wait for it anyway, whereas it doesn't
need to wait at all if an async runtime resume is scheduled at the end of the
"complete" stage.

> > This means that direct_complete would be useful even for S3 transitions
> > with this patch applied.  Not to mention the suspend-to-idle case in which
> > the resume really doesn't go through the firmware.
> 
> Certainly.
> 
> > That said, perhaps the check as proposed is too coarse-grained.  We need to
> > do something like this in the ACPI PM domain code (and we do it already) and
> > for PCI devices that are likely to be affected by the issue at hand.  So
> > what about the appended patch (on top of https://patchwork.kernel.org/patch/7291711/)
> > instead?
> 
> It seems reasonable.  If it turns out that more drivers need to check 
> for firmware interference, we can add them in later on.

OK, let me respin it with a changelog, then.

Thanks,
Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web