[RFC][PATCH 0/2] PM / sleep: Make it possible to check if suspend/resume goes via firmware

[RFC][PATCH 0/2] PM / sleep: Make it possible to check if suspend/resume goes via firmware

[Rafael J. Wysocki]

Hi All,

Patch [1/2] introduces a new mechanism that can be used to check whether or not
platform firmware is going to be involved in system suspend or was involved
in system resume.

Patch [2/2] modifies the PM core to use the mechanism added by [1/2].

Comments welcome.

Thanks,
Rafael

[RFC][PATCH 1/2] PM / sleep: Add flags to indicate platform firmware involvement

[Rafael J. Wysocki]

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

There are quite a few cases in which device drivers, bus types or
even the PM core itself may benefit from knowing whether or not
the platform firmware will be involved in the upcoming system power
transition (during system suspend) or whether or not it was involved
in it (during system resume).

For this reason, introduce global system suspend flags that can be
used by the platform code to expose that information for the benefit
of the other parts of the kernel and make the ACPI core set them
as appropriate.

Users of the new flags will be added later.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/sleep.c    |    3 +++
 include/linux/suspend.h |   30 ++++++++++++++++++++++++++++++
 kernel/power/suspend.c  |    3 +++
 3 files changed, 36 insertions(+)

Index: linux-pm/include/linux/suspend.h
===================================================================
--- linux-pm.orig/include/linux/suspend.h
+++ linux-pm/include/linux/suspend.h
@@ -202,6 +202,36 @@ struct platform_freeze_ops {
 extern void suspend_set_ops(const struct platform_suspend_ops *ops);
 extern int suspend_valid_only_mem(suspend_state_t state);
 
+extern unsigned int pm_suspend_global_flags;
+
+#define PM_SUSPEND_FLAG_FW_SUSPEND	(1 << 0)
+#define PM_SUSPEND_FLAG_FW_RESUME	(1 << 1)
+
+static inline void pm_suspend_clear_flags(void)
+{
+	pm_suspend_global_flags = 0;
+}
+
+static inline void pm_set_suspend_via_firmware(void)
+{
+	pm_suspend_global_flags |= PM_SUSPEND_FLAG_FW_SUSPEND;
+}
+
+static inline void pm_set_resume_via_firmware(void)
+{
+	pm_suspend_global_flags |= PM_SUSPEND_FLAG_FW_RESUME;
+}
+
+static inline bool pm_suspend_via_firmware(void)
+{
+	return !!(pm_suspend_global_flags & PM_SUSPEND_FLAG_FW_SUSPEND);
+}
+
+static inline bool pm_resume_via_firmware(void)
+{
+	return !!(pm_suspend_global_flags & PM_SUSPEND_FLAG_FW_RESUME);
+}
+
 /* Suspend-to-idle state machnine. */
 enum freeze_state {
 	FREEZE_STATE_NONE,      /* Not suspended/suspending. */
Index: linux-pm/kernel/power/suspend.c
===================================================================
--- linux-pm.orig/kernel/power/suspend.c
+++ linux-pm/kernel/power/suspend.c
@@ -35,6 +35,8 @@
 const char *pm_labels[] = { "mem", "standby", "freeze", NULL };
 const char *pm_states[PM_SUSPEND_MAX];
 
+unsigned int pm_suspend_global_flags;
+
 static const struct platform_suspend_ops *suspend_ops;
 static const struct platform_freeze_ops *freeze_ops;
 static DECLARE_WAIT_QUEUE_HEAD(suspend_freeze_wait_head);
@@ -493,6 +495,7 @@ static int enter_state(suspend_state_t s
 #endif
 
 	pr_debug("PM: Preparing system for sleep (%s)\n", pm_states[state]);
+	pm_suspend_clear_flags();
 	error = suspend_prepare(state);
 	if (error)
 		goto Unlock;
Index: linux-pm/drivers/acpi/sleep.c
===================================================================
--- linux-pm.orig/drivers/acpi/sleep.c
+++ linux-pm/drivers/acpi/sleep.c
@@ -487,6 +487,8 @@ static int acpi_suspend_begin(suspend_st
 		pr_err("ACPI does not support sleep state S%u\n", acpi_state);
 		return -ENOSYS;
 	}
+	if (acpi_state > ACPI_STATE_S1)
+		pm_set_suspend_via_firmware();
 
 	acpi_pm_start(acpi_state);
 	return 0;
@@ -522,6 +524,7 @@ static int acpi_suspend_enter(suspend_st
 		if (error)
 			return error;
 		pr_info(PREFIX "Low-level resume complete\n");
+		pm_set_resume_via_firmware();
 		break;
 	}
 	trace_suspend_resume(TPS("acpi_suspend"), acpi_state, false);

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

[Rafael J. Wysocki]

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);
 }
 
 /**

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1306 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 20807 bytes --]

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

[Alan Stern]

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

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

[Rafael J. Wysocki]

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 */
 

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

[Alan Stern]

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

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

[Rafael J. Wysocki]

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

[RFC][PATCH v2 0/2] PM / sleep: Make it possible to check if suspend/resume goes via firmware

[Rafael J. Wysocki]

On Wednesday, September 30, 2015 02:46:10 AM Rafael J. Wysocki wrote:
> Hi All,
> 
> Patch [1/2] introduces a new mechanism that can be used to check whether or not
> platform firmware is going to be involved in system suspend or was involved
> in system resume.

That's still the case, but patch [2/2] modifies the PCI PM layer and the general
ACPI PM domain only to use the mechanism added by the [1/2].

Comments welcome.

Thanks,
Rafael

[RFC][PATCH v2 1/2] PM / sleep: Add flags to indicate platform firmware involvement

[Rafael J. Wysocki]

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

There are quite a few cases in which device drivers, bus types or
even the PM core itself may benefit from knowing whether or not
the platform firmware will be involved in the upcoming system power
transition (during system suspend) or whether or not it was involved
in it (during system resume).

For this reason, introduce global system suspend flags that can be
used by the platform code to expose that information for the benefit
of the other parts of the kernel and make the ACPI core set them
as appropriate.

Users of the new flags will be added later.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

v1 -> v2: Apply EXPORT_SYMBOL_GPL() to pm_suspend_global_flags so modular
drivers can use it too.

---
 drivers/acpi/sleep.c    |    3 +++
 include/linux/suspend.h |   36 ++++++++++++++++++++++++++++++++++++
 kernel/power/suspend.c  |    4 ++++
 3 files changed, 43 insertions(+)

Index: linux-pm/include/linux/suspend.h
===================================================================
--- linux-pm.orig/include/linux/suspend.h
+++ linux-pm/include/linux/suspend.h
@@ -202,6 +202,36 @@ struct platform_freeze_ops {
 extern void suspend_set_ops(const struct platform_suspend_ops *ops);
 extern int suspend_valid_only_mem(suspend_state_t state);
 
+extern unsigned int pm_suspend_global_flags;
+
+#define PM_SUSPEND_FLAG_FW_SUSPEND	(1 << 0)
+#define PM_SUSPEND_FLAG_FW_RESUME	(1 << 1)
+
+static inline void pm_suspend_clear_flags(void)
+{
+	pm_suspend_global_flags = 0;
+}
+
+static inline void pm_set_suspend_via_firmware(void)
+{
+	pm_suspend_global_flags |= PM_SUSPEND_FLAG_FW_SUSPEND;
+}
+
+static inline void pm_set_resume_via_firmware(void)
+{
+	pm_suspend_global_flags |= PM_SUSPEND_FLAG_FW_RESUME;
+}
+
+static inline bool pm_suspend_via_firmware(void)
+{
+	return !!(pm_suspend_global_flags & PM_SUSPEND_FLAG_FW_SUSPEND);
+}
+
+static inline bool pm_resume_via_firmware(void)
+{
+	return !!(pm_suspend_global_flags & PM_SUSPEND_FLAG_FW_RESUME);
+}
+
 /* Suspend-to-idle state machnine. */
 enum freeze_state {
 	FREEZE_STATE_NONE,      /* Not suspended/suspending. */
@@ -241,6 +271,12 @@ extern int pm_suspend(suspend_state_t st
 #else /* !CONFIG_SUSPEND */
 #define suspend_valid_only_mem	NULL
 
+static inline void pm_suspend_clear_flags(void) {}
+static inline void pm_set_suspend_via_firmware(void) {}
+static inline void pm_set_resume_via_firmware(void) {}
+static inline bool pm_suspend_via_firmware(void) { return false; }
+static inline bool pm_resume_via_firmware(void) { return false; }
+
 static inline void suspend_set_ops(const struct platform_suspend_ops *ops) {}
 static inline int pm_suspend(suspend_state_t state) { return -ENOSYS; }
 static inline bool idle_should_freeze(void) { return false; }
Index: linux-pm/kernel/power/suspend.c
===================================================================
--- linux-pm.orig/kernel/power/suspend.c
+++ linux-pm/kernel/power/suspend.c
@@ -35,6 +35,9 @@
 const char *pm_labels[] = { "mem", "standby", "freeze", NULL };
 const char *pm_states[PM_SUSPEND_MAX];
 
+unsigned int pm_suspend_global_flags;
+EXPORT_SYMBOL_GPL(pm_suspend_global_flags);
+
 static const struct platform_suspend_ops *suspend_ops;
 static const struct platform_freeze_ops *freeze_ops;
 static DECLARE_WAIT_QUEUE_HEAD(suspend_freeze_wait_head);
@@ -493,6 +496,7 @@ static int enter_state(suspend_state_t s
 #endif
 
 	pr_debug("PM: Preparing system for sleep (%s)\n", pm_states[state]);
+	pm_suspend_clear_flags();
 	error = suspend_prepare(state);
 	if (error)
 		goto Unlock;
Index: linux-pm/drivers/acpi/sleep.c
===================================================================
--- linux-pm.orig/drivers/acpi/sleep.c
+++ linux-pm/drivers/acpi/sleep.c
@@ -487,6 +487,8 @@ static int acpi_suspend_begin(suspend_st
 		pr_err("ACPI does not support sleep state S%u\n", acpi_state);
 		return -ENOSYS;
 	}
+	if (acpi_state > ACPI_STATE_S1)
+		pm_set_suspend_via_firmware();
 
 	acpi_pm_start(acpi_state);
 	return 0;
@@ -522,6 +524,7 @@ static int acpi_suspend_enter(suspend_st
 		if (error)
 			return error;
 		pr_info(PREFIX "Low-level resume complete\n");
+		pm_set_resume_via_firmware();
 		break;
 	}
 	trace_suspend_resume(TPS("acpi_suspend"), acpi_state, false);

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

[Rafael J. Wysocki]

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

There is a concern that if the platform firmware was involved in
the system resume that's being completed,  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.

The devices that are most likely to be affected by this issue are
PCI devices and ACPI-enumerated devices using the general ACPI PM
domain, so to prevent it from happening for those devices, force a
runtime resume for them if they have their power.direct_complete
flags set and the platform firmware was involved in the resume
transition currently in progress.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

v1 -> v2: Do it for devices that are most likely to be affected (PCI and the
ACPI PM domain users) rather than for all devices in the system.

---
 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 */
 

[PATCH v3 0/3] PM / sleep: Make it possible to check if suspend/resume goes via firmware

[Rafael J. Wysocki]

On Friday, October 02, 2015 03:50:42 AM Rafael J. Wysocki wrote:
> On Wednesday, September 30, 2015 02:46:10 AM Rafael J. Wysocki wrote:
> > Hi All,
> > 
> > Patch [1/2] introduces a new mechanism that can be used to check whether or not
> > platform firmware is going to be involved in system suspend or was involved
> > in system resume.
> 
> That's still the case, but patch [2/2] modifies the PCI PM layer and the general
> ACPI PM domain only to use the mechanism added by the [1/2].

The first two patches are unchanged (in the face of the continuing lack of
comments I'll add them to my v4.4 queue at one point).

The last patch is new and actually is an RFC, but it shows how the new
mechanism introduced by [1/3] can be used.

Thanks,
Rafael

[PATCH v3 1/3] PM / sleep: Add flags to indicate platform firmware involvement

[Rafael J. Wysocki]

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

There are quite a few cases in which device drivers, bus types or
even the PM core itself may benefit from knowing whether or not
the platform firmware will be involved in the upcoming system power
transition (during system suspend) or whether or not it was involved
in it (during system resume).

For this reason, introduce global system suspend flags that can be
used by the platform code to expose that information for the benefit
of the other parts of the kernel and make the ACPI core set them
as appropriate.

Users of the new flags will be added later.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/sleep.c    |    3 +++
 include/linux/suspend.h |   36 ++++++++++++++++++++++++++++++++++++
 kernel/power/suspend.c  |    4 ++++
 3 files changed, 43 insertions(+)

Index: linux-pm/include/linux/suspend.h
===================================================================
--- linux-pm.orig/include/linux/suspend.h
+++ linux-pm/include/linux/suspend.h
@@ -202,6 +202,36 @@ struct platform_freeze_ops {
 extern void suspend_set_ops(const struct platform_suspend_ops *ops);
 extern int suspend_valid_only_mem(suspend_state_t state);
 
+extern unsigned int pm_suspend_global_flags;
+
+#define PM_SUSPEND_FLAG_FW_SUSPEND	(1 << 0)
+#define PM_SUSPEND_FLAG_FW_RESUME	(1 << 1)
+
+static inline void pm_suspend_clear_flags(void)
+{
+	pm_suspend_global_flags = 0;
+}
+
+static inline void pm_set_suspend_via_firmware(void)
+{
+	pm_suspend_global_flags |= PM_SUSPEND_FLAG_FW_SUSPEND;
+}
+
+static inline void pm_set_resume_via_firmware(void)
+{
+	pm_suspend_global_flags |= PM_SUSPEND_FLAG_FW_RESUME;
+}
+
+static inline bool pm_suspend_via_firmware(void)
+{
+	return !!(pm_suspend_global_flags & PM_SUSPEND_FLAG_FW_SUSPEND);
+}
+
+static inline bool pm_resume_via_firmware(void)
+{
+	return !!(pm_suspend_global_flags & PM_SUSPEND_FLAG_FW_RESUME);
+}
+
 /* Suspend-to-idle state machnine. */
 enum freeze_state {
 	FREEZE_STATE_NONE,      /* Not suspended/suspending. */
@@ -241,6 +271,12 @@ extern int pm_suspend(suspend_state_t st
 #else /* !CONFIG_SUSPEND */
 #define suspend_valid_only_mem	NULL
 
+static inline void pm_suspend_clear_flags(void) {}
+static inline void pm_set_suspend_via_firmware(void) {}
+static inline void pm_set_resume_via_firmware(void) {}
+static inline bool pm_suspend_via_firmware(void) { return false; }
+static inline bool pm_resume_via_firmware(void) { return false; }
+
 static inline void suspend_set_ops(const struct platform_suspend_ops *ops) {}
 static inline int pm_suspend(suspend_state_t state) { return -ENOSYS; }
 static inline bool idle_should_freeze(void) { return false; }
Index: linux-pm/kernel/power/suspend.c
===================================================================
--- linux-pm.orig/kernel/power/suspend.c
+++ linux-pm/kernel/power/suspend.c
@@ -35,6 +35,9 @@
 const char *pm_labels[] = { "mem", "standby", "freeze", NULL };
 const char *pm_states[PM_SUSPEND_MAX];
 
+unsigned int pm_suspend_global_flags;
+EXPORT_SYMBOL_GPL(pm_suspend_global_flags);
+
 static const struct platform_suspend_ops *suspend_ops;
 static const struct platform_freeze_ops *freeze_ops;
 static DECLARE_WAIT_QUEUE_HEAD(suspend_freeze_wait_head);
@@ -493,6 +496,7 @@ static int enter_state(suspend_state_t s
 #endif
 
 	pr_debug("PM: Preparing system for sleep (%s)\n", pm_states[state]);
+	pm_suspend_clear_flags();
 	error = suspend_prepare(state);
 	if (error)
 		goto Unlock;
Index: linux-pm/drivers/acpi/sleep.c
===================================================================
--- linux-pm.orig/drivers/acpi/sleep.c
+++ linux-pm/drivers/acpi/sleep.c
@@ -487,6 +487,8 @@ static int acpi_suspend_begin(suspend_st
 		pr_err("ACPI does not support sleep state S%u\n", acpi_state);
 		return -ENOSYS;
 	}
+	if (acpi_state > ACPI_STATE_S1)
+		pm_set_suspend_via_firmware();
 
 	acpi_pm_start(acpi_state);
 	return 0;
@@ -522,6 +524,7 @@ static int acpi_suspend_enter(suspend_st
 		if (error)
 			return error;
 		pr_info(PREFIX "Low-level resume complete\n");
+		pm_set_resume_via_firmware();
 		break;
 	}
 	trace_suspend_resume(TPS("acpi_suspend"), acpi_state, false);

[PATCH v3 2/3] PM / PCI / ACPI: Kick devices that might have been reset by firmware

[Rafael J. Wysocki]

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

There is a concern that if the platform firmware was involved in
the system resume that's being completed,  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.

The devices that are most likely to be affected by this issue are
PCI devices and ACPI-enumerated devices using the general ACPI PM
domain, so to prevent it from happening for those devices, force a
runtime resume for them if they have their power.direct_complete
flags set and the platform firmware was involved in the resume
transition currently in progress.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 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 */
 

[RFC][PATCH v3 3/3] input: i8042: Avoid resetting controller on system suspend/resume

[Rafael J. Wysocki]

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

If the upcoming system suspend is not going to be handled by the
platform firmware, like in the suspend-to-idle case, it is not
necessary to reset the controller in i8042_pm_suspend(), so avoid
doing that.

Moreover, if the system resume currently in progress has not been
started by the platform firmware, like in the suspend-to-idle case,
i8042_controller_resume() need not be called by i8042_pm_resume(),
so avoid doing that too in that case.

Additionally, try to catch the event that woke up the system by
calling the interrupt handler early during system resume if it has
not been started by the platform firmware.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/input/serio/i8042.c |   15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

Index: linux-pm/drivers/input/serio/i8042.c
===================================================================
--- linux-pm.orig/drivers/input/serio/i8042.c
+++ linux-pm/drivers/input/serio/i8042.c
@@ -24,6 +24,7 @@
 #include <linux/platform_device.h>
 #include <linux/i8042.h>
 #include <linux/slab.h>
+#include <linux/suspend.h>
 
 #include <asm/io.h>
 
@@ -1170,7 +1171,8 @@ static int i8042_pm_suspend(struct devic
 {
 	int i;
 
-	i8042_controller_reset(true);
+	if (pm_suspend_via_firmware())
+		i8042_controller_reset(true);
 
 	/* Set up serio interrupts for system wakeup. */
 	for (i = 0; i < I8042_NUM_PORTS; i++) {
@@ -1183,6 +1185,14 @@ static int i8042_pm_suspend(struct devic
 	return 0;
 }
 
+static int i8042_pm_resume_noirq(struct device *dev)
+{
+	if (!pm_resume_via_firmware())
+		i8042_interrupt(0, NULL);
+
+	return 0;
+}
+
 static int i8042_pm_resume(struct device *dev)
 {
 	int i;
@@ -1199,7 +1209,7 @@ static int i8042_pm_resume(struct device
 	 * to bring it in a sane state. (In case of S2D we expect
 	 * BIOS to reset the controller for us.)
 	 */
-	return i8042_controller_resume(true);
+	return pm_resume_via_firmware() ? i8042_controller_resume(true) : 0;
 }
 
 static int i8042_pm_thaw(struct device *dev)
@@ -1223,6 +1233,7 @@ static int i8042_pm_restore(struct devic
 
 static const struct dev_pm_ops i8042_pm_ops = {
 	.suspend	= i8042_pm_suspend,
+	.resume_noirq	= i8042_pm_resume_noirq,
 	.resume		= i8042_pm_resume,
 	.thaw		= i8042_pm_thaw,
 	.poweroff	= i8042_pm_reset,

Re: [RFC][PATCH v3 3/3] input: i8042: Avoid resetting controller on system suspend/resume

[Dmitry Torokhov]

On Wed, Oct 07, 2015 at 12:53:49AM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> If the upcoming system suspend is not going to be handled by the
> platform firmware, like in the suspend-to-idle case, it is not
> necessary to reset the controller in i8042_pm_suspend(), so avoid
> doing that.
> 
> Moreover, if the system resume currently in progress has not been
> started by the platform firmware, like in the suspend-to-idle case,
> i8042_controller_resume() need not be called by i8042_pm_resume(),
> so avoid doing that too in that case.
> 
> Additionally, try to catch the event that woke up the system by
> calling the interrupt handler early during system resume if it has
> not been started by the platform firmware.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/input/serio/i8042.c |   15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> Index: linux-pm/drivers/input/serio/i8042.c
> ===================================================================
> --- linux-pm.orig/drivers/input/serio/i8042.c
> +++ linux-pm/drivers/input/serio/i8042.c
> @@ -24,6 +24,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/i8042.h>
>  #include <linux/slab.h>
> +#include <linux/suspend.h>
>  
>  #include <asm/io.h>
>  
> @@ -1170,7 +1171,8 @@ static int i8042_pm_suspend(struct devic
>  {
>  	int i;
>  
> -	i8042_controller_reset(true);
> +	if (pm_suspend_via_firmware())
> +		i8042_controller_reset(true);
>  
>  	/* Set up serio interrupts for system wakeup. */
>  	for (i = 0; i < I8042_NUM_PORTS; i++) {
> @@ -1183,6 +1185,14 @@ static int i8042_pm_suspend(struct devic
>  	return 0;
>  }
>  
> +static int i8042_pm_resume_noirq(struct device *dev)
> +{
> +	if (!pm_resume_via_firmware())
> +		i8042_interrupt(0, NULL);
> +
> +	return 0;
> +}
> +
>  static int i8042_pm_resume(struct device *dev)
>  {
>  	int i;
> @@ -1199,7 +1209,7 @@ static int i8042_pm_resume(struct device
>  	 * to bring it in a sane state. (In case of S2D we expect
>  	 * BIOS to reset the controller for us.)
>  	 */
> -	return i8042_controller_resume(true);
> +	return pm_resume_via_firmware() ? i8042_controller_resume(true) : 0;

What happens if we were going to suspend via firmware so we reset the
controller but then we got wakeup condition and we actually did not
suspend. What pm_resume_via_firmware() will return in this case?

Thanks.

-- 
Dmitry

Re: [RFC][PATCH v3 3/3] input: i8042: Avoid resetting controller on system suspend/resume

[Rafael J. Wysocki]

On Tuesday, October 06, 2015 03:34:42 PM Dmitry Torokhov wrote:
> On Wed, Oct 07, 2015 at 12:53:49AM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > If the upcoming system suspend is not going to be handled by the
> > platform firmware, like in the suspend-to-idle case, it is not
> > necessary to reset the controller in i8042_pm_suspend(), so avoid
> > doing that.
> > 
> > Moreover, if the system resume currently in progress has not been
> > started by the platform firmware, like in the suspend-to-idle case,
> > i8042_controller_resume() need not be called by i8042_pm_resume(),
> > so avoid doing that too in that case.
> > 
> > Additionally, try to catch the event that woke up the system by
> > calling the interrupt handler early during system resume if it has
> > not been started by the platform firmware.
> > 
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/input/serio/i8042.c |   15 +++++++++++++--
> >  1 file changed, 13 insertions(+), 2 deletions(-)
> > 
> > Index: linux-pm/drivers/input/serio/i8042.c
> > ===================================================================
> > --- linux-pm.orig/drivers/input/serio/i8042.c
> > +++ linux-pm/drivers/input/serio/i8042.c
> > @@ -24,6 +24,7 @@
> >  #include <linux/platform_device.h>
> >  #include <linux/i8042.h>
> >  #include <linux/slab.h>
> > +#include <linux/suspend.h>
> >  
> >  #include <asm/io.h>
> >  
> > @@ -1170,7 +1171,8 @@ static int i8042_pm_suspend(struct devic
> >  {
> >  	int i;
> >  
> > -	i8042_controller_reset(true);
> > +	if (pm_suspend_via_firmware())
> > +		i8042_controller_reset(true);
> >  
> >  	/* Set up serio interrupts for system wakeup. */
> >  	for (i = 0; i < I8042_NUM_PORTS; i++) {
> > @@ -1183,6 +1185,14 @@ static int i8042_pm_suspend(struct devic
> >  	return 0;
> >  }
> >  
> > +static int i8042_pm_resume_noirq(struct device *dev)
> > +{
> > +	if (!pm_resume_via_firmware())
> > +		i8042_interrupt(0, NULL);
> > +
> > +	return 0;
> > +}
> > +
> >  static int i8042_pm_resume(struct device *dev)
> >  {
> >  	int i;
> > @@ -1199,7 +1209,7 @@ static int i8042_pm_resume(struct device
> >  	 * to bring it in a sane state. (In case of S2D we expect
> >  	 * BIOS to reset the controller for us.)
> >  	 */
> > -	return i8042_controller_resume(true);
> > +	return pm_resume_via_firmware() ? i8042_controller_resume(true) : 0;
> 
> What happens if we were going to suspend via firmware so we reset the
> controller but then we got wakeup condition and we actually did not
> suspend. What pm_resume_via_firmware() will return in this case?

It will return 'false'.  Do we need to resume the controller then?  But I guess
'false' should be passed to i8042_controller_resume() in that case?

Rafael

Re: [RFC][PATCH v3 3/3] input: i8042: Avoid resetting controller on system suspend/resume

[Dmitry Torokhov]

On Wed, Oct 07, 2015 at 01:08:30AM +0200, Rafael J. Wysocki wrote:
> On Tuesday, October 06, 2015 03:34:42 PM Dmitry Torokhov wrote:
> > On Wed, Oct 07, 2015 at 12:53:49AM +0200, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > 
> > > If the upcoming system suspend is not going to be handled by the
> > > platform firmware, like in the suspend-to-idle case, it is not
> > > necessary to reset the controller in i8042_pm_suspend(), so avoid
> > > doing that.
> > > 
> > > Moreover, if the system resume currently in progress has not been
> > > started by the platform firmware, like in the suspend-to-idle case,
> > > i8042_controller_resume() need not be called by i8042_pm_resume(),
> > > so avoid doing that too in that case.
> > > 
> > > Additionally, try to catch the event that woke up the system by
> > > calling the interrupt handler early during system resume if it has
> > > not been started by the platform firmware.
> > > 
> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > ---
> > >  drivers/input/serio/i8042.c |   15 +++++++++++++--
> > >  1 file changed, 13 insertions(+), 2 deletions(-)
> > > 
> > > Index: linux-pm/drivers/input/serio/i8042.c
> > > ===================================================================
> > > --- linux-pm.orig/drivers/input/serio/i8042.c
> > > +++ linux-pm/drivers/input/serio/i8042.c
> > > @@ -24,6 +24,7 @@
> > >  #include <linux/platform_device.h>
> > >  #include <linux/i8042.h>
> > >  #include <linux/slab.h>
> > > +#include <linux/suspend.h>
> > >  
> > >  #include <asm/io.h>
> > >  
> > > @@ -1170,7 +1171,8 @@ static int i8042_pm_suspend(struct devic
> > >  {
> > >  	int i;
> > >  
> > > -	i8042_controller_reset(true);
> > > +	if (pm_suspend_via_firmware())
> > > +		i8042_controller_reset(true);
> > >  
> > >  	/* Set up serio interrupts for system wakeup. */
> > >  	for (i = 0; i < I8042_NUM_PORTS; i++) {
> > > @@ -1183,6 +1185,14 @@ static int i8042_pm_suspend(struct devic
> > >  	return 0;
> > >  }
> > >  
> > > +static int i8042_pm_resume_noirq(struct device *dev)
> > > +{
> > > +	if (!pm_resume_via_firmware())
> > > +		i8042_interrupt(0, NULL);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  static int i8042_pm_resume(struct device *dev)
> > >  {
> > >  	int i;
> > > @@ -1199,7 +1209,7 @@ static int i8042_pm_resume(struct device
> > >  	 * to bring it in a sane state. (In case of S2D we expect
> > >  	 * BIOS to reset the controller for us.)
> > >  	 */
> > > -	return i8042_controller_resume(true);
> > > +	return pm_resume_via_firmware() ? i8042_controller_resume(true) : 0;
> > 
> > What happens if we were going to suspend via firmware so we reset the
> > controller but then we got wakeup condition and we actually did not
> > suspend. What pm_resume_via_firmware() will return in this case?
> 
> It will return 'false'.  Do we need to resume the controller then?

Yes.

>  But I guess
> 'false' should be passed to i8042_controller_resume() in that case?

Yes, we do not need to reset the controller in this case, just
reactivate multiplexing mode, etc.

Thanks.

-- 
Dmitry

Re: [RFC][PATCH v3 3/3] input: i8042: Avoid resetting controller on system suspend/resume

[Rafael J. Wysocki]

On Tuesday, October 06, 2015 03:43:08 PM Dmitry Torokhov wrote:
> On Wed, Oct 07, 2015 at 01:08:30AM +0200, Rafael J. Wysocki wrote:
> > On Tuesday, October 06, 2015 03:34:42 PM Dmitry Torokhov wrote:
> > > On Wed, Oct 07, 2015 at 12:53:49AM +0200, Rafael J. Wysocki wrote:
> > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > 
> > > > If the upcoming system suspend is not going to be handled by the
> > > > platform firmware, like in the suspend-to-idle case, it is not
> > > > necessary to reset the controller in i8042_pm_suspend(), so avoid
> > > > doing that.
> > > > 
> > > > Moreover, if the system resume currently in progress has not been
> > > > started by the platform firmware, like in the suspend-to-idle case,
> > > > i8042_controller_resume() need not be called by i8042_pm_resume(),
> > > > so avoid doing that too in that case.
> > > > 
> > > > Additionally, try to catch the event that woke up the system by
> > > > calling the interrupt handler early during system resume if it has
> > > > not been started by the platform firmware.
> > > > 
> > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > ---
> > > >  drivers/input/serio/i8042.c |   15 +++++++++++++--
> > > >  1 file changed, 13 insertions(+), 2 deletions(-)
> > > > 
> > > > Index: linux-pm/drivers/input/serio/i8042.c
> > > > ===================================================================
> > > > --- linux-pm.orig/drivers/input/serio/i8042.c
> > > > +++ linux-pm/drivers/input/serio/i8042.c
> > > > @@ -24,6 +24,7 @@
> > > >  #include <linux/platform_device.h>
> > > >  #include <linux/i8042.h>
> > > >  #include <linux/slab.h>
> > > > +#include <linux/suspend.h>
> > > >  
> > > >  #include <asm/io.h>
> > > >  
> > > > @@ -1170,7 +1171,8 @@ static int i8042_pm_suspend(struct devic
> > > >  {
> > > >  	int i;
> > > >  
> > > > -	i8042_controller_reset(true);
> > > > +	if (pm_suspend_via_firmware())
> > > > +		i8042_controller_reset(true);
> > > >  
> > > >  	/* Set up serio interrupts for system wakeup. */
> > > >  	for (i = 0; i < I8042_NUM_PORTS; i++) {
> > > > @@ -1183,6 +1185,14 @@ static int i8042_pm_suspend(struct devic
> > > >  	return 0;
> > > >  }
> > > >  
> > > > +static int i8042_pm_resume_noirq(struct device *dev)
> > > > +{
> > > > +	if (!pm_resume_via_firmware())
> > > > +		i8042_interrupt(0, NULL);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > >  static int i8042_pm_resume(struct device *dev)
> > > >  {
> > > >  	int i;
> > > > @@ -1199,7 +1209,7 @@ static int i8042_pm_resume(struct device
> > > >  	 * to bring it in a sane state. (In case of S2D we expect
> > > >  	 * BIOS to reset the controller for us.)
> > > >  	 */
> > > > -	return i8042_controller_resume(true);
> > > > +	return pm_resume_via_firmware() ? i8042_controller_resume(true) : 0;
> > > 
> > > What happens if we were going to suspend via firmware so we reset the
> > > controller but then we got wakeup condition and we actually did not
> > > suspend. What pm_resume_via_firmware() will return in this case?
> > 
> > It will return 'false'.  Do we need to resume the controller then?
> 
> Yes.
> 
> >  But I guess
> > 'false' should be passed to i8042_controller_resume() in that case?
> 
> Yes, we do not need to reset the controller in this case, just
> reactivate multiplexing mode, etc.

So something like this I suppose:

---
 drivers/input/serio/i8042.c |   16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

Index: linux-pm/drivers/input/serio/i8042.c
===================================================================
--- linux-pm.orig/drivers/input/serio/i8042.c
+++ linux-pm/drivers/input/serio/i8042.c
@@ -24,6 +24,7 @@
 #include <linux/platform_device.h>
 #include <linux/i8042.h>
 #include <linux/slab.h>
+#include <linux/suspend.h>
 
 #include <asm/io.h>
 
@@ -1170,7 +1171,8 @@ static int i8042_pm_suspend(struct devic
 {
 	int i;
 
-	i8042_controller_reset(true);
+	if (pm_suspend_via_firmware())
+		i8042_controller_reset(true);
 
 	/* Set up serio interrupts for system wakeup. */
 	for (i = 0; i < I8042_NUM_PORTS; i++) {
@@ -1183,6 +1185,14 @@ static int i8042_pm_suspend(struct devic
 	return 0;
 }
 
+static int i8042_pm_resume_noirq(struct device *dev)
+{
+	if (!pm_resume_via_firmware())
+		i8042_interrupt(0, NULL);
+
+	return 0;
+}
+
 static int i8042_pm_resume(struct device *dev)
 {
 	int i;
@@ -1199,7 +1209,8 @@ static int i8042_pm_resume(struct device
 	 * to bring it in a sane state. (In case of S2D we expect
 	 * BIOS to reset the controller for us.)
 	 */
-	return i8042_controller_resume(true);
+	return pm_suspend_via_firmware() ?
+		i8042_controller_resume(pm_resume_via_firmware()) : 0;
 }
 
 static int i8042_pm_thaw(struct device *dev)
@@ -1223,6 +1234,7 @@ static int i8042_pm_restore(struct devic
 
 static const struct dev_pm_ops i8042_pm_ops = {
 	.suspend	= i8042_pm_suspend,
+	.resume_noirq	= i8042_pm_resume_noirq,
 	.resume		= i8042_pm_resume,
 	.thaw		= i8042_pm_thaw,
 	.poweroff	= i8042_pm_reset,

Re: Re: [RFC][PATCH v3 3/3] input: i8042: Avoid resetting controller on system suspend/resume

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1752 bytes --]

Hi Rafael,

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

config: i386-randconfig-i1-201540 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers/input/serio/i8042.c: In function 'i8042_pm_suspend':
>> drivers/input/serio/i8042.c:1174:6: error: implicit declaration of function 'pm_suspend_via_firmware' [-Werror=implicit-function-declaration]
     if (pm_suspend_via_firmware())
         ^
   drivers/input/serio/i8042.c: In function 'i8042_pm_resume_noirq':
>> drivers/input/serio/i8042.c:1190:7: error: implicit declaration of function 'pm_resume_via_firmware' [-Werror=implicit-function-declaration]
     if (!pm_resume_via_firmware())
          ^
   cc1: some warnings being treated as errors

vim +/pm_suspend_via_firmware +1174 drivers/input/serio/i8042.c

  1168	 */
  1169	
  1170	static int i8042_pm_suspend(struct device *dev)
  1171	{
  1172		int i;
  1173	
> 1174		if (pm_suspend_via_firmware())
  1175			i8042_controller_reset(true);
  1176	
  1177		/* Set up serio interrupts for system wakeup. */
  1178		for (i = 0; i < I8042_NUM_PORTS; i++) {
  1179			struct serio *serio = i8042_ports[i].serio;
  1180	
  1181			if (serio && device_may_wakeup(&serio->dev))
  1182				enable_irq_wake(i8042_ports[i].irq);
  1183		}
  1184	
  1185		return 0;
  1186	}
  1187	
  1188	static int i8042_pm_resume_noirq(struct device *dev)
  1189	{
> 1190		if (!pm_resume_via_firmware())
  1191			i8042_interrupt(0, NULL);
  1192	
  1193		return 0;

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

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 20381 bytes --]

Re: [RFC][PATCH v3 3/3] input: i8042: Avoid resetting controller on system suspend/resume

[Dmitry Torokhov]

On Wed, Oct 07, 2015 at 01:31:44AM +0200, Rafael J. Wysocki wrote:
> On Tuesday, October 06, 2015 03:43:08 PM Dmitry Torokhov wrote:
> > On Wed, Oct 07, 2015 at 01:08:30AM +0200, Rafael J. Wysocki wrote:
> > > On Tuesday, October 06, 2015 03:34:42 PM Dmitry Torokhov wrote:
> > > > On Wed, Oct 07, 2015 at 12:53:49AM +0200, Rafael J. Wysocki wrote:
> > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > 
> > > > > If the upcoming system suspend is not going to be handled by the
> > > > > platform firmware, like in the suspend-to-idle case, it is not
> > > > > necessary to reset the controller in i8042_pm_suspend(), so avoid
> > > > > doing that.
> > > > > 
> > > > > Moreover, if the system resume currently in progress has not been
> > > > > started by the platform firmware, like in the suspend-to-idle case,
> > > > > i8042_controller_resume() need not be called by i8042_pm_resume(),
> > > > > so avoid doing that too in that case.
> > > > > 
> > > > > Additionally, try to catch the event that woke up the system by
> > > > > calling the interrupt handler early during system resume if it has
> > > > > not been started by the platform firmware.
> > > > > 
> > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > ---
> > > > >  drivers/input/serio/i8042.c |   15 +++++++++++++--
> > > > >  1 file changed, 13 insertions(+), 2 deletions(-)
> > > > > 
> > > > > Index: linux-pm/drivers/input/serio/i8042.c
> > > > > ===================================================================
> > > > > --- linux-pm.orig/drivers/input/serio/i8042.c
> > > > > +++ linux-pm/drivers/input/serio/i8042.c
> > > > > @@ -24,6 +24,7 @@
> > > > >  #include <linux/platform_device.h>
> > > > >  #include <linux/i8042.h>
> > > > >  #include <linux/slab.h>
> > > > > +#include <linux/suspend.h>
> > > > >  
> > > > >  #include <asm/io.h>
> > > > >  
> > > > > @@ -1170,7 +1171,8 @@ static int i8042_pm_suspend(struct devic
> > > > >  {
> > > > >  	int i;
> > > > >  
> > > > > -	i8042_controller_reset(true);
> > > > > +	if (pm_suspend_via_firmware())
> > > > > +		i8042_controller_reset(true);
> > > > >  
> > > > >  	/* Set up serio interrupts for system wakeup. */
> > > > >  	for (i = 0; i < I8042_NUM_PORTS; i++) {
> > > > > @@ -1183,6 +1185,14 @@ static int i8042_pm_suspend(struct devic
> > > > >  	return 0;
> > > > >  }
> > > > >  
> > > > > +static int i8042_pm_resume_noirq(struct device *dev)
> > > > > +{
> > > > > +	if (!pm_resume_via_firmware())
> > > > > +		i8042_interrupt(0, NULL);
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > >  static int i8042_pm_resume(struct device *dev)
> > > > >  {
> > > > >  	int i;
> > > > > @@ -1199,7 +1209,7 @@ static int i8042_pm_resume(struct device
> > > > >  	 * to bring it in a sane state. (In case of S2D we expect
> > > > >  	 * BIOS to reset the controller for us.)
> > > > >  	 */
> > > > > -	return i8042_controller_resume(true);
> > > > > +	return pm_resume_via_firmware() ? i8042_controller_resume(true) : 0;
> > > > 
> > > > What happens if we were going to suspend via firmware so we reset the
> > > > controller but then we got wakeup condition and we actually did not
> > > > suspend. What pm_resume_via_firmware() will return in this case?
> > > 
> > > It will return 'false'.  Do we need to resume the controller then?
> > 
> > Yes.
> > 
> > >  But I guess
> > > 'false' should be passed to i8042_controller_resume() in that case?
> > 
> > Yes, we do not need to reset the controller in this case, just
> > reactivate multiplexing mode, etc.
> 
> So something like this I suppose:
> 
> ---
>  drivers/input/serio/i8042.c |   16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> Index: linux-pm/drivers/input/serio/i8042.c
> ===================================================================
> --- linux-pm.orig/drivers/input/serio/i8042.c
> +++ linux-pm/drivers/input/serio/i8042.c
> @@ -24,6 +24,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/i8042.h>
>  #include <linux/slab.h>
> +#include <linux/suspend.h>
>  
>  #include <asm/io.h>
>  
> @@ -1170,7 +1171,8 @@ static int i8042_pm_suspend(struct devic
>  {
>  	int i;
>  
> -	i8042_controller_reset(true);
> +	if (pm_suspend_via_firmware())
> +		i8042_controller_reset(true);
>  
>  	/* Set up serio interrupts for system wakeup. */
>  	for (i = 0; i < I8042_NUM_PORTS; i++) {
> @@ -1183,6 +1185,14 @@ static int i8042_pm_suspend(struct devic
>  	return 0;
>  }
>  
> +static int i8042_pm_resume_noirq(struct device *dev)
> +{
> +	if (!pm_resume_via_firmware())
> +		i8042_interrupt(0, NULL);
> +
> +	return 0;
> +}
> +
>  static int i8042_pm_resume(struct device *dev)
>  {
>  	int i;
> @@ -1199,7 +1209,8 @@ static int i8042_pm_resume(struct device
>  	 * to bring it in a sane state. (In case of S2D we expect
>  	 * BIOS to reset the controller for us.)
>  	 */
> -	return i8042_controller_resume(true);
> +	return pm_suspend_via_firmware() ?
> +		i8042_controller_resume(pm_resume_via_firmware()) : 0;

Hmm, looks right, maybe just be more verbose...

	/*
	 * If firmware was not going to be involved in suspend we did
	 * not restore controller state to whatever it was when we were
	 * booting, so we do not need to do anything.
	 */
	if (!pm_suspend_via_firmware())
		return 0;

	/*
	 * We only need to reset controller if we are resuming after
	 * handing off control to the firmware, otherwise we can
	 * simply restore the mode.
	 */
	do_reset = pm_resume_via_firmware();

	return i8042_controller_resume(do_reset);

>  }
>  
>  static int i8042_pm_thaw(struct device *dev)
> @@ -1223,6 +1234,7 @@ static int i8042_pm_restore(struct devic
>  
>  static const struct dev_pm_ops i8042_pm_ops = {
>  	.suspend	= i8042_pm_suspend,
> +	.resume_noirq	= i8042_pm_resume_noirq,
>  	.resume		= i8042_pm_resume,
>  	.thaw		= i8042_pm_thaw,
>  	.poweroff	= i8042_pm_reset,
> 

-- 
Dmitry

Re: [RFC][PATCH v3 3/3] input: i8042: Avoid resetting controller on system suspend/resume

[Rafael J. Wysocki]

On Wed, Oct 7, 2015 at 1:26 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Wed, Oct 07, 2015 at 01:31:44AM +0200, Rafael J. Wysocki wrote:
>> On Tuesday, October 06, 2015 03:43:08 PM Dmitry Torokhov wrote:
>> > On Wed, Oct 07, 2015 at 01:08:30AM +0200, Rafael J. Wysocki wrote:
>> > > On Tuesday, October 06, 2015 03:34:42 PM Dmitry Torokhov wrote:

[cut]

>> So something like this I suppose:
>>
>> ---
>>  drivers/input/serio/i8042.c |   16 ++++++++++++++--
>>  1 file changed, 14 insertions(+), 2 deletions(-)
>>
>> Index: linux-pm/drivers/input/serio/i8042.c
>> ===================================================================
>> --- linux-pm.orig/drivers/input/serio/i8042.c
>> +++ linux-pm/drivers/input/serio/i8042.c
>> @@ -24,6 +24,7 @@
>>  #include <linux/platform_device.h>
>>  #include <linux/i8042.h>
>>  #include <linux/slab.h>
>> +#include <linux/suspend.h>
>>
>>  #include <asm/io.h>
>>
>> @@ -1170,7 +1171,8 @@ static int i8042_pm_suspend(struct devic
>>  {
>>       int i;
>>
>> -     i8042_controller_reset(true);
>> +     if (pm_suspend_via_firmware())
>> +             i8042_controller_reset(true);
>>
>>       /* Set up serio interrupts for system wakeup. */
>>       for (i = 0; i < I8042_NUM_PORTS; i++) {
>> @@ -1183,6 +1185,14 @@ static int i8042_pm_suspend(struct devic
>>       return 0;
>>  }
>>
>> +static int i8042_pm_resume_noirq(struct device *dev)
>> +{
>> +     if (!pm_resume_via_firmware())
>> +             i8042_interrupt(0, NULL);
>> +
>> +     return 0;
>> +}
>> +
>>  static int i8042_pm_resume(struct device *dev)
>>  {
>>       int i;
>> @@ -1199,7 +1209,8 @@ static int i8042_pm_resume(struct device
>>        * to bring it in a sane state. (In case of S2D we expect
>>        * BIOS to reset the controller for us.)
>>        */
>> -     return i8042_controller_resume(true);
>> +     return pm_suspend_via_firmware() ?
>> +             i8042_controller_resume(pm_resume_via_firmware()) : 0;
>
> Hmm, looks right, maybe just be more verbose...
>
>         /*
>          * If firmware was not going to be involved in suspend we did
>          * not restore controller state to whatever it was when we were
>          * booting, so we do not need to do anything.
>          */
>         if (!pm_suspend_via_firmware())
>                 return 0;
>
>         /*
>          * We only need to reset controller if we are resuming after
>          * handing off control to the firmware, otherwise we can
>          * simply restore the mode.
>          */
>         do_reset = pm_resume_via_firmware();
>
>         return i8042_controller_resume(do_reset);
>
>>  }
>>
>>  static int i8042_pm_thaw(struct device *dev)
>> @@ -1223,6 +1234,7 @@ static int i8042_pm_restore(struct devic
>>
>>  static const struct dev_pm_ops i8042_pm_ops = {
>>       .suspend        = i8042_pm_suspend,
>> +     .resume_noirq   = i8042_pm_resume_noirq,
>>       .resume         = i8042_pm_resume,
>>       .thaw           = i8042_pm_thaw,
>>       .poweroff       = i8042_pm_reset,

OK

I'll send a new version shortly.

Thanks,
Rafael

[Update][PATCH 3/3] input: i8042: Avoid resetting controller on system suspend/resume

[Rafael J. Wysocki]

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

If the upcoming system suspend is not going to be handled by the
platform firmware, like in the suspend-to-idle case, it is not
necessary to reset the controller in i8042_pm_suspend(), so avoid
doing that.

Moreover, if the system resume currently in progress has not been
started by the platform firmware, like in the suspend-to-idle case,
i8042_controller_resume() need not be called by i8042_pm_resume(),
so avoid doing that too in that case.

Additionally, try to catch the event that woke up the system by
calling the interrupt handler early during system resume if it has
not been started by the platform firmware.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/input/serio/i8042.c |   32 +++++++++++++++++++++++++++-----
 1 file changed, 27 insertions(+), 5 deletions(-)

Index: linux-pm/drivers/input/serio/i8042.c
===================================================================
--- linux-pm.orig/drivers/input/serio/i8042.c
+++ linux-pm/drivers/input/serio/i8042.c
@@ -24,6 +24,7 @@
 #include <linux/platform_device.h>
 #include <linux/i8042.h>
 #include <linux/slab.h>
+#include <linux/suspend.h>
 
 #include <asm/io.h>
 
@@ -1170,7 +1171,8 @@ static int i8042_pm_suspend(struct devic
 {
 	int i;
 
-	i8042_controller_reset(true);
+	if (pm_suspend_via_firmware())
+		i8042_controller_reset(true);
 
 	/* Set up serio interrupts for system wakeup. */
 	for (i = 0; i < I8042_NUM_PORTS; i++) {
@@ -1183,8 +1185,17 @@ static int i8042_pm_suspend(struct devic
 	return 0;
 }
 
+static int i8042_pm_resume_noirq(struct device *dev)
+{
+	if (!pm_resume_via_firmware())
+		i8042_interrupt(0, NULL);
+
+	return 0;
+}
+
 static int i8042_pm_resume(struct device *dev)
 {
+	bool force_reset;
 	int i;
 
 	for (i = 0; i < I8042_NUM_PORTS; i++) {
@@ -1195,11 +1206,21 @@ static int i8042_pm_resume(struct device
 	}
 
 	/*
-	 * On resume from S2R we always try to reset the controller
-	 * to bring it in a sane state. (In case of S2D we expect
-	 * BIOS to reset the controller for us.)
+	 * If platform firmware was not going to be involved in suspend, we did
+	 * not restore the controller state to whatever it had been at boot
+	 * time, so we do not need to do anything.
 	 */
-	return i8042_controller_resume(true);
+	if (!pm_suspend_via_firmware())
+		return 0;
+
+	/*
+	 * We only need to reset the controller if we are resuming after handing
+	 * off control to the platform firmware, otherwise we can simply restore
+	 * the mode.
+	 */
+	force_reset = pm_resume_via_firmware();
+
+	return i8042_controller_resume(force_reset);
 }
 
 static int i8042_pm_thaw(struct device *dev)
@@ -1223,6 +1244,7 @@ static int i8042_pm_restore(struct devic
 
 static const struct dev_pm_ops i8042_pm_ops = {
 	.suspend	= i8042_pm_suspend,
+	.resume_noirq	= i8042_pm_resume_noirq,
 	.resume		= i8042_pm_resume,
 	.thaw		= i8042_pm_thaw,
 	.poweroff	= i8042_pm_reset,

Re: [Update][PATCH 3/3] input: i8042: Avoid resetting controller on system suspend/resume

[Rafael J. Wysocki]

On Wednesday, October 07, 2015 03:03:57 AM Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> If the upcoming system suspend is not going to be handled by the
> platform firmware, like in the suspend-to-idle case, it is not
> necessary to reset the controller in i8042_pm_suspend(), so avoid
> doing that.
> 
> Moreover, if the system resume currently in progress has not been
> started by the platform firmware, like in the suspend-to-idle case,
> i8042_controller_resume() need not be called by i8042_pm_resume(),
> so avoid doing that too in that case.
> 
> Additionally, try to catch the event that woke up the system by
> calling the interrupt handler early during system resume if it has
> not been started by the platform firmware.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

If there are no more comments here, I'll tentatively queue it up for v4.4
along with the [1-2/3] from this series.

Please let me know if there are any problems with that.

Thanks,
Rafael


> ---
>  drivers/input/serio/i8042.c |   32 +++++++++++++++++++++++++++-----
>  1 file changed, 27 insertions(+), 5 deletions(-)
> 
> Index: linux-pm/drivers/input/serio/i8042.c
> ===================================================================
> --- linux-pm.orig/drivers/input/serio/i8042.c
> +++ linux-pm/drivers/input/serio/i8042.c
> @@ -24,6 +24,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/i8042.h>
>  #include <linux/slab.h>
> +#include <linux/suspend.h>
>  
>  #include <asm/io.h>
>  
> @@ -1170,7 +1171,8 @@ static int i8042_pm_suspend(struct devic
>  {
>  	int i;
>  
> -	i8042_controller_reset(true);
> +	if (pm_suspend_via_firmware())
> +		i8042_controller_reset(true);
>  
>  	/* Set up serio interrupts for system wakeup. */
>  	for (i = 0; i < I8042_NUM_PORTS; i++) {
> @@ -1183,8 +1185,17 @@ static int i8042_pm_suspend(struct devic
>  	return 0;
>  }
>  
> +static int i8042_pm_resume_noirq(struct device *dev)
> +{
> +	if (!pm_resume_via_firmware())
> +		i8042_interrupt(0, NULL);
> +
> +	return 0;
> +}
> +
>  static int i8042_pm_resume(struct device *dev)
>  {
> +	bool force_reset;
>  	int i;
>  
>  	for (i = 0; i < I8042_NUM_PORTS; i++) {
> @@ -1195,11 +1206,21 @@ static int i8042_pm_resume(struct device
>  	}
>  
>  	/*
> -	 * On resume from S2R we always try to reset the controller
> -	 * to bring it in a sane state. (In case of S2D we expect
> -	 * BIOS to reset the controller for us.)
> +	 * If platform firmware was not going to be involved in suspend, we did
> +	 * not restore the controller state to whatever it had been at boot
> +	 * time, so we do not need to do anything.
>  	 */
> -	return i8042_controller_resume(true);
> +	if (!pm_suspend_via_firmware())
> +		return 0;
> +
> +	/*
> +	 * We only need to reset the controller if we are resuming after handing
> +	 * off control to the platform firmware, otherwise we can simply restore
> +	 * the mode.
> +	 */
> +	force_reset = pm_resume_via_firmware();
> +
> +	return i8042_controller_resume(force_reset);
>  }
>  
>  static int i8042_pm_thaw(struct device *dev)
> @@ -1223,6 +1244,7 @@ static int i8042_pm_restore(struct devic
>  
>  static const struct dev_pm_ops i8042_pm_ops = {
>  	.suspend	= i8042_pm_suspend,
> +	.resume_noirq	= i8042_pm_resume_noirq,
>  	.resume		= i8042_pm_resume,
>  	.thaw		= i8042_pm_thaw,
>  	.poweroff	= i8042_pm_reset,
> 
> --

Re: [Update][PATCH 3/3] input: i8042: Avoid resetting controller on system suspend/resume

[Dmitry Torokhov]

On Mon, Oct 12, 2015 at 10:17:05PM +0200, Rafael J. Wysocki wrote:
> On Wednesday, October 07, 2015 03:03:57 AM Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > If the upcoming system suspend is not going to be handled by the
> > platform firmware, like in the suspend-to-idle case, it is not
> > necessary to reset the controller in i8042_pm_suspend(), so avoid
> > doing that.
> > 
> > Moreover, if the system resume currently in progress has not been
> > started by the platform firmware, like in the suspend-to-idle case,
> > i8042_controller_resume() need not be called by i8042_pm_resume(),
> > so avoid doing that too in that case.
> > 
> > Additionally, try to catch the event that woke up the system by
> > calling the interrupt handler early during system resume if it has
> > not been started by the platform firmware.
> > 
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> If there are no more comments here, I'll tentatively queue it up for v4.4
> along with the [1-2/3] from this series.
> 
> Please let me know if there are any problems with that.

No problems from my POV.

Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Thanks.

-- 
Dmitry

Re: [Update][PATCH 3/3] input: i8042: Avoid resetting controller on system suspend/resume

[Rafael J. Wysocki]

On Monday, October 12, 2015 01:11:44 PM Dmitry Torokhov wrote:
> On Mon, Oct 12, 2015 at 10:17:05PM +0200, Rafael J. Wysocki wrote:
> > On Wednesday, October 07, 2015 03:03:57 AM Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > 
> > > If the upcoming system suspend is not going to be handled by the
> > > platform firmware, like in the suspend-to-idle case, it is not
> > > necessary to reset the controller in i8042_pm_suspend(), so avoid
> > > doing that.
> > > 
> > > Moreover, if the system resume currently in progress has not been
> > > started by the platform firmware, like in the suspend-to-idle case,
> > > i8042_controller_resume() need not be called by i8042_pm_resume(),
> > > so avoid doing that too in that case.
> > > 
> > > Additionally, try to catch the event that woke up the system by
> > > calling the interrupt handler early during system resume if it has
> > > not been started by the platform firmware.
> > > 
> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > If there are no more comments here, I'll tentatively queue it up for v4.4
> > along with the [1-2/3] from this series.
> > 
> > Please let me know if there are any problems with that.
> 
> No problems from my POV.
> 
> Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Thanks!