diff mbox

[Update,2x,1/2] ACPI / PM: Always enable wakeup GPEs when enabling device wakeup

Message ID 5962983.s3jy9r09Hm@vostro.rjw.lan (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Rafael J. Wysocki July 20, 2014, 11:51 p.m. UTC
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Subject: ACPI / PM: Always enable wakeup GPEs when enabling device wakeup

Wakeup GPEs are currently only enabled when setting up devices for
remote wakeup at run time.  During system-wide transitions they are
enabled by ACPICA at the very last stage of suspend (before asking
the BIOS to take over).  Of course, that only works for system
sleep states supported by ACPI, so in particular it doesn't work
for the "freeze" sleep state.

For this reason, modify the ACPI core device PM code to enable wakeup
GPEs for devices when setting them up for wakeup regardless of whether
that is remote wakeup at runtime or system wakeup.  That allows the
same device wakeup setup routine to be used for both runtime PM and
system-wide PM and makes it possible to reduce code size quite a bit.

That should make things like ACPI-based PCI Wake-on-LAN work with
the "freeze" sleep state among other things.

Tested-on: Toshiba Portege R500
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

The PCI ACPI device PM notify handler has to be updated to avoid running
runtime resume callbacks during system suspend too.

---
 drivers/acpi/device_pm.c |   83 ++++++++++++++++++++---------------------------
 drivers/pci/pci-acpi.c   |   15 +++++---
 include/acpi/acpi_bus.h  |   11 ------
 3 files changed, 47 insertions(+), 62 deletions(-)


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

Comments

Peter Zijlstra July 21, 2014, 8:17 a.m. UTC | #1
On Mon, Jul 21, 2014 at 01:51:46AM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Subject: ACPI / PM: Always enable wakeup GPEs when enabling device wakeup
> 
> Wakeup GPEs are currently only enabled when setting up devices for
> remote wakeup at run time.  During system-wide transitions they are
> enabled by ACPICA at the very last stage of suspend (before asking
> the BIOS to take over).  Of course, that only works for system
> sleep states supported by ACPI, so in particular it doesn't work
> for the "freeze" sleep state.
> 
> For this reason, modify the ACPI core device PM code to enable wakeup
> GPEs for devices when setting them up for wakeup regardless of whether
> that is remote wakeup at runtime or system wakeup.  That allows the
> same device wakeup setup routine to be used for both runtime PM and
> system-wide PM and makes it possible to reduce code size quite a bit.
> 
> That should make things like ACPI-based PCI Wake-on-LAN work with
> the "freeze" sleep state among other things.
> 
> Tested-on: Toshiba Portege R500
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> The PCI ACPI device PM notify handler has to be updated to avoid running
> runtime resume callbacks during system suspend too.

So I tested the first version, with that my WSM-EP didn't resume on WoL
and pressing the power button after the WoL had it crash and burn in the
igb driver.

Today I tested this latest version and WoL still didn't trigger a
resume, but the power button did make it go again, no crashes and I
suppose I can confirm the earlier patch that stopped making it go halt
works.

When I 'halt' I can wake the machine back up using a WoL so that all
_should_ work afaik.
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael J. Wysocki July 22, 2014, 1:02 a.m. UTC | #2
On Monday, July 21, 2014 10:17:34 AM Peter Zijlstra wrote:
> On Mon, Jul 21, 2014 at 01:51:46AM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > Subject: ACPI / PM: Always enable wakeup GPEs when enabling device wakeup
> > 
> > Wakeup GPEs are currently only enabled when setting up devices for
> > remote wakeup at run time.  During system-wide transitions they are
> > enabled by ACPICA at the very last stage of suspend (before asking
> > the BIOS to take over).  Of course, that only works for system
> > sleep states supported by ACPI, so in particular it doesn't work
> > for the "freeze" sleep state.
> > 
> > For this reason, modify the ACPI core device PM code to enable wakeup
> > GPEs for devices when setting them up for wakeup regardless of whether
> > that is remote wakeup at runtime or system wakeup.  That allows the
> > same device wakeup setup routine to be used for both runtime PM and
> > system-wide PM and makes it possible to reduce code size quite a bit.
> > 
> > That should make things like ACPI-based PCI Wake-on-LAN work with
> > the "freeze" sleep state among other things.
> > 
> > Tested-on: Toshiba Portege R500
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> > 
> > The PCI ACPI device PM notify handler has to be updated to avoid running
> > runtime resume callbacks during system suspend too.
> 
> So I tested the first version, with that my WSM-EP didn't resume on WoL
> and pressing the power button after the WoL had it crash and burn in the
> igb driver.
> 
> Today I tested this latest version and WoL still didn't trigger a
> resume, but the power button did make it go again, no crashes and I
> suppose I can confirm the earlier patch that stopped making it go halt
> works.

OK, thanks!

> When I 'halt' I can wake the machine back up using a WoL so that all
> _should_ work afaik.

Yes, it should.

I'll send an updated patchset shortly, so please test that one.  If it
doesn't help, we'll need to dig deeper still.

Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

Index: linux-pm/drivers/acpi/device_pm.c
===================================================================
--- linux-pm.orig/drivers/acpi/device_pm.c
+++ linux-pm/drivers/acpi/device_pm.c
@@ -600,26 +600,10 @@  int acpi_pm_device_sleep_state(struct de
 }
 EXPORT_SYMBOL(acpi_pm_device_sleep_state);
 
-#ifdef CONFIG_PM_RUNTIME
-/**
- * acpi_wakeup_device - Wakeup notification handler for ACPI devices.
- * @handle: ACPI handle of the device the notification is for.
- * @event: Type of the signaled event.
- * @context: Device corresponding to @handle.
- */
-static void acpi_wakeup_device(acpi_handle handle, u32 event, void *context)
-{
-	struct device *dev = context;
-
-	if (event == ACPI_NOTIFY_DEVICE_WAKE && dev) {
-		pm_wakeup_event(dev, 0);
-		pm_runtime_resume(dev);
-	}
-}
-
 /**
- * __acpi_device_run_wake - Enable/disable runtime remote wakeup for device.
- * @adev: ACPI device to enable/disable the remote wakeup for.
+ * acpi_device_wakeup - Enable/disable wakeup functionality for device.
+ * @adev: ACPI device to enable/disable wakeup functionality for.
+ * @target_state: State the system is transitioning into.
  * @enable: Whether to enable or disable the wakeup functionality.
  *
  * Enable/disable the GPE associated with @adev so that it can generate
@@ -629,7 +613,8 @@  static void acpi_wakeup_device(acpi_hand
  * Callers must ensure that @adev is a valid ACPI device node before executing
  * this function.
  */
-int __acpi_device_run_wake(struct acpi_device *adev, bool enable)
+static int acpi_device_wakeup(struct acpi_device *adev, u32 target_state,
+			      bool enable)
 {
 	struct acpi_device_wakeup *wakeup = &adev->wakeup;
 
@@ -637,7 +622,7 @@  int __acpi_device_run_wake(struct acpi_d
 		acpi_status res;
 		int error;
 
-		error = acpi_enable_wakeup_device_power(adev, ACPI_STATE_S0);
+		error = acpi_enable_wakeup_device_power(adev, target_state);
 		if (error)
 			return error;
 
@@ -653,6 +638,27 @@  int __acpi_device_run_wake(struct acpi_d
 	return 0;
 }
 
+#ifdef CONFIG_PM_RUNTIME
+/**
+ * acpi_wakeup_device - Wakeup notification handler for ACPI devices.
+ * @handle: ACPI handle of the device the notification is for.
+ * @event: Type of the signaled event.
+ * @context: Device corresponding to @handle.
+ */
+static void acpi_wakeup_device(acpi_handle handle, u32 event, void *context)
+{
+	struct device *dev = context;
+
+	if (event == ACPI_NOTIFY_DEVICE_WAKE && dev) {
+		pm_wakeup_event(dev, 0);
+		/*
+		 * Use the PM workqueue to avoid running the runtime resume
+		 * callback during system suspend which may not be appropriate.
+		 */
+		pm_request_resume(dev);
+	}
+}
+
 /**
  * acpi_pm_device_run_wake - Enable/disable remote wakeup for given device.
  * @dev: Device to enable/disable the platform to wake up.
@@ -673,7 +679,7 @@  int acpi_pm_device_run_wake(struct devic
 		return -ENODEV;
 	}
 
-	return __acpi_device_run_wake(adev, enable);
+	return acpi_device_wakeup(adev, enable, ACPI_STATE_S0);
 }
 EXPORT_SYMBOL(acpi_pm_device_run_wake);
 #else
@@ -683,20 +689,6 @@  static inline void acpi_wakeup_device(ac
 
 #ifdef CONFIG_PM_SLEEP
 /**
- * __acpi_device_sleep_wake - Enable or disable device to wake up the system.
- * @dev: Device to enable/desible to wake up the system.
- * @target_state: System state the device is supposed to wake up from.
- * @enable: Whether to enable or disable @dev to wake up the system.
- */
-int __acpi_device_sleep_wake(struct acpi_device *adev, u32 target_state,
-			     bool enable)
-{
-	return enable ?
-		acpi_enable_wakeup_device_power(adev, target_state) :
-		acpi_disable_wakeup_device_power(adev);
-}
-
-/**
  * acpi_pm_device_sleep_wake - Enable or disable device to wake up the system.
  * @dev: Device to enable/desible to wake up the system from sleep states.
  * @enable: Whether to enable or disable @dev to wake up the system.
@@ -716,8 +708,7 @@  int acpi_pm_device_sleep_wake(struct dev
 		return -ENODEV;
 	}
 
-	error = __acpi_device_sleep_wake(adev, acpi_target_system_state(),
-					 enable);
+	error = acpi_device_wakeup(adev, acpi_target_system_state(), enable);
 	if (!error)
 		dev_info(dev, "System wakeup %s by ACPI\n",
 				enable ? "enabled" : "disabled");
@@ -775,13 +766,13 @@  int acpi_dev_runtime_suspend(struct devi
 
 	remote_wakeup = dev_pm_qos_flags(dev, PM_QOS_FLAG_REMOTE_WAKEUP) >
 				PM_QOS_FLAGS_NONE;
-	error = __acpi_device_run_wake(adev, remote_wakeup);
+	error = acpi_device_wakeup(adev, ACPI_STATE_S0, remote_wakeup);
 	if (remote_wakeup && error)
 		return -EAGAIN;
 
 	error = acpi_dev_pm_low_power(dev, adev, ACPI_STATE_S0);
 	if (error)
-		__acpi_device_run_wake(adev, false);
+		acpi_device_wakeup(adev, ACPI_STATE_S0, false);
 
 	return error;
 }
@@ -804,7 +795,7 @@  int acpi_dev_runtime_resume(struct devic
 		return 0;
 
 	error = acpi_dev_pm_full_power(adev);
-	__acpi_device_run_wake(adev, false);
+	acpi_device_wakeup(adev, ACPI_STATE_S0, false);
 	return error;
 }
 EXPORT_SYMBOL_GPL(acpi_dev_runtime_resume);
@@ -860,13 +851,13 @@  int acpi_dev_suspend_late(struct device
 
 	target_state = acpi_target_system_state();
 	wakeup = device_may_wakeup(dev);
-	error = __acpi_device_sleep_wake(adev, target_state, wakeup);
+	error = acpi_device_wakeup(adev, target_state, wakeup);
 	if (wakeup && error)
 		return error;
 
 	error = acpi_dev_pm_low_power(dev, adev, target_state);
 	if (error)
-		__acpi_device_sleep_wake(adev, ACPI_STATE_UNKNOWN, false);
+		acpi_device_wakeup(adev, ACPI_STATE_UNKNOWN, false);
 
 	return error;
 }
@@ -889,7 +880,7 @@  int acpi_dev_resume_early(struct device
 		return 0;
 
 	error = acpi_dev_pm_full_power(adev);
-	__acpi_device_sleep_wake(adev, ACPI_STATE_UNKNOWN, false);
+	acpi_device_wakeup(adev, ACPI_STATE_UNKNOWN, false);
 	return error;
 }
 EXPORT_SYMBOL_GPL(acpi_dev_resume_early);
@@ -1052,7 +1043,7 @@  int acpi_dev_pm_attach(struct device *de
 	dev->pm_domain = &acpi_general_pm_domain;
 	if (power_on) {
 		acpi_dev_pm_full_power(adev);
-		__acpi_device_run_wake(adev, false);
+		acpi_device_wakeup(adev, ACPI_STATE_S0, false);
 	}
 	return 0;
 }
@@ -1086,7 +1077,7 @@  void acpi_dev_pm_detach(struct device *d
 			 */
 			dev_pm_qos_hide_latency_limit(dev);
 			dev_pm_qos_hide_flags(dev);
-			__acpi_device_run_wake(adev, false);
+			acpi_device_wakeup(adev, ACPI_STATE_S0, false);
 			acpi_dev_pm_low_power(dev, adev, ACPI_STATE_S0);
 		}
 	}
Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -532,13 +532,8 @@  static inline int acpi_pm_device_sleep_s
 #endif
 
 #ifdef CONFIG_PM_RUNTIME
-int __acpi_device_run_wake(struct acpi_device *, bool);
 int acpi_pm_device_run_wake(struct device *, bool);
 #else
-static inline int __acpi_device_run_wake(struct acpi_device *adev, bool en)
-{
-	return -ENODEV;
-}
 static inline int acpi_pm_device_run_wake(struct device *dev, bool enable)
 {
 	return -ENODEV;
@@ -546,14 +541,8 @@  static inline int acpi_pm_device_run_wak
 #endif
 
 #ifdef CONFIG_PM_SLEEP
-int __acpi_device_sleep_wake(struct acpi_device *, u32, bool);
 int acpi_pm_device_sleep_wake(struct device *, bool);
 #else
-static inline int __acpi_device_sleep_wake(struct acpi_device *adev,
-					   u32 target_state, bool enable)
-{
-	return -ENODEV;
-}
 static inline int acpi_pm_device_sleep_wake(struct device *dev, bool enable)
 {
 	return -ENODEV;
Index: linux-pm/drivers/pci/pci-acpi.c
===================================================================
--- linux-pm.orig/drivers/pci/pci-acpi.c
+++ linux-pm/drivers/pci/pci-acpi.c
@@ -47,9 +47,15 @@  static void pci_acpi_wake_dev(acpi_handl
 	if (pci_dev->pme_poll)
 		pci_dev->pme_poll = false;
 
+	pci_wakeup_event(pci_dev);
+	/*
+	 * Use the PM workqueue for resuming devices to avoid running runtime
+	 * resume callbacks during system suspend which may not be appropriate.
+	 * Bridges are an exception, because they are never suspended, so it is
+	 * not necessary to resume them here.
+	 */
 	if (pci_dev->current_state == PCI_D3cold) {
-		pci_wakeup_event(pci_dev);
-		pm_runtime_resume(&pci_dev->dev);
+		pm_request_resume(&pci_dev->dev);
 		return;
 	}
 
@@ -57,11 +63,10 @@  static void pci_acpi_wake_dev(acpi_handl
 	if (pci_dev->pme_support)
 		pci_check_pme_status(pci_dev);
 
-	pci_wakeup_event(pci_dev);
-	pm_runtime_resume(&pci_dev->dev);
-
 	if (pci_dev->subordinate)
 		pci_pme_wakeup_bus(pci_dev->subordinate);
+	else
+		pm_request_resume(&pci_dev->dev);
 }
 
 /**