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

[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!