[patch 2.6.25-rc6 0/7] misc pm wake patches

[patch 2.6.25-rc6 0/7] misc pm wake patches

[David Brownell]

Here are patches I've had sitting around for a while ... many of
them are IMO now mergeable, but I think the last two need work
to integrate better with ACPI and PCI.

 - Crosslink ACPI and matching "real" device nodes in sysfs.
   Updated version of a patch from early last year.

 - acpi_pm_device_sleep_state() cleanup

 - pci_choose_state() cleanup and fixes (but arguably this
   needs to change given the new method scheme)

 - USB uses pci_choose_state(), seems safe given those fixes

 - Make ACPI set up device.power.can_wakeup driver model flags

 - Make ACPI use device_may_wakeup(dev) policy inputs from
   userspace, instead of /proc/acpi/wakeup ... probably needs
   work yet given the method reordering stuff

 - Make PCI set up device.power.can_wakeup flags ... presumably
   this still breaks on PPC, which needs more work.

[patch 2.6.25-rc6 1/7] crosslink ACPI and "real" device nodes

[David Brownell]

From: David Brownell <dbrownell@users.sourceforge.net>

Add cross-links between ACPI device and "real" devices in sysfs,
exposing otherwise-hidden interrelationships between the various
device nodes which ACPI creates.  As a representative example one
hardware device is exposed as two logical devices (PNP and ACPI):

  .../pnp0/00:06/
  .../LNXSYSTM:00/device:00/PNP0A03:00/device:15/PNP0B00:00/

The PNP device gets a "firmware_node" link pointing to the ACPI device.
The ACPI device has a "physical_node" link pointing to the PNP device.
Linux drivers currently bind only to the "physical" device nodes.

Other device trees provided by firmware tables could be exported
using the same scheme; these names aren't ACPI-specific.

(Based on a patch from Zhang Rui.  This version is modified to not
depend on the patch teaching ACPI about driver model wakeup flags.)

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
Cc: Zhang Rui <rui.zhang@intel.com> 
---
Change from previous version:  if creating the link fails, a
pr_debug level message is emitted.

 drivers/acpi/glue.c |   27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

--- g26.orig/drivers/acpi/glue.c	2008-03-08 11:11:18.000000000 -0800
+++ g26/drivers/acpi/glue.c	2008-03-08 11:29:07.000000000 -0800
@@ -142,6 +142,7 @@ EXPORT_SYMBOL(acpi_get_physical_device);
 
 static int acpi_bind_one(struct device *dev, acpi_handle handle)
 {
+	struct acpi_device *acpi_dev;
 	acpi_status status;
 
 	if (dev->archdata.acpi_handle) {
@@ -157,6 +158,23 @@ static int acpi_bind_one(struct device *
 	}
 	dev->archdata.acpi_handle = handle;
 
+	status = acpi_bus_get_device(handle, &acpi_dev);
+	if (!ACPI_FAILURE(status)) {
+		int ret;
+
+		ret = sysfs_create_link(&dev->kobj, &acpi_dev->dev.kobj,
+				"firmware_node");
+		if (ret != 0)
+			pr_debug(PREFIX "Can't create %s link for %s\n",
+				"firmware_node", dev->bus_id);
+
+		ret = sysfs_create_link(&acpi_dev->dev.kobj, &dev->kobj,
+				"physical_node");
+		if (ret != 0)
+			pr_debug(PREFIX "Can't create %s link for %s\n",
+				"physical_node", acpi_dev->dev.bus_id);
+	}
+
 	return 0;
 }
 
@@ -165,8 +183,17 @@ static int acpi_unbind_one(struct device
 	if (!dev->archdata.acpi_handle)
 		return 0;
 	if (dev == acpi_get_physical_device(dev->archdata.acpi_handle)) {
+		struct acpi_device *acpi_dev;
+
 		/* acpi_get_physical_device increase refcnt by one */
 		put_device(dev);
+
+		if (!acpi_bus_get_device(dev->archdata.acpi_handle,
+					&acpi_dev)) {
+			sysfs_remove_link(&dev->kobj, "firmware_node");
+			sysfs_remove_link(&acpi_dev->dev.kobj, "physical_node");
+		}
+
 		acpi_detach_data(dev->archdata.acpi_handle,
 				 acpi_glue_data_handler);
 		dev->archdata.acpi_handle = NULL;

[patch 2.6.25-rc6 2/7] acpi_pm_device_sleep_state() cleanup

[David Brownell]

Get rid of a superfluous acpi_pm_device_sleep_state() parameter.  The
only legitimate value of that parameter must be derived from the first
parameter, which is what all the callers already do.  (However, this
does not address the fact that ACPI still doesn't set up those flags.)

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
 drivers/acpi/sleep/main.c  |    8 ++++----
 drivers/pci/pci-acpi.c     |    3 +--
 drivers/pnp/pnpacpi/core.c |    4 +---
 include/acpi/acpi_bus.h    |    4 ++--
 4 files changed, 8 insertions(+), 11 deletions(-)

--- g26.orig/drivers/acpi/sleep/main.c	2008-02-24 02:07:56.000000000 -0800
+++ g26/drivers/acpi/sleep/main.c	2008-02-24 02:07:57.000000000 -0800
@@ -412,8 +412,8 @@ int acpi_suspend(u32 acpi_state)
 /**
  *	acpi_pm_device_sleep_state - return preferred power state of ACPI device
  *		in the system sleep state given by %acpi_target_sleep_state
- *	@dev: device to examine
- *	@wake: if set, the device should be able to wake up the system
+ *	@dev: device to examine; its driver model wakeup flags control
+ *		whether it should be able to wake up the system
  *	@d_min_p: used to store the upper limit of allowed states range
  *	Return value: preferred power state of the device on success, -ENODEV on
  *		failure (ie. if there's no 'struct acpi_device' for @dev)
@@ -431,7 +431,7 @@ int acpi_suspend(u32 acpi_state)
  *	via @wake.
  */
 
-int acpi_pm_device_sleep_state(struct device *dev, int wake, int *d_min_p)
+int acpi_pm_device_sleep_state(struct device *dev, int *d_min_p)
 {
 	acpi_handle handle = DEVICE_ACPI_HANDLE(dev);
 	struct acpi_device *adev;
@@ -470,7 +470,7 @@ int acpi_pm_device_sleep_state(struct de
 	 * can wake the system.  _S0W may be valid, too.
 	 */
 	if (acpi_target_sleep_state == ACPI_STATE_S0 ||
-	    (wake && adev->wakeup.state.enabled &&
+	    (device_may_wakeup(dev) && adev->wakeup.state.enabled &&
 	     adev->wakeup.sleep_state <= acpi_target_sleep_state)) {
 		acpi_status status;
 
--- g26.orig/drivers/pci/pci-acpi.c	2008-02-24 02:07:56.000000000 -0800
+++ g26/drivers/pci/pci-acpi.c	2008-02-24 02:07:57.000000000 -0800
@@ -249,8 +249,7 @@ static pci_power_t acpi_pci_choose_state
 {
 	int acpi_state;
 
-	acpi_state = acpi_pm_device_sleep_state(&pdev->dev,
-		device_may_wakeup(&pdev->dev), NULL);
+	acpi_state = acpi_pm_device_sleep_state(&pdev->dev, NULL);
 	if (acpi_state < 0)
 		return PCI_POWER_ERROR;
 
--- g26.orig/drivers/pnp/pnpacpi/core.c	2008-02-24 02:07:56.000000000 -0800
+++ g26/drivers/pnp/pnpacpi/core.c	2008-02-24 02:07:57.000000000 -0800
@@ -132,9 +132,7 @@ static int pnpacpi_suspend(struct pnp_de
 {
 	int power_state;
 
-	power_state = acpi_pm_device_sleep_state(&dev->dev,
-						device_may_wakeup(&dev->dev),
-						NULL);
+	power_state = acpi_pm_device_sleep_state(&dev->dev, NULL);
 	if (power_state < 0)
 		power_state = (state.event == PM_EVENT_ON) ?
 				ACPI_STATE_D0 : ACPI_STATE_D3;
--- g26.orig/include/acpi/acpi_bus.h	2008-02-24 02:07:56.000000000 -0800
+++ g26/include/acpi/acpi_bus.h	2008-02-24 02:07:57.000000000 -0800
@@ -376,9 +376,9 @@ acpi_handle acpi_get_pci_rootbridge_hand
 #define DEVICE_ACPI_HANDLE(dev) ((acpi_handle)((dev)->archdata.acpi_handle))
 
 #ifdef CONFIG_PM_SLEEP
-int acpi_pm_device_sleep_state(struct device *, int, int *);
+int acpi_pm_device_sleep_state(struct device *, int *);
 #else /* !CONFIG_PM_SLEEP */
-static inline int acpi_pm_device_sleep_state(struct device *d, int w, int *p)
+static inline int acpi_pm_device_sleep_state(struct device *d, int *p)
 {
 	if (p)
 		*p = ACPI_STATE_D0;

[patch 2.6.25-rc6 3/7] pci_choose_state() cleanup and fixes

[David Brownell]

Clean up pci_choose_state():

 - pci_choose_state() should only return PCI_D0, unless the system is
   entering a suspend (or hibernate) system state.

 - Only use platform_pci_choose_state() when entering a suspend
   state ... and avoid PCI_D1 and PCI_D2 when appropriate.

 - Corrrect kerneldoc.

Note that for now only ACPI provides platform_pci_choose_state(), so
this could be a minor change in behavior on some non-PC systems:  it
avoids D3 except in the final stage of hibernation.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
 drivers/pci/pci.c |   47 ++++++++++++++++++++++++++---------------------
 1 file changed, 26 insertions(+), 21 deletions(-)

--- g26.orig/drivers/pci/pci.c	2008-02-24 00:18:16.000000000 -0800
+++ g26/drivers/pci/pci.c	2008-02-24 00:41:18.000000000 -0800
@@ -523,44 +523,49 @@ pci_set_power_state(struct pci_dev *dev,
 }
 
 pci_power_t (*platform_pci_choose_state)(struct pci_dev *dev, pm_message_t state);
- 
+
 /**
  * pci_choose_state - Choose the power state of a PCI device
  * @dev: PCI device to be suspended
- * @state: target sleep state for the whole system. This is the value
- *	that is passed to suspend() function.
+ * @mesg: value passed to suspend() function.
  *
  * Returns PCI power state suitable for given device and given system
- * message.
+ * power state transition.
  */
 
-pci_power_t pci_choose_state(struct pci_dev *dev, pm_message_t state)
+pci_power_t pci_choose_state(struct pci_dev *dev, pm_message_t mesg)
 {
 	pci_power_t ret;
 
+	/* PCI legacy PM? */
 	if (!pci_find_capability(dev, PCI_CAP_ID_PM))
 		return PCI_D0;
 
-	if (platform_pci_choose_state) {
-		ret = platform_pci_choose_state(dev, state);
-		if (ret != PCI_POWER_ERROR)
-			return ret;
-	}
-
-	switch (state.event) {
-	case PM_EVENT_ON:
-		return PCI_D0;
-	case PM_EVENT_FREEZE:
-	case PM_EVENT_PRETHAW:
-		/* REVISIT both freeze and pre-thaw "should" use D0 */
+	switch (mesg.event) {
 	case PM_EVENT_SUSPEND:
+		/* NOTE:  platform_pci_choose_state() should only return
+		 * states where wakeup won't work if
+		 *   - !device_may_wakeup(&dev->dev), or
+		 *   - dev can't wake from the target system state
+		 */
+		if (platform_pci_choose_state) {
+			ret = platform_pci_choose_state(dev, mesg);
+			if (ret == PCI_POWER_ERROR)
+				ret = PCI_D3hot;
+			else if ((ret == PCI_D1 || ret == PCI_D2)
+					&& pci_no_d1d2(dev))
+				ret = PCI_D3hot;
+			break;
+		}
+		/* FALLTHROUGH ... D3hot works, but may be suboptimal */
 	case PM_EVENT_HIBERNATE:
-		return PCI_D3hot;
+		ret = PCI_D3hot;
+		break;
 	default:
-		printk("Unrecognized suspend event %d\n", state.event);
-		BUG();
+		ret = PCI_D0;
+		break;
 	}
-	return PCI_D0;
+	return ret;
 }
 
 EXPORT_SYMBOL(pci_choose_state);

[patch 2.6.25-rc6 4/7] USB uses pci_choose_state()

[David Brownell]

Given a preceding patch that makes pci_choose_state() behave sanely,
use it to select the appropriate PCI_Dx state to use when suspending
a host controller.

USB hosts may now use states like PCI_D1 and PCI_D2, where appropriate.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
 drivers/usb/core/hcd-pci.c |   25 ++++++++-----------------
 1 file changed, 8 insertions(+), 17 deletions(-)

--- g26.orig/drivers/usb/core/hcd-pci.c	2008-02-24 02:02:45.000000000 -0800
+++ g26/drivers/usb/core/hcd-pci.c	2008-02-24 02:08:08.000000000 -0800
@@ -224,18 +224,6 @@ int usb_hcd_pci_suspend(struct pci_dev *
 	}
 	synchronize_irq(dev->irq);
 
-	/* FIXME until the generic PM interfaces change a lot more, this
-	 * can't use PCI D1 and D2 states.  For example, the confusion
-	 * between messages and states will need to vanish, and messages
-	 * will need to provide a target system state again.
-	 *
-	 * It'll be important to learn characteristics of the target state,
-	 * especially on embedded hardware where the HCD will often be in
-	 * charge of an external VBUS power supply and one or more clocks.
-	 * Some target system states will leave them active; others won't.
-	 * (With PCI, that's often handled by platform BIOS code.)
-	 */
-
 	/* even when the PCI layer rejects some of the PCI calls
 	 * below, HCs can try global suspend and reduce DMA traffic.
 	 * PM-sensitive HCDs may already have done this.
@@ -248,6 +236,7 @@ int usb_hcd_pci_suspend(struct pci_dev *
 	 * low power state, if the hardware allows.
 	 */
 	if (hcd->state == HC_STATE_SUSPENDED) {
+		pci_power_t		d_state;
 
 		/* no DMA or IRQs except when HC is active */
 		if (dev->current_state == PCI_D0) {
@@ -265,27 +254,29 @@ int usb_hcd_pci_suspend(struct pci_dev *
 			dev_dbg(hcd->self.controller, "--> PCI D0/legacy\n");
 			goto done;
 		}
+		d_state = pci_choose_state(dev, message);
 
-		/* NOTE:  dev->current_state becomes nonzero only here, and
-		 * only for devices that support PCI PM.  Also, exiting
+		/* NOTE:  dev->current_state becomes nonzero only here, using
+		 * D3hot unless the platform chooses D2 or D1.  Also, exiting
 		 * PCI_D3 (but not PCI_D1 or PCI_D2) is allowed to reset
 		 * some device state (e.g. as part of clock reinit).
 		 */
-		retval = pci_set_power_state(dev, PCI_D3hot);
+		retval = pci_set_power_state(dev, d_state);
 		suspend_report_result(pci_set_power_state, retval);
 		if (retval == 0) {
 			int wake = device_can_wakeup(&hcd->self.root_hub->dev);
 
 			wake = wake && device_may_wakeup(hcd->self.controller);
 
-			dev_dbg(hcd->self.controller, "--> PCI D3%s\n",
+			dev_dbg(hcd->self.controller, "--> PCI D%d%s\n",
+					d_state,
 					wake ? "/wakeup" : "");
 
 			/* Ignore these return values.  We rely on pci code to
 			 * reject requests the hardware can't implement, rather
 			 * than coding the same thing.
 			 */
-			(void) pci_enable_wake(dev, PCI_D3hot, wake);
+			(void) pci_enable_wake(dev, d_state, wake);
 			(void) pci_enable_wake(dev, PCI_D3cold, wake);
 		} else {
 			dev_dbg(&dev->dev, "PCI D3 suspend fail, %d\n",

[patch 2.6.25-rc6 5/7] ACPI sets up device.power.can_wakeup flags

[David Brownell]

This exports the "acpi_device.wakeup.flags.valid" flag to the driver
model as "device.power.can_wakeup".  That is, each "ACPI device" in
the /proc/acpi/wakeup file that corresponds to a "real" device will
initialize the can_wakeup flag to true.

A separate patch is needed to make ACPI pay attention to the userspace
input:  devices that can_wakeup have a should_wakeup flag, initially
true, which is settable via sysfs and tested using device_may_wakeup().
That's intended to be the primary policy input to the system, not the
ACPI-specific /proc/acpi/wakeup file.

Note also that the ACPI tables list only motherboard devices, but many
systems include more peripherals than those.  While the USB framework
handles USB peripherals, nothing in Linux currently sets those flags
for wakeup-capable devices on other expansion busses (like PCI).

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
 drivers/acpi/button.c |    3 +++
 drivers/acpi/glue.c   |    1 +
 drivers/acpi/power.c  |    4 ++++
 3 files changed, 8 insertions(+)

--- g26.orig/drivers/acpi/button.c	2008-03-20 03:02:45.000000000 -0700
+++ g26/drivers/acpi/button.c	2008-03-20 03:27:24.000000000 -0700
@@ -476,6 +476,9 @@ static int acpi_button_add(struct acpi_d
 	if (button->type == ACPI_BUTTON_TYPE_LID)
 		acpi_lid_send_state(button);
 
+	/* for these devices the ACPI node *IS* the "physical" device */
+	device_init_wakeup(&device->dev, device->wakeup.flags.valid);
+
 	if (device->wakeup.flags.valid) {
 		/* Button's GPE is run-wake GPE */
 		acpi_set_gpe_type(device->wakeup.gpe_device,
--- g26.orig/drivers/acpi/glue.c	2008-03-20 03:27:11.000000000 -0700
+++ g26/drivers/acpi/glue.c	2008-03-20 03:27:25.000000000 -0700
@@ -162,6 +162,7 @@ static int acpi_bind_one(struct device *
 	if (!ACPI_FAILURE(status)) {
 		int ret;
 
+		device_init_wakeup(dev, acpi_dev->wakeup.flags.valid);
 		ret = sysfs_create_link(&dev->kobj, &acpi_dev->dev.kobj,
 				"firmware_node");
 		if (ret != 0)
--- g26.orig/drivers/acpi/power.c	2008-03-20 03:02:45.000000000 -0700
+++ g26/drivers/acpi/power.c	2008-03-20 03:27:25.000000000 -0700
@@ -313,6 +313,7 @@ int acpi_enable_wakeup_device_power(stru
 		ret = acpi_power_on(dev->wakeup.resources.handles[i], dev);
 		if (ret) {
 			printk(KERN_ERR PREFIX "Transition power state\n");
+			device_init_wakeup(&dev->dev, 0);
 			dev->wakeup.flags.valid = 0;
 			return -1;
 		}
@@ -322,6 +323,7 @@ int acpi_enable_wakeup_device_power(stru
 	status = acpi_evaluate_object(dev->handle, "_PSW", &arg_list, NULL);
 	if (ACPI_FAILURE(status) && (status != AE_NOT_FOUND)) {
 		printk(KERN_ERR PREFIX "Evaluate _PSW\n");
+		device_init_wakeup(&dev->dev, 0);
 		dev->wakeup.flags.valid = 0;
 		ret = -1;
 	}
@@ -351,6 +353,7 @@ int acpi_disable_wakeup_device_power(str
 	status = acpi_evaluate_object(dev->handle, "_PSW", &arg_list, NULL);
 	if (ACPI_FAILURE(status) && (status != AE_NOT_FOUND)) {
 		printk(KERN_ERR PREFIX "Evaluate _PSW\n");
+		device_init_wakeup(&dev->dev, 0);
 		dev->wakeup.flags.valid = 0;
 		return -1;
 	}
@@ -360,6 +363,7 @@ int acpi_disable_wakeup_device_power(str
 		ret = acpi_power_off_device(dev->wakeup.resources.handles[i], dev);
 		if (ret) {
 			printk(KERN_ERR PREFIX "Transition power state\n");
+			device_init_wakeup(&dev->dev, 0);
 			dev->wakeup.flags.valid = 0;
 			return -1;
 		}

[patch 2.6.25-rc6 6/7] ACPI uses device_may_wakeup() policy inputs

[David Brownell]

This imports the driver model device.power.may_wakeup flags to ACPI,
using it to *REPLACE* the /proc/acpi/wakeup flags for some devices.
It depends on the previous patch making device.power.can_wakeup behave.
It does that by:

 - Implementing platform_enable_wakeup(), which is currently invoked only
   by pci_enable_wake().  When that's called -- probably in the driver
   suspend() call -- it updates acpi_device.wakeup.state.enabled flag in
   the same way writing to /proc/acpi/wakeup updates it.
   
 - Updating the usage of the corresponding ACPI flags when turning on
   wakeup power domains and GPEs.

THIS PATCH NEEDS MORE ATTENTION because of the way the ACPI method
invocations have been changing, e.g. the 1.0 vs 2.0 sequencing.

Right now it's not clear to me whether the GPEs are always enabled at
the right time, and for that matter whether the rules haven't changed
so that drivers can no longer effectively control those settings from
suspend() unless acpi_new_pts_ordering is in effect...

---
 drivers/acpi/sleep/main.c   |    2 +-
 drivers/acpi/sleep/wakeup.c |   44 ++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 43 insertions(+), 3 deletions(-)

--- g26.orig/drivers/acpi/sleep/main.c	2008-02-24 02:07:57.000000000 -0800
+++ g26/drivers/acpi/sleep/main.c	2008-02-24 02:08:04.000000000 -0800
@@ -470,7 +470,7 @@ int acpi_pm_device_sleep_state(struct de
 	 * can wake the system.  _S0W may be valid, too.
 	 */
 	if (acpi_target_sleep_state == ACPI_STATE_S0 ||
-	    (device_may_wakeup(dev) && adev->wakeup.state.enabled &&
+	    (device_may_wakeup(dev) &&
 	     adev->wakeup.sleep_state <= acpi_target_sleep_state)) {
 		acpi_status status;
 
--- g26.orig/drivers/acpi/sleep/wakeup.c	2008-02-24 02:03:27.000000000 -0800
+++ g26/drivers/acpi/sleep/wakeup.c	2008-02-24 02:08:04.000000000 -0800
@@ -35,12 +35,22 @@ void acpi_enable_wakeup_device_prep(u8 s
 		struct acpi_device *dev = container_of(node,
 						       struct acpi_device,
 						       wakeup_list);
+		struct device *ldev;
 
 		if (!dev->wakeup.flags.valid ||
 		    !dev->wakeup.state.enabled ||
 		    (sleep_state > (u32) dev->wakeup.sleep_state))
 			continue;
 
+		ldev = acpi_get_physical_device(dev->handle);
+		if (ldev) {
+			int flag = device_may_wakeup(ldev);
+
+			put_device(ldev);
+			if (!flag)
+				continue;
+		}
+
 		spin_unlock(&acpi_device_lock);
 		acpi_enable_wakeup_device_power(dev);
 		spin_lock(&acpi_device_lock);
@@ -57,8 +67,8 @@ void acpi_enable_wakeup_device(u8 sleep_
 {
 	struct list_head *node, *next;
 
-	/* 
-	 * Caution: this routine must be invoked when interrupt is disabled 
+	/*
+	 * Caution: this routine must be invoked when interrupt is disabled
 	 * Refer ACPI2.0: P212
 	 */
 	ACPI_FUNCTION_TRACE("acpi_enable_wakeup_device");
@@ -107,6 +117,7 @@ void acpi_disable_wakeup_device(u8 sleep
 	list_for_each_safe(node, next, &acpi_wakeup_device_list) {
 		struct acpi_device *dev =
 			container_of(node, struct acpi_device, wakeup_list);
+		struct device *ldev;
 
 		if (!dev->wakeup.flags.valid)
 			continue;
@@ -125,6 +136,15 @@ void acpi_disable_wakeup_device(u8 sleep
 			continue;
 		}
 
+		ldev = acpi_get_physical_device(dev->handle);
+		if (ldev) {
+			int flag = device_may_wakeup(ldev);
+
+			put_device(ldev);
+			if (!flag)
+				continue;
+		}
+
 		spin_unlock(&acpi_device_lock);
 		acpi_disable_wakeup_device_power(dev);
 		/* Never disable run-wake GPE */
@@ -139,6 +159,24 @@ void acpi_disable_wakeup_device(u8 sleep
 	spin_unlock(&acpi_device_lock);
 }
 
+static int acpi_platform_enable_wakeup(struct device *dev, int is_on)
+{
+	struct acpi_device	*adev;
+	int			status;
+
+	if (!device_can_wakeup(dev))
+		return -EINVAL;
+	if (is_on && !device_may_wakeup(dev))
+		return -EINVAL;
+
+	status = acpi_bus_get_device(DEVICE_ACPI_HANDLE(dev), &adev);
+	if (status < 0)
+		return status;
+
+	adev->wakeup.state.enabled = !!is_on;
+	return 0;
+}
+
 static int __init acpi_wakeup_device_init(void)
 {
 	struct list_head *node, *next;
@@ -146,6 +184,8 @@ static int __init acpi_wakeup_device_ini
 	if (acpi_disabled)
 		return 0;
 
+	platform_enable_wakeup = acpi_platform_enable_wakeup;
+
 	spin_lock(&acpi_device_lock);
 	list_for_each_safe(node, next, &acpi_wakeup_device_list) {
 		struct acpi_device *dev = container_of(node,

[patch 2.6.25-rc6 7/7] PCI set up device.power.can_wakeup flags

[David Brownell]

This patch teaches "pci_dev" about the driver model wakeup support, by
marking devices as supporting wakeup when the PME# capability is listed
in a PCI PM capability.  Right now, few PCI drivers support wakeup events:
only USB hosts, and various network drivers.

A previous version of this patch broke on PowerPC platforms after they
changed how they do early PCI initialization (following the first version
of this patch).  Potentially the real fix involves switching PCI init on
all platforms so they all adopt the driver model "init() then add()" model,
calling device_initialize() early and pci_setup_device() before device_add().

Also, note that ACPI has its own notions of what devices are wakeup-capable.
Assuming that the PCI PM# capability is queried before calling device_add()
to initialize driver model wakeup flags, ACPI overrides those settings for
devices with entries in ACPI tables in its platform_notify().

That's how ACPI kicks in legacy PCI PM (e.g. Intel's UHCI controllers
don't use PME#), declares that _it_ will handle SMBUSALERT# thank you, and
handles various other board quirks.

The fact that ACPI flags some bridges as wakeup-capable is puzzling though.
I'm guessing it relates to whether PME# (or WAKE# on PCIE, etc) is actually
wired up for use by add-on cards, and this marks an area where ACPI doesn't
yet work correctly (it doesn't handle such wakeups).


NOT YET READY FOR PRIME TIME ... expected to still break PowerPC.
---
 drivers/pci/probe.c |   16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

--- g26.orig/drivers/pci/probe.c	2008-02-24 00:18:23.000000000 -0800
+++ g26/drivers/pci/probe.c	2008-02-24 01:02:30.000000000 -0800
@@ -678,6 +678,7 @@ static void pci_read_irq(struct pci_dev 
 static int pci_setup_device(struct pci_dev * dev)
 {
 	u32 class;
+	u16 pm;
 
 	sprintf(pci_name(dev), "%04x:%02x:%02x.%d", pci_domain_nr(dev->bus),
 		dev->bus->number, PCI_SLOT(dev->devfn), PCI_FUNC(dev->devfn));
@@ -707,6 +708,19 @@ static int pci_setup_device(struct pci_d
 		pci_read_config_word(dev, PCI_SUBSYSTEM_VENDOR_ID, &dev->subsystem_vendor);
 		pci_read_config_word(dev, PCI_SUBSYSTEM_ID, &dev->subsystem_device);
 
+		/* PCI PM capable devices may be able to issue PME# (wakeup) */
+		pm = pci_find_capability(dev, PCI_CAP_ID_PM);
+		if (pm) {
+			pci_read_config_word(dev, pm + PCI_PM_PMC, &pm);
+			if (pm & PCI_PM_CAP_PME_MASK)
+				device_init_wakeup(&dev->dev, 1);
+
+			/* REVISIT: if (pm & PCI_PM_CAP_PME_D3cold) then
+			 * pci pm spec 1.2, section 3.2.4 says we should
+			 * init PCI_PM_CTRL_PME_{STATUS,ENABLE} ...
+			 */
+		}
+
 		/*
 		 *	Do the ugly legacy mode stuff here rather than broken chip
 		 *	quirk code. Legacy mode ATA controllers have fixed
@@ -903,6 +917,7 @@ pci_scan_device(struct pci_bus *bus, int
 
 	dev->bus = bus;
 	dev->sysdata = bus->sysdata;
+	device_initialize(&dev->dev);
 	dev->dev.parent = bus->bridge;
 	dev->dev.bus = &pci_bus_type;
 	dev->devfn = devfn;
@@ -927,7 +942,6 @@ pci_scan_device(struct pci_bus *bus, int
 
 void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
 {
-	device_initialize(&dev->dev);
 	dev->dev.release = pci_release_dev;
 	pci_dev_get(dev);
 

Re: [linux-pm] [patch 2.6.25-rc6 7/7] PCI set up device.power.can_wakeup flags

[Alan Stern]

On Thu, 20 Mar 2008, David Brownell wrote:

> A previous version of this patch broke on PowerPC platforms after they
> changed how they do early PCI initialization (following the first version
> of this patch).  Potentially the real fix involves switching PCI init on
> all platforms so they all adopt the driver model "init() then add()" model,
> calling device_initialize() early and pci_setup_device() before device_add().

Would it help to break this apart into two patches: the first to switch 
PCI over to the driver model approach, and the second to set up the 
can_wakeup flags?  Those really are two very different goals.

What are the barriers to doing the first part right now?  Is it just 
PPC?

Alan Stern

Re: [linux-pm] [patch 2.6.25-rc6 7/7] PCI set up device.power.can_wakeup flags

[David Brownell]

On Thursday 20 March 2008, Alan Stern wrote:
> On Thu, 20 Mar 2008, David Brownell wrote:
> 
> > A previous version of this patch broke on PowerPC platforms after they
> > changed how they do early PCI initialization (following the first version
> > of this patch).  Potentially the real fix involves switching PCI init on
> > all platforms so they all adopt the driver model "init() then add()" model,
> > calling device_initialize() early and pci_setup_device() before device_add().
> 
> Would it help to break this apart into two patches: the first to switch 
> PCI over to the driver model approach, and the second to set up the 
> can_wakeup flags?  Those really are two very different goals.

Presumably, yes.  I'm not sure why there isn't a more
homogeneous approach to PCI init, between arch/* trees,
so I suspect there may be some down'n'dirty voodoo to
avoid tripping over.

That said, I'd sure *hope* that it could be cleaned up
in a way that all platforms can share one chunk of code
doing setup (like this) based on config space access.


> What are the barriers to doing the first part right now?  Is it just 
> PPC?

I have no idea, since I've not really looked at this in
any detail for some time now.  (The relevant PPC change
was more than two years ago...)  Heck, maybe it's even
been done already, and I just didn't notice!

- Dave

Re: [patch 2.6.25-rc6 3/7] pci_choose_state() cleanup and fixes

[Rafael J. Wysocki]

On Thursday, 20 of March 2008, David Brownell wrote:
> Clean up pci_choose_state():
> 
>  - pci_choose_state() should only return PCI_D0, unless the system is
>    entering a suspend (or hibernate) system state.
> 
>  - Only use platform_pci_choose_state() when entering a suspend
>    state ... and avoid PCI_D1 and PCI_D2 when appropriate.
> 
>  - Corrrect kerneldoc.
> 
> Note that for now only ACPI provides platform_pci_choose_state(), so
> this could be a minor change in behavior on some non-PC systems:  it
> avoids D3 except in the final stage of hibernation.
> 
> Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
> ---
>  drivers/pci/pci.c |   47 ++++++++++++++++++++++++++---------------------
>  1 file changed, 26 insertions(+), 21 deletions(-)
> 
> --- g26.orig/drivers/pci/pci.c	2008-02-24 00:18:16.000000000 -0800
> +++ g26/drivers/pci/pci.c	2008-02-24 00:41:18.000000000 -0800
> @@ -523,44 +523,49 @@ pci_set_power_state(struct pci_dev *dev,
>  }
>  
>  pci_power_t (*platform_pci_choose_state)(struct pci_dev *dev, pm_message_t state);
> - 
> +
>  /**
>   * pci_choose_state - Choose the power state of a PCI device
>   * @dev: PCI device to be suspended
> - * @state: target sleep state for the whole system. This is the value
> - *	that is passed to suspend() function.
> + * @mesg: value passed to suspend() function.
>   *
>   * Returns PCI power state suitable for given device and given system
> - * message.
> + * power state transition.
>   */
>  
> -pci_power_t pci_choose_state(struct pci_dev *dev, pm_message_t state)
> +pci_power_t pci_choose_state(struct pci_dev *dev, pm_message_t mesg)
>  {
>  	pci_power_t ret;
>  
> +	/* PCI legacy PM? */
>  	if (!pci_find_capability(dev, PCI_CAP_ID_PM))
>  		return PCI_D0;
>  
> -	if (platform_pci_choose_state) {
> -		ret = platform_pci_choose_state(dev, state);
> -		if (ret != PCI_POWER_ERROR)
> -			return ret;
> -	}
> -
> -	switch (state.event) {
> -	case PM_EVENT_ON:
> -		return PCI_D0;
> -	case PM_EVENT_FREEZE:
> -	case PM_EVENT_PRETHAW:
> -		/* REVISIT both freeze and pre-thaw "should" use D0 */
> +	switch (mesg.event) {
>  	case PM_EVENT_SUSPEND:
> +		/* NOTE:  platform_pci_choose_state() should only return
> +		 * states where wakeup won't work if
> +		 *   - !device_may_wakeup(&dev->dev), or
> +		 *   - dev can't wake from the target system state
> +		 */
> +		if (platform_pci_choose_state) {
> +			ret = platform_pci_choose_state(dev, mesg);
> +			if (ret == PCI_POWER_ERROR)
> +				ret = PCI_D3hot;
> +			else if ((ret == PCI_D1 || ret == PCI_D2)
> +					&& pci_no_d1d2(dev))
> +				ret = PCI_D3hot;
> +			break;
> +		}
> +		/* FALLTHROUGH ... D3hot works, but may be suboptimal */
>  	case PM_EVENT_HIBERNATE:
> -		return PCI_D3hot;
> +		ret = PCI_D3hot;

This is clearly wrong.  It should do the same as for suspend here (_S4D may be
defined and we should take it into account if it is).

> +		break;
>  	default:
> -		printk("Unrecognized suspend event %d\n", state.event);
> -		BUG();
> +		ret = PCI_D0;
> +		break;
>  	}
> -	return PCI_D0;
> +	return ret;
>  }
>  
>  EXPORT_SYMBOL(pci_choose_state);

I really don't think pci_choose_state() should take the state argument at all.

Thanks,
Rafael

Re: [patch 2.6.25-rc6 3/7] pci_choose_state() cleanup and fixes

[David Brownell]

> > +	switch (mesg.event) {
> >  	case PM_EVENT_SUSPEND:
> > +		/* NOTE:  platform_pci_choose_state() should only return
> > +		 * states where wakeup won't work if
> > +		 *   - !device_may_wakeup(&dev->dev), or
> > +		 *   - dev can't wake from the target system state
> > +		 */
> > +		if (platform_pci_choose_state) {
> > +			ret = platform_pci_choose_state(dev, mesg);
> > +			if (ret == PCI_POWER_ERROR)
> > +				ret = PCI_D3hot;
> > +			else if ((ret == PCI_D1 || ret == PCI_D2)
> > +					&& pci_no_d1d2(dev))
> > +				ret = PCI_D3hot;
> > +			break;
> > +		}
> > +		/* FALLTHROUGH ... D3hot works, but may be suboptimal */
> >  	case PM_EVENT_HIBERNATE:
> > -		return PCI_D3hot;
> > +		ret = PCI_D3hot;
>
> This is clearly wrong.  It should do the same as for suspend here (_S4D may be
> defined and we should take it into account if it is).

So you're saying the original code was wrong, and this patch should
change that behavior too?



> > +		break;
> >  	default:
> > -		printk("Unrecognized suspend event %d\n", state.event);
> > -		BUG();
> > +		ret = PCI_D0;
> > +		break;
> >  	}
> > -	return PCI_D0;
> > +	return ret;
> >  }
> >  
> >  EXPORT_SYMBOL(pci_choose_state);
>
> I really don't think pci_choose_state() should take the state argument at all.

There is no "state" argument.  It's a pm_message_t, which does
not package the target state.

A patch to change the calling syntax for this would necessarily
be a different patch... and would need to change the callers too.

- Dave

Re: [patch 2.6.25-rc6 3/7] pci_choose_state() cleanup and fixes

[Rafael J. Wysocki]

On Friday, 21 of March 2008, David Brownell wrote:
> > > +	switch (mesg.event) {
> > >  	case PM_EVENT_SUSPEND:
> > > +		/* NOTE:  platform_pci_choose_state() should only return
> > > +		 * states where wakeup won't work if
> > > +		 *   - !device_may_wakeup(&dev->dev), or
> > > +		 *   - dev can't wake from the target system state
> > > +		 */
> > > +		if (platform_pci_choose_state) {
> > > +			ret = platform_pci_choose_state(dev, mesg);
> > > +			if (ret == PCI_POWER_ERROR)
> > > +				ret = PCI_D3hot;
> > > +			else if ((ret == PCI_D1 || ret == PCI_D2)
> > > +					&& pci_no_d1d2(dev))
> > > +				ret = PCI_D3hot;
> > > +			break;
> > > +		}
> > > +		/* FALLTHROUGH ... D3hot works, but may be suboptimal */
> > >  	case PM_EVENT_HIBERNATE:
> > > -		return PCI_D3hot;
> > > +		ret = PCI_D3hot;
> >
> > This is clearly wrong.  It should do the same as for suspend here (_S4D may be
> > defined and we should take it into account if it is).
> 
> So you're saying the original code was wrong, and this patch should
> change that behavior too?

The original code executed platform_pci_choose_state() first, if defined, and
if that succeeded, it just returned the result.  You put
platform_pci_choose_state() under the switch(). :-)

In fact the entire switch() in pci_choose_state() is just confusing.  I'd make
it return PCI_D3hot if platform_pci_choose_state() is undefined or fails (I
wonder if pci_choose_state() is ever called with PMSG_ON).  That at least
would be consistent with the behavior of acpi_pm_device_sleep_state().

Consequently, the 'state' argument would simply be unnecessary (and in fact
it's ignored if platform_pci_choose_state() is defined).

> > > +		break;
> > >  	default:
> > > -		printk("Unrecognized suspend event %d\n", state.event);
> > > -		BUG();
> > > +		ret = PCI_D0;
> > > +		break;
> > >  	}
> > > -	return PCI_D0;
> > > +	return ret;
> > >  }
> > >  
> > >  EXPORT_SYMBOL(pci_choose_state);
> >
> > I really don't think pci_choose_state() should take the state argument at all.
> 
> There is no "state" argument.  It's a pm_message_t, which does
> not package the target state.

Correct, but the pm_message_t argument is called 'state', confusingly so.

> A patch to change the calling syntax for this would necessarily
> be a different patch... and would need to change the callers too.

Agreed.

Thanks,
Rafael

Re: [linux-pm] [patch 2.6.25-rc6 3/7] pci_choose_state() cleanup and fixes

[Alan Stern]

On Fri, 21 Mar 2008, Rafael J. Wysocki wrote:

> The original code executed platform_pci_choose_state() first, if defined, and
> if that succeeded, it just returned the result.  You put
> platform_pci_choose_state() under the switch(). :-)

For FREEZE and QUIESCE, is there ever any reason to leave D0?  These 
calls are documented as not requiring (and not desiring!) any change in 
power level.

> Consequently, the 'state' argument would simply be unnecessary (and in fact
> it's ignored if platform_pci_choose_state() is defined).

It should not be ignored, for the reason given above.

Alan Stern

Re: [linux-pm] [patch 2.6.25-rc6 3/7] pci_choose_state() cleanup and fixes

[Rafael J. Wysocki]

On Friday, 21 of March 2008, Alan Stern wrote:
> On Fri, 21 Mar 2008, Rafael J. Wysocki wrote:
> 
> > The original code executed platform_pci_choose_state() first, if defined, and
> > if that succeeded, it just returned the result.  You put
> > platform_pci_choose_state() under the switch(). :-)
> 
> For FREEZE and QUIESCE, is there ever any reason to leave D0?

No, but there's a reason to leave it for HIBERNATE and that's what my original
comment was about.

> These calls are documented as not requiring (and not desiring!) any change
> in power level.

Correct, but that's not the point.

Is there any reason why pci_choose_state() should try to figure out what kind
of operation is being performed by the driver and tailor its output to that?
I don't think so.  Rather, the driver should know what it's doing and either
call pci_choose_state() or not.  If the state of the device is not to be
changed, there's no reason to call pci_choose_state() at all.

My opinion is that drivers should only use pci_choose_state() if they _intend_
to change the state of the device and they should expect that the result may
not depend on the 'state' argument passed to it, but on the target state set by
the platform.  [Note that with the new suspend/hibernation callbacks there
won't be the pm_message_t argument to pass to pci_choose_state().]

Now, we need to settle _what_ exactly pci_choose_state() is supposed
to return, because it's not clear right now.  Should that be the lowest power
state in which the device can be in given system state (that's what
platform_pci_choose_state() will return)?  Or should that be the highest power
state in which the device can be in given system state?  Or anything else?

Thanks,
Rafael

Re: [patch 2.6.25-rc6 1/7] crosslink ACPI and "real" device nodes

[Zhao Yakui]

On Thu, 2008-03-20 at 14:09 -0700, David Brownell wrote:
> From: David Brownell <dbrownell@users.sourceforge.net>
> 
> Add cross-links between ACPI device and "real" devices in sysfs,
> exposing otherwise-hidden interrelationships between the various
> device nodes which ACPI creates.  As a representative example one
> hardware device is exposed as two logical devices (PNP and ACPI):
> 
>   .../pnp0/00:06/
>   .../LNXSYSTM:00/device:00/PNP0A03:00/device:15/PNP0B00:00/
> 
> The PNP device gets a "firmware_node" link pointing to the ACPI device.
> The ACPI device has a "physical_node" link pointing to the PNP device.
> Linux drivers currently bind only to the "physical" device nodes.
Very good idea. 
But maybe there is a lot of ACPI devices on the laptops. And we take a
little care about the association between the acpi device and "real"
device.
Maybe it is more useful if the link is set up for the acpi device with
the _PRW object. Maybe the link that points to ACPI device with the _PRW
object is created in the /sys/power/.
> 
> Other device trees provided by firmware tables could be exported
> using the same scheme; these names aren't ACPI-specific.
> 
> (Based on a patch from Zhang Rui.  This version is modified to not
> depend on the patch teaching ACPI about driver model wakeup flags.)
> Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
> Cc: Zhang Rui <rui.zhang@intel.com> 
> ---
> Change from previous version:  if creating the link fails, a
> pr_debug level message is emitted.
> 
>  drivers/acpi/glue.c |   27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> --- g26.orig/drivers/acpi/glue.c	2008-03-08 11:11:18.000000000 -0800
> +++ g26/drivers/acpi/glue.c	2008-03-08 11:29:07.000000000 -0800
> @@ -142,6 +142,7 @@ EXPORT_SYMBOL(acpi_get_physical_device);
>  
>  static int acpi_bind_one(struct device *dev, acpi_handle handle)
>  {
> +	struct acpi_device *acpi_dev;
>  	acpi_status status;
>  
>  	if (dev->archdata.acpi_handle) {
> @@ -157,6 +158,23 @@ static int acpi_bind_one(struct device *
>  	}
>  	dev->archdata.acpi_handle = handle;
>  
> +	status = acpi_bus_get_device(handle, &acpi_dev);
> +	if (!ACPI_FAILURE(status)) {
> +		int ret;
> +
> +		ret = sysfs_create_link(&dev->kobj, &acpi_dev->dev.kobj,
> +				"firmware_node");
> +		if (ret != 0)
> +			pr_debug(PREFIX "Can't create %s link for %s\n",
> +				"firmware_node", dev->bus_id);
> +
> +		ret = sysfs_create_link(&acpi_dev->dev.kobj, &dev->kobj,
> +				"physical_node");
> +		if (ret != 0)
> +			pr_debug(PREFIX "Can't create %s link for %s\n",
> +				"physical_node", acpi_dev->dev.bus_id);
> +	}
> +
>  	return 0;
>  }
>  
> @@ -165,8 +183,17 @@ static int acpi_unbind_one(struct device
>  	if (!dev->archdata.acpi_handle)
>  		return 0;
>  	if (dev == acpi_get_physical_device(dev->archdata.acpi_handle)) {
> +		struct acpi_device *acpi_dev;
> +
>  		/* acpi_get_physical_device increase refcnt by one */
>  		put_device(dev);
> +
> +		if (!acpi_bus_get_device(dev->archdata.acpi_handle,
> +					&acpi_dev)) {
> +			sysfs_remove_link(&dev->kobj, "firmware_node");
> +			sysfs_remove_link(&acpi_dev->dev.kobj, "physical_node");
> +		}
> +
>  		acpi_detach_data(dev->archdata.acpi_handle,
>  				 acpi_glue_data_handler);
>  		dev->archdata.acpi_handle = NULL;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch 2.6.25-rc6 1/7] crosslink ACPI and "real" device nodes

[David Brownell]

On Thursday 20 March 2008, Zhao Yakui wrote:
> On Thu, 2008-03-20 at 14:09 -0700, David Brownell wrote:
> > From: David Brownell <dbrownell@users.sourceforge.net>
> > 
> > Add cross-links between ACPI device and "real" devices in sysfs,
> > exposing otherwise-hidden interrelationships between the various
> > device nodes which ACPI creates.  As a representative example one
> > hardware device is exposed as two logical devices (PNP and ACPI):
> > 
> >   .../pnp0/00:06/
> >   .../LNXSYSTM:00/device:00/PNP0A03:00/device:15/PNP0B00:00/
> > 
> > The PNP device gets a "firmware_node" link pointing to the ACPI device.
> > The ACPI device has a "physical_node" link pointing to the PNP device.
> > Linux drivers currently bind only to the "physical" device nodes.
>  
> Very good idea. 
> But maybe there is a lot of ACPI devices on the laptops. And we take a
> little care about the association between the acpi device and "real"
> device.

Are you suggesting that the ACPI nodes shouldn't exist at all?
Or that something is wrong with how they're set up or used?

For now, there's some confusion.  Devices listed in ACPI tables
have one or two extra sysfs device nodes.  I think *something*
should help sort out the confusion -- this patch (originally
from Zhang Rui, a co-worker of yours) is the only help for that
which I've seen so far.  Do you propose some other solution?
If so, what is it ... and what are its advantages?


> Maybe it is more useful if the link is set up for the acpi device with
> the _PRW object. Maybe the link that points to ACPI device with the _PRW
> object is created in the /sys/power/.

That's way too many "maybes" for my taste!  Maybe I don't see
how "_PRW" could be the only interesting thing, when most of
the ACPI nodes don't even *have* such records ... and when not
all power-manageable devices are even known to ACPI.

The notion of adding ACPI-specific stuff to generic areas (like
/sys/power) really bothers me...

- Dave

Re: [patch 2.6.25-rc6 5/7] ACPI sets up device.power.can_wakeup flags

[Zhao Yakui]

On Thu, 2008-03-20 at 14:20 -0700, David Brownell wrote:
> This exports the "acpi_device.wakeup.flags.valid" flag to the driver
> model as "device.power.can_wakeup".  That is, each "ACPI device" in
> the /proc/acpi/wakeup file that corresponds to a "real" device will
> initialize the can_wakeup flag to true.
> 
For the button device it is good to set the wakeup flag. 
Good. When the acpi device supports the _PRW object, the wakeup flag bit
is set.( Can_wakeup; should_wakeup).
> A separate patch is needed to make ACPI pay attention to the userspace
> input:  devices that can_wakeup have a should_wakeup flag, initially
> true, which is settable via sysfs and tested using device_may_wakeup().
> That's intended to be the primary policy input to the system, not the
> ACPI-specific /proc/acpi/wakeup file.


> Note also that the ACPI tables list only motherboard devices, but many
> systems include more peripherals than those.  While the USB framework
> handles USB peripherals, nothing in Linux currently sets those flags
> for wakeup-capable devices on other expansion busses (like PCI).
> 
> Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
> ---
>  drivers/acpi/button.c |    3 +++
>  drivers/acpi/glue.c   |    1 +
>  drivers/acpi/power.c  |    4 ++++
>  3 files changed, 8 insertions(+)
> 
> --- g26.orig/drivers/acpi/button.c	2008-03-20 03:02:45.000000000 -0700
> +++ g26/drivers/acpi/button.c	2008-03-20 03:27:24.000000000 -0700
> @@ -476,6 +476,9 @@ static int acpi_button_add(struct acpi_d
>  	if (button->type == ACPI_BUTTON_TYPE_LID)
>  		acpi_lid_send_state(button);
>  
> +	/* for these devices the ACPI node *IS* the "physical" device */
> +	device_init_wakeup(&device->dev, device->wakeup.flags.valid);
> +

>  	if (device->wakeup.flags.valid) {
>  		/* Button's GPE is run-wake GPE */
>  		acpi_set_gpe_type(device->wakeup.gpe_device,
> --- g26.orig/drivers/acpi/glue.c	2008-03-20 03:27:11.000000000 -0700
> +++ g26/drivers/acpi/glue.c	2008-03-20 03:27:25.000000000 -0700
> @@ -162,6 +162,7 @@ static int acpi_bind_one(struct device *
>  	if (!ACPI_FAILURE(status)) {
>  		int ret;
>  
> +		device_init_wakeup(dev, acpi_dev->wakeup.flags.valid);
>  		ret = sysfs_create_link(&dev->kobj, &acpi_dev->dev.kobj,
>  				"firmware_node");
>  		if (ret != 0)
> --- g26.orig/drivers/acpi/power.c	2008-03-20 03:02:45.000000000 -0700
> +++ g26/drivers/acpi/power.c	2008-03-20 03:27:25.000000000 -0700
> @@ -313,6 +313,7 @@ int acpi_enable_wakeup_device_power(stru
>  		ret = acpi_power_on(dev->wakeup.resources.handles[i], dev);
>  		if (ret) {
>  			printk(KERN_ERR PREFIX "Transition power state\n");
> +			device_init_wakeup(&dev->dev, 0);
>  			dev->wakeup.flags.valid = 0;
>  			return -1;
>  		}
> @@ -322,6 +323,7 @@ int acpi_enable_wakeup_device_power(stru
>  	status = acpi_evaluate_object(dev->handle, "_PSW", &arg_list, NULL);
>  	if (ACPI_FAILURE(status) && (status != AE_NOT_FOUND)) {
>  		printk(KERN_ERR PREFIX "Evaluate _PSW\n");
> +		device_init_wakeup(&dev->dev, 0);
>  		dev->wakeup.flags.valid = 0;
>  		ret = -1;
>  	}
> @@ -351,6 +353,7 @@ int acpi_disable_wakeup_device_power(str
>  	status = acpi_evaluate_object(dev->handle, "_PSW", &arg_list, NULL);
>  	if (ACPI_FAILURE(status) && (status != AE_NOT_FOUND)) {
>  		printk(KERN_ERR PREFIX "Evaluate _PSW\n");
> +		device_init_wakeup(&dev->dev, 0);
>  		dev->wakeup.flags.valid = 0;
>  		return -1;
>  	}
> @@ -360,6 +363,7 @@ int acpi_disable_wakeup_device_power(str
>  		ret = acpi_power_off_device(dev->wakeup.resources.handles[i], dev);
>  		if (ret) {
>  			printk(KERN_ERR PREFIX "Transition power state\n");
> +			device_init_wakeup(&dev->dev, 0);
>  			dev->wakeup.flags.valid = 0;
>  			return -1;
>  		}
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch 2.6.25-rc6 3/7] pci_choose_state() cleanup and fixes

[David Brownell]

On Thursday 20 March 2008, Rafael J. Wysocki wrote:
> The original code executed platform_pci_choose_state() first, if defined, and
> if that succeeded, it just returned the result.  You put
> platform_pci_choose_state() under the switch(). :-)

So the updated patch just chooses a new state when drivers are
supposed to enter a lowpowerstate:  SUSPEND and HIBERNATE.
Appended.


> In fact the entire switch() in pci_choose_state() is just confusing.

All it does is implement the rules that have been defined for
ages now:  most of the pm_message_t transitions should not
change device power states.


> I'd make 
> it return PCI_D3hot if platform_pci_choose_state() is undefined or fails

See above:  this implements the current rules, which say
that in most cases devices shoudn't change powerstates.


> > > I really don't think pci_choose_state() should take the state argument at all.
> > 
> > There is no "state" argument.  It's a pm_message_t, which does
> > not package the target state.
> 
> Correct, but the pm_message_t argument is called 'state', confusingly so.

Which was (and is!) one of the cleanups in $SUBJECT.

- Dave


======== CUT HERE
Clean up pci_choose_state():

 - pci_choose_state() should only return PCI_D0, unless the system is
   entering a suspend (or hibernate) system state.

 - Only use platform_pci_choose_state() when entering a suspend
   state ... and avoid PCI_D1 and PCI_D2 when appropriate.

 - Corrrect kerneldoc.

Note that for now only ACPI provides platform_pci_choose_state(), so
this could be a minor change in behavior on some non-PC systems:  it
avoids D3 except in the final stage of hibernation.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
 drivers/pci/pci.c |   49 ++++++++++++++++++++++++++-----------------------
 1 file changed, 26 insertions(+), 23 deletions(-)

--- g26.orig/drivers/pci/pci.c	2008-03-20 03:02:46.000000000 -0700
+++ g26/drivers/pci/pci.c	2008-03-21 00:51:19.000000000 -0700
@@ -523,46 +523,49 @@ pci_set_power_state(struct pci_dev *dev,
 }
 
 pci_power_t (*platform_pci_choose_state)(struct pci_dev *dev, pm_message_t state);
- 
+
 /**
  * pci_choose_state - Choose the power state of a PCI device
  * @dev: PCI device to be suspended
- * @state: target sleep state for the whole system. This is the value
- *	that is passed to suspend() function.
+ * @mesg: value passed to suspend() function.
  *
  * Returns PCI power state suitable for given device and given system
- * message.
+ * power state transition.
  */
-
-pci_power_t pci_choose_state(struct pci_dev *dev, pm_message_t state)
+pci_power_t pci_choose_state(struct pci_dev *dev, pm_message_t mesg)
 {
 	pci_power_t ret;
 
+	/* PCI legacy PM? */
 	if (!pci_find_capability(dev, PCI_CAP_ID_PM))
 		return PCI_D0;
 
-	if (platform_pci_choose_state) {
-		ret = platform_pci_choose_state(dev, state);
-		if (ret != PCI_POWER_ERROR)
-			return ret;
-	}
-
-	switch (state.event) {
-	case PM_EVENT_ON:
-		return PCI_D0;
-	case PM_EVENT_FREEZE:
-	case PM_EVENT_PRETHAW:
-		/* REVISIT both freeze and pre-thaw "should" use D0 */
+	switch (mesg.event) {
 	case PM_EVENT_SUSPEND:
 	case PM_EVENT_HIBERNATE:
-		return PCI_D3hot;
+		/* NOTE:  platform_pci_choose_state() should only return
+		 * states where wakeup won't work if
+		 *   - !device_may_wakeup(&dev->dev), or
+		 *   - dev can't wake from the target system state
+		 */
+		if (platform_pci_choose_state) {
+			ret = platform_pci_choose_state(dev, mesg);
+			if (ret == PCI_POWER_ERROR)
+				ret = PCI_D3hot;
+			else if ((ret == PCI_D1 || ret == PCI_D2)
+					&& pci_no_d1d2(dev))
+				ret = PCI_D3hot;
+			break;
+		}
+		/* D3hot works, but may be suboptimal */
+		ret = PCI_D3hot;
+		break;
 	default:
-		printk("Unrecognized suspend event %d\n", state.event);
-		BUG();
+		ret = PCI_D0;
+		break;
 	}
-	return PCI_D0;
+	return ret;
 }
-
 EXPORT_SYMBOL(pci_choose_state);
 
 static int pci_save_pcie_state(struct pci_dev *dev)

Re: [linux-pm] [patch 2.6.25-rc6 3/7] pci_choose_state() cleanup and fixes

[David Brownell]

On Thursday 20 March 2008, Rafael J. Wysocki wrote:
> Is there any reason why pci_choose_state() should try to figure out what kind
> of operation is being performed by the driver and tailor its output to that?

It's always been specified to do that ... but it's always done
that in a buggy way.  (Which is why USB never used it.)


> I don't think so.  Rather, the driver should know what it's doing and either
> call pci_choose_state() or not.  If the state of the device is not to be
> changed, there's no reason to call pci_choose_state() at all.

You seem to object to letting drivers offload this particular
bit of work to infrastructure.  What's the dividing line between
being OK to offload vs not eing OK?  Why not let the drivers make
that choice?


> 	[Note that with the new suspend/hibernation callbacks there
> won't be the pm_message_t argument to pass to pci_choose_state().]

The pm_message_t will necessarily linger until all drivers have
been converted and re-tested.  Which can't be an overnight thing.


> Now, we need to settle _what_ exactly pci_choose_state() is supposed
> to return, because it's not clear right now.

Kerneldoc says:

 * Returns PCI power state suitable for given device and given system
 * power state transition.

Some comments added by $SUBJECT also clarify:

                /* NOTE:  platform_pci_choose_state() should only return
                 * states where wakeup won't work if
                 *   - !device_may_wakeup(&dev->dev), or
                 *   - dev can't wake from the target system state
                 */

That happens to be what acpi_pm_device_sleep_state() computes;
there may not be other implementations of that call, yet.  I can
imagine a default that would look at PCI PM capabilities, to be
used on non-ACPI platforms.


>	 Should that be the lowest power 
> state in which the device can be in given system state (that's what
> platform_pci_choose_state() will return)?

Actually the comment above is the *entirety* of the docs for
that call:  it chooses a state.  You're asking a question of
policy (how/why it chooses); that's outside the scope of PCI.


> Or should that be the highest power 
> state in which the device can be in given system state?
> Or anything else? 

It happens that the current Linux ACPI glue chooses the
lowest power PCI state that's compatible with the target
system state ... optimizing for power savings rather than
for quick resumes.

It might make sense to support a policy that optimizes for
fast resumes from some states.  But I can't see that'd be
worth much effort at this point, with bigger fish to fry!

- Dave


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

Re: [patch 2.6.25-rc6 1/7] crosslink ACPI and "real" device nodes

[Zhao Yakui]

On Fri, 2008-03-21 at 00:31 -0700, David Brownell wrote:
> On Thursday 20 March 2008, Zhao Yakui wrote:
> > On Thu, 2008-03-20 at 14:09 -0700, David Brownell wrote:
> > > From: David Brownell <dbrownell@users.sourceforge.net>
> > > 
> > > Add cross-links between ACPI device and "real" devices in sysfs,
> > > exposing otherwise-hidden interrelationships between the various
> > > device nodes which ACPI creates.  As a representative example one
> > > hardware device is exposed as two logical devices (PNP and ACPI):
> > > 
> > >   .../pnp0/00:06/
> > >   .../LNXSYSTM:00/device:00/PNP0A03:00/device:15/PNP0B00:00/
> > > 
> > > The PNP device gets a "firmware_node" link pointing to the ACPI device.
> > > The ACPI device has a "physical_node" link pointing to the PNP device.
> > > Linux drivers currently bind only to the "physical" device nodes.
> >  
> > Very good idea. 
> > But maybe there is a lot of ACPI devices on the laptops. And we take a
> > little care about the association between the acpi device and "real"
> > device.
> 
> Are you suggesting that the ACPI nodes shouldn't exist at all?
> Or that something is wrong with how they're set up or used?
No. The ACPI nodes should exist. What I said is whether it is necessary
to create the link for all the ACPI devices between the ACPI device and
"real" node device.  Of course it is also OK if link is created for the
ACPI device with the ability to wake the sleeping system. 
> For now, there's some confusion.  Devices listed in ACPI tables
> have one or two extra sysfs device nodes.  I think *something*
> should help sort out the confusion -- this patch (originally
> from Zhang Rui, a co-worker of yours) is the only help for that
> which I've seen so far.  Do you propose some other solution?
> If so, what is it ... and what are its advantages?
> > Maybe it is more useful if the link is set up for the acpi device with
> > the _PRW object. Maybe the link that points to ACPI device with the _PRW
> > object is created in the /sys/power/.
> 
> That's way too many "maybes" for my taste!  Maybe I don't see
> how "_PRW" could be the only interesting thing, when most of
> the ACPI nodes don't even *have* such records ... and when not
> all power-manageable devices are even known to ACPI.
Sorry. What I means is that the link(point to the ACPI device with the
ability to wake the sleeping system) is created in the /sys/power/. 
After doing so, we can easily check which device has the ability to wake
the sleeping system. (The _PRW object indicates that the device have the
ability to wake the sleeping system). 
> 
> The notion of adding ACPI-specific stuff to generic areas (like
> /sys/power) really bothers me...
> 
> - Dave
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch 2.6.25-rc6 1/7] crosslink ACPI and "real" device nodes

[David Brownell]

On Friday 21 March 2008, Zhao Yakui wrote:
> On Fri, 2008-03-21 at 00:31 -0700, David Brownell wrote:
> > > > The PNP device gets a "firmware_node" link pointing to the ACPI device.
> > > > The ACPI device has a "physical_node" link pointing to the PNP device.
> > > > Linux drivers currently bind only to the "physical" device nodes.
> > >  
> > > Very good idea. 
> > > But maybe there is a lot of ACPI devices on the laptops. And we take a
> > > little care about the association between the acpi device and "real"
> > > device.
> > 
> > Are you suggesting that the ACPI nodes shouldn't exist at all?
> > Or that something is wrong with how they're set up or used?
> 
> No. The ACPI nodes should exist. What I said is whether it is necessary
> to create the link for all the ACPI devices between the ACPI device and
> "real" node device.  Of course it is also OK if link is created for the
> ACPI device with the ability to wake the sleeping system. 

The confusion exists for all ACPI device nodes that
mirror "real" device nodes (like PNP or PCI devices).

It's *not* limited to wake-capable devices.


> > For now, there's some confusion.  Devices listed in ACPI tables
> > have one or two extra sysfs device nodes.  I think *something*
> > should help sort out the confusion ...

(Minor correction:  *most* devices listed in ACPI tables have
the problem of extra sysfs nodes.  A few don't; like buttons.)


> Sorry. What I means is that the link(point to the ACPI device with the
> ability to wake the sleeping system) is created in the /sys/power/. 
> After doing so, we can easily check which device has the ability to wake
> the sleeping system. 

There's no need for symlinks to do that ... or ACPI.

I've been meaning to repost my script that scans sysfs
for the wakeup-capable devices ... I updated it a short
while ago to work right without "legacy" sysfs nodes.

- Dave

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

Re: [linux-pm] [patch 2.6.25-rc6 3/7] pci_choose_state() cleanup and fixes

[Rafael J. Wysocki]

On Friday, 21 of March 2008, David Brownell wrote:
> On Thursday 20 March 2008, Rafael J. Wysocki wrote:
> > Is there any reason why pci_choose_state() should try to figure out what kind
> > of operation is being performed by the driver and tailor its output to that?
> 
> It's always been specified to do that ... but it's always done
> that in a buggy way.  (Which is why USB never used it.)

I hope you realize that your change will affect all drivers calling
pci_choose_state() on ACPI systems on FREEZE/PRETHAW in a hard to predict
way?  Perhaps some of them actually rely on getting D3hot on FREEZE, for
example?

> > I don't think so.  Rather, the driver should know what it's doing and either
> > call pci_choose_state() or not.  If the state of the device is not to be
> > changed, there's no reason to call pci_choose_state() at all.
> 
> You seem to object to letting drivers offload this particular
> bit of work to infrastructure.

No, I don't.  I just don't think it's a good idea to change the existing and
widely used function for this purpose.  If I needed some specific functionality
at the infrastructure level, I'd add a new function for that with a new
changelog etc.  Then, made drivers switch to that and remove the old one.

> What's the dividing line between being OK to offload vs not eing OK?  Why not
> let the drivers make that choice?
> 
> 
> > 	[Note that with the new suspend/hibernation callbacks there
> > won't be the pm_message_t argument to pass to pci_choose_state().]
> 
> The pm_message_t will necessarily linger until all drivers have
> been converted and re-tested.  Which can't be an overnight thing.

No, it can't.  Still, suppose a driver is using the new callbacks.  How is it
supposed to use pci_choose_state()?

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

Re: [patch 2.6.25-rc6 3/7] pci_choose_state() cleanup and fixes

[Rafael J. Wysocki]

On Friday, 21 of March 2008, David Brownell wrote:
> On Thursday 20 March 2008, Rafael J. Wysocki wrote:
> > The original code executed platform_pci_choose_state() first, if defined, and
> > if that succeeded, it just returned the result.  You put
> > platform_pci_choose_state() under the switch(). :-)
> 
> So the updated patch just chooses a new state when drivers are
> supposed to enter a lowpowerstate:  SUSPEND and HIBERNATE.
> Appended.
> 
> 
> > In fact the entire switch() in pci_choose_state() is just confusing.
> 
> All it does is implement the rules that have been defined for
> ages now:  most of the pm_message_t transitions should not
> change device power states.

But they do at the moment, so you're going to change how the code currently
works on a quite large scale.

> > I'd make it return PCI_D3hot if platform_pci_choose_state() is undefined
> > or fails 
> 
> See above:  this implements the current rules, which say
> that in most cases devices shoudn't change powerstates.

Okay, say you're changing pci_choose_state() to follow the documentation.
Are you sure, however, that it won't cause any regressions to appear?
 
> > > > I really don't think pci_choose_state() should take the state argument at all.
> > > 
> > > There is no "state" argument.  It's a pm_message_t, which does
> > > not package the target state.
> > 
> > Correct, but the pm_message_t argument is called 'state', confusingly so.
> 
> Which was (and is!) one of the cleanups in $SUBJECT.
> 
> - Dave
> 
> 
> ======== CUT HERE
> Clean up pci_choose_state():
> 
>  - pci_choose_state() should only return PCI_D0, unless the system is
>    entering a suspend (or hibernate) system state.
> 
>  - Only use platform_pci_choose_state() when entering a suspend
>    state ... and avoid PCI_D1 and PCI_D2 when appropriate.
> 
>  - Corrrect kerneldoc.
> 
> Note that for now only ACPI provides platform_pci_choose_state(), so
> this could be a minor change in behavior on some non-PC systems:  it
> avoids D3 except in the final stage of hibernation.

Well, in fact the change is quite substantial as far as ACPI systems are
concerned, because on such systems the existing code will most likely
return D3hot for FREEZE/PRETHAW and the new code will return D0 in that cases.

> Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
> ---
>  drivers/pci/pci.c |   49 ++++++++++++++++++++++++++-----------------------
>  1 file changed, 26 insertions(+), 23 deletions(-)
> 
> --- g26.orig/drivers/pci/pci.c	2008-03-20 03:02:46.000000000 -0700
> +++ g26/drivers/pci/pci.c	2008-03-21 00:51:19.000000000 -0700
> @@ -523,46 +523,49 @@ pci_set_power_state(struct pci_dev *dev,
>  }
>  
>  pci_power_t (*platform_pci_choose_state)(struct pci_dev *dev, pm_message_t state);
> - 
> +
>  /**
>   * pci_choose_state - Choose the power state of a PCI device
>   * @dev: PCI device to be suspended
> - * @state: target sleep state for the whole system. This is the value
> - *	that is passed to suspend() function.
> + * @mesg: value passed to suspend() function.
>   *
>   * Returns PCI power state suitable for given device and given system
> - * message.
> + * power state transition.
>   */
> -
> -pci_power_t pci_choose_state(struct pci_dev *dev, pm_message_t state)
> +pci_power_t pci_choose_state(struct pci_dev *dev, pm_message_t mesg)
>  {
>  	pci_power_t ret;
>  
> +	/* PCI legacy PM? */
>  	if (!pci_find_capability(dev, PCI_CAP_ID_PM))
>  		return PCI_D0;
>  
> -	if (platform_pci_choose_state) {
> -		ret = platform_pci_choose_state(dev, state);
> -		if (ret != PCI_POWER_ERROR)
> -			return ret;
> -	}
> -
> -	switch (state.event) {
> -	case PM_EVENT_ON:
> -		return PCI_D0;
> -	case PM_EVENT_FREEZE:
> -	case PM_EVENT_PRETHAW:
> -		/* REVISIT both freeze and pre-thaw "should" use D0 */
> +	switch (mesg.event) {
>  	case PM_EVENT_SUSPEND:
>  	case PM_EVENT_HIBERNATE:
> -		return PCI_D3hot;
> +		/* NOTE:  platform_pci_choose_state() should only return
> +		 * states where wakeup won't work if
> +		 *   - !device_may_wakeup(&dev->dev), or
> +		 *   - dev can't wake from the target system state
> +		 */
> +		if (platform_pci_choose_state) {
> +			ret = platform_pci_choose_state(dev, mesg);
> +			if (ret == PCI_POWER_ERROR)
> +				ret = PCI_D3hot;
> +			else if ((ret == PCI_D1 || ret == PCI_D2)
> +					&& pci_no_d1d2(dev))
> +				ret = PCI_D3hot;
> +			break;
> +		}
> +		/* D3hot works, but may be suboptimal */
> +		ret = PCI_D3hot;
> +		break;

I would do:

+		if (platform_pci_choose_state) {
+			ret = platform_pci_choose_state(dev, mesg);
+			if (ret == PCI_POWER_ERROR || (pci_no_d1d2(dev)
+			    && (ret == PCI_D1 || ret == PCI_D2)))
+				ret = PCI_D3hot;
+		} else {
+			/* D3hot works, but may be suboptimal */
+			ret = PCI_D3hot;
+		}
+		break;

>  	default:
> -		printk("Unrecognized suspend event %d\n", state.event);
> -		BUG();
> +		ret = PCI_D0;
> +		break;
>  	}
> -	return PCI_D0;
> +	return ret;
>  }
> -
>  EXPORT_SYMBOL(pci_choose_state);
>  
>  static int pci_save_pcie_state(struct pci_dev *dev)
> 
> 

Thanks,
Rafael

Re: [patch 2.6.25-rc6 3/7] pci_choose_state() cleanup and fixes

[David Brownell]

On Friday 21 March 2008, Rafael J. Wysocki wrote:
> On Friday, 21 of March 2008, David Brownell wrote:
> > All it does is implement the rules that have been defined for
> > ages now:  most of the pm_message_t transitions should not
> > change device power states.
> 
> But they do at the moment, so you're going to change how the code
> currently works on a quite large scale.

Are you arguing that this bug "should not be fixed"?

Or are you arguing that there's something wrong with
the rules, so that they "should not be followed"?

Or something else?


> > > I'd make it return PCI_D3hot if platform_pci_choose_state()
> > > is undefined or fails 
> > 
> > See above:  this implements the current rules, which say
> > that in most cases devices shoudn't change powerstates.
> 
> Okay, say you're changing pci_choose_state() to follow the
> documentation. 

Not just documentation -- the current architecture of the
whole suspend process.  Surely it's essential not to have
bugs like those at the core of that process?

If that process is broken, but this PCI bug hides it, we'll
be having a boatload of unexpected trouble in a while...


> Are you sure, however, that it won't cause any regressions to appear?

No more than any other bugfix turning up other latent bugs.
That's why we have a process which lets fixes cook for a while
before they merge.


> > +	switch (mesg.event) {
> >  	case PM_EVENT_SUSPEND:
> >  	case PM_EVENT_HIBERNATE:
> > -		return PCI_D3hot;
> > +		/* NOTE:  platform_pci_choose_state() should only return
> > +		 * states where wakeup won't work if
> > +		 *   - !device_may_wakeup(&dev->dev), or
> > +		 *   - dev can't wake from the target system state
> > +		 */
> > +		if (platform_pci_choose_state) {
> > +			ret = platform_pci_choose_state(dev, mesg);
> > +			if (ret == PCI_POWER_ERROR)
> > +				ret = PCI_D3hot;
> > +			else if ((ret == PCI_D1 || ret == PCI_D2)
> > +					&& pci_no_d1d2(dev))
> > +				ret = PCI_D3hot;
> > +			break;
> > +		}
> > +		/* D3hot works, but may be suboptimal */
> > +		ret = PCI_D3hot;
> > +		break;
> 
> I would do:
> 
> +		if (platform_pci_choose_state) {
> +			ret = platform_pci_choose_state(dev, mesg);
> +			if (ret == PCI_POWER_ERROR || (pci_no_d1d2(dev)
> +			    && (ret == PCI_D1 || ret == PCI_D2)))

That's ... phenomenally ugly and incomprehensible!  It hides
almost the entire structure of the conditionals.


> +				ret = PCI_D3hot;
> +		} else {
> +			/* D3hot works, but may be suboptimal */
> +			ret = PCI_D3hot;
> +		}

Extra/pointless brackets after "else" ... so you think the quickie
fix should have removed that first "break"?  Simpler to just have
said so.


> +		break;


======== CUT HERE
Clean up pci_choose_state():

 - pci_choose_state() should only return PCI_D0, unless the system is
   entering a suspend (or hibernate) system state.

 - Only use platform_pci_choose_state() when entering a suspend
   state ... and avoid PCI_D1 and PCI_D2 when appropriate.

 - Corrrect kerneldoc.

Note that for now only ACPI provides platform_pci_choose_state(), so
this could be a minor change in behavior on some non-PC systems:  it
avoids D3 except in the final stage of hibernation.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
 drivers/pci/pci.c |   53 ++++++++++++++++++++++++++++++-----------------------
 1 file changed, 30 insertions(+), 23 deletions(-)

--- g26.orig/drivers/pci/pci.c	2008-03-22 10:25:48.000000000 -0700
+++ g26/drivers/pci/pci.c	2008-03-22 10:44:28.000000000 -0700
@@ -523,46 +523,53 @@ pci_set_power_state(struct pci_dev *dev,
 }
 
 pci_power_t (*platform_pci_choose_state)(struct pci_dev *dev, pm_message_t state);
- 
+
 /**
  * pci_choose_state - Choose the power state of a PCI device
  * @dev: PCI device to be suspended
- * @state: target sleep state for the whole system. This is the value
- *	that is passed to suspend() function.
+ * @mesg: value passed to suspend() function.
  *
  * Returns PCI power state suitable for given device and given system
- * message.
+ * power state transition.
  */
-
-pci_power_t pci_choose_state(struct pci_dev *dev, pm_message_t state)
+pci_power_t pci_choose_state(struct pci_dev *dev, pm_message_t mesg)
 {
 	pci_power_t ret;
 
+	/* PCI legacy PM? */
 	if (!pci_find_capability(dev, PCI_CAP_ID_PM))
 		return PCI_D0;
 
-	if (platform_pci_choose_state) {
-		ret = platform_pci_choose_state(dev, state);
-		if (ret != PCI_POWER_ERROR)
-			return ret;
-	}
-
-	switch (state.event) {
-	case PM_EVENT_ON:
-		return PCI_D0;
-	case PM_EVENT_FREEZE:
-	case PM_EVENT_PRETHAW:
-		/* REVISIT both freeze and pre-thaw "should" use D0 */
+	switch (mesg.event) {
 	case PM_EVENT_SUSPEND:
 	case PM_EVENT_HIBERNATE:
-		return PCI_D3hot;
+		/*
+		 * D3hot works on all devices, but may be suboptimal on
+		 * a given system.  Factors include wakeups, power use,
+		 * the optional D3hot->D0 reset, latency, and more.
+		 *
+		 * If there's a platform_pci_choose_state(), it could
+		 * provide better answers for this system.  It should
+		 * only return states where wakeup won't work if:
+		 *   - !device_may_wakeup(&dev->dev), or
+		 *   - dev can't wake from the target system state
+		 */
+		ret = PCI_D3hot;
+		if (platform_pci_choose_state) {
+			ret = platform_pci_choose_state(dev, mesg);
+			if (ret == PCI_POWER_ERROR)
+				ret = PCI_D3hot;
+			else if ((ret == PCI_D1 || ret == PCI_D2)
+					&& pci_no_d1d2(dev))
+				ret = PCI_D3hot;
+		}
+		break;
 	default:
-		printk("Unrecognized suspend event %d\n", state.event);
-		BUG();
+		ret = PCI_D0;
+		break;
 	}
-	return PCI_D0;
+	return ret;
 }
-
 EXPORT_SYMBOL(pci_choose_state);
 
 static int pci_save_pcie_state(struct pci_dev *dev)

Re: [linux-pm] [patch 2.6.25-rc6 3/7] pci_choose_state() cleanup and fixes

[David Brownell]

On Friday 21 March 2008, Rafael J. Wysocki wrote:
> > 
> > You seem to object to letting drivers offload this particular
> > bit of work to infrastructure.
> 
> No, I don't.  I just don't think it's a good idea to change the existing and
> widely used function for this purpose.  If I needed some specific functionality
> at the infrastructure level, I'd add a new function for that with a new
> changelog etc.  Then, made drivers switch to that and remove the old one.

I see that a lot of drivers have at some point, not long ago,
been converted to use this routine.  They previously just
used PCI_D3 in all cases.

It seems to me that your objection boils down to the concern
that those drivers may just have pushed their bug out a level,
rather than actually fixing their bugs.

Which I can sympathize with ... but that doesn't change the
fact that any driver in that position *still* has a bug that
needs to be fixed.  And if that bug is highlighted by this
patch ... well, there's still a driver bug to be fixed.


> > >     [Note that with the new suspend/hibernation callbacks there
> > > won't be the pm_message_t argument to pass to pci_choose_state().]
> >
> > The pm_message_t will necessarily linger until all drivers have
> > been converted and re-tested.  Which can't be an overnight thing.
>
> No, it can't.  Still, suppose a driver is using the new callbacks.  How is
> it supposed to use pci_choose_state()?

Hey, you're the one providing those callbacks.  How were you
going to answer that question *before* I posted this overdue
bugfix patch for pci_choose_state()?

- Dave

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

Re: [linux-pm] [patch 2.6.25-rc6 3/7] pci_choose_state() cleanup and fixes

[Rafael J. Wysocki]

On Saturday, 22 of March 2008, David Brownell wrote:
> On Friday 21 March 2008, Rafael J. Wysocki wrote:
> > > 
> > > You seem to object to letting drivers offload this particular
> > > bit of work to infrastructure.
> > 
> > No, I don't.  I just don't think it's a good idea to change the existing and
> > widely used function for this purpose.  If I needed some specific functionality
> > at the infrastructure level, I'd add a new function for that with a new
> > changelog etc.  Then, made drivers switch to that and remove the old one.
> 
> I see that a lot of drivers have at some point, not long ago,
> been converted to use this routine.  They previously just
> used PCI_D3 in all cases.
> 
> It seems to me that your objection boils down to the concern
> that those drivers may just have pushed their bug out a level,
> rather than actually fixing their bugs.

Or, the authors of the drivers looked at what the code in
pci_choose_state() actually did and used it on that basis.

> Which I can sympathize with ... but that doesn't change the
> fact that any driver in that position *still* has a bug that
> needs to be fixed.  And if that bug is highlighted by this
> patch ... well, there's still a driver bug to be fixed.

Well, IMHO, the safe way of doing thing would be to audit and possibly fix the
drivers first.  Otherwise, we may get regression reports and we'll have to
revert the patch anyway if we're unable to fix them in a timely manner.

> > > >     [Note that with the new suspend/hibernation callbacks there
> > > > won't be the pm_message_t argument to pass to pci_choose_state().]
> > >
> > > The pm_message_t will necessarily linger until all drivers have
> > > been converted and re-tested.  Which can't be an overnight thing.
> >
> > No, it can't.  Still, suppose a driver is using the new callbacks.  How is
> > it supposed to use pci_choose_state()?
> 
> Hey, you're the one providing those callbacks.  How were you
> going to answer that question *before* I posted this overdue
> bugfix patch for pci_choose_state()?

Then it was simple, because pci_choose_state() returned D0 only for PMSG_ON.

Also, I was going to provide a simplified version of it that wouldn't take
arguments and would return D3hot in case when platform_pci_choose_state()
was not defined.

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

Re: [linux-pm] [patch 2.6.25-rc6 3/7] pci_choose_state() cleanup and fixes

[David Brownell]

On Saturday 22 March 2008, Rafael J. Wysocki wrote:
> 
> > It seems to me that your objection boils down to the concern
> > that those drivers may just have pushed their bug out a level,
> > rather than actually fixing their bugs.
> 
> Or, the authors of the drivers looked at what the code in
> pci_choose_state() actually did and used it on that basis.

That sounds like *exactly* the same thing.

Re: [patch 2.6.25-rc6 3/7] pci_choose_state() cleanup and fixes

[Rafael J. Wysocki]

On Saturday, 22 of March 2008, David Brownell wrote:
> On Friday 21 March 2008, Rafael J. Wysocki wrote:
> > On Friday, 21 of March 2008, David Brownell wrote:
> > > All it does is implement the rules that have been defined for
> > > ages now:  most of the pm_message_t transitions should not
> > > change device power states.
> > 
> > But they do at the moment, so you're going to change how the code
> > currently works on a quite large scale.
> 
> Are you arguing that this bug "should not be fixed"?
> 
> Or are you arguing that there's something wrong with
> the rules, so that they "should not be followed"?
> 
> Or something else?

Something else.  As I said before somewhere, I'd introduce a new function
imilar to pci_choose_state(), but with the semantics you want it to have.
Then, I'd make drivers switch to it and remove the original pci_choose_state()
when it's no longer used.  Clean and safe.

> > > > I'd make it return PCI_D3hot if platform_pci_choose_state()
> > > > is undefined or fails 
> > > 
> > > See above:  this implements the current rules, which say
> > > that in most cases devices shoudn't change powerstates.
> > 
> > Okay, say you're changing pci_choose_state() to follow the
> > documentation. 
> 
> Not just documentation -- the current architecture of the
> whole suspend process.  Surely it's essential not to have
> bugs like those at the core of that process?
> 
> If that process is broken, but this PCI bug hides it, we'll
> be having a boatload of unexpected trouble in a while...

I'm not against fixing bugs, but let's do that without giving users and testers
a hard time if possible.

> > Are you sure, however, that it won't cause any regressions to appear?
> 
> No more than any other bugfix turning up other latent bugs.
> That's why we have a process which lets fixes cook for a while
> before they merge.

In fact if a bugfix turns up any later bugs, then we should fix all of the
later bugs along with that bugfix, preferably in one series of patches.

> > > +	switch (mesg.event) {
> > >  	case PM_EVENT_SUSPEND:
> > >  	case PM_EVENT_HIBERNATE:
> > > -		return PCI_D3hot;
> > > +		/* NOTE:  platform_pci_choose_state() should only return
> > > +		 * states where wakeup won't work if
> > > +		 *   - !device_may_wakeup(&dev->dev), or
> > > +		 *   - dev can't wake from the target system state
> > > +		 */
> > > +		if (platform_pci_choose_state) {
> > > +			ret = platform_pci_choose_state(dev, mesg);
> > > +			if (ret == PCI_POWER_ERROR)
> > > +				ret = PCI_D3hot;
> > > +			else if ((ret == PCI_D1 || ret == PCI_D2)
> > > +					&& pci_no_d1d2(dev))
> > > +				ret = PCI_D3hot;
> > > +			break;
> > > +		}
> > > +		/* D3hot works, but may be suboptimal */
> > > +		ret = PCI_D3hot;
> > > +		break;
> > 
> > I would do:
> > 
> > +		if (platform_pci_choose_state) {
> > +			ret = platform_pci_choose_state(dev, mesg);
> > +			if (ret == PCI_POWER_ERROR || (pci_no_d1d2(dev)
> > +			    && (ret == PCI_D1 || ret == PCI_D2)))
> 
> That's ... phenomenally ugly and incomprehensible! It hides 
> almost the entire structure of the conditionals.

I obviously disagree with that.  What exactly is incomprehensible in it?
"Return D3hot if an error has been returned or a state we can't use has been
returned".  Quite simple, no?

> > +				ret = PCI_D3hot;
> > +		} else {
> > +			/* D3hot works, but may be suboptimal */
> > +			ret = PCI_D3hot;
> > +		}
> 
> Extra/pointless brackets after "else" ...

Please have a look at Chapter 3 in Documentation/CodingStyle (just before 3.1).

> so you think the quickie fix should have removed that first "break"?  Simpler
> to just have said so.
> 
> > +		break;
> 
> 
> ======== CUT HERE
> Clean up pci_choose_state():
> 
>  - pci_choose_state() should only return PCI_D0, unless the system is
>    entering a suspend (or hibernate) system state.
> 
>  - Only use platform_pci_choose_state() when entering a suspend
>    state ... and avoid PCI_D1 and PCI_D2 when appropriate.
> 
>  - Corrrect kerneldoc.
> 
> Note that for now only ACPI provides platform_pci_choose_state(), so
> this could be a minor change in behavior on some non-PC systems:  it
> avoids D3 except in the final stage of hibernation.
> 
> Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
> ---
>  drivers/pci/pci.c |   53 ++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 30 insertions(+), 23 deletions(-)
> 
> --- g26.orig/drivers/pci/pci.c	2008-03-22 10:25:48.000000000 -0700
> +++ g26/drivers/pci/pci.c	2008-03-22 10:44:28.000000000 -0700
> @@ -523,46 +523,53 @@ pci_set_power_state(struct pci_dev *dev,
>  }
>  
>  pci_power_t (*platform_pci_choose_state)(struct pci_dev *dev, pm_message_t state);
> - 
> +
>  /**
>   * pci_choose_state - Choose the power state of a PCI device
>   * @dev: PCI device to be suspended
> - * @state: target sleep state for the whole system. This is the value
> - *	that is passed to suspend() function.
> + * @mesg: value passed to suspend() function.
>   *
>   * Returns PCI power state suitable for given device and given system
> - * message.
> + * power state transition.
>   */
> -
> -pci_power_t pci_choose_state(struct pci_dev *dev, pm_message_t state)
> +pci_power_t pci_choose_state(struct pci_dev *dev, pm_message_t mesg)
>  {
>  	pci_power_t ret;
>  
> +	/* PCI legacy PM? */
>  	if (!pci_find_capability(dev, PCI_CAP_ID_PM))
>  		return PCI_D0;
>  
> -	if (platform_pci_choose_state) {
> -		ret = platform_pci_choose_state(dev, state);
> -		if (ret != PCI_POWER_ERROR)
> -			return ret;
> -	}
> -
> -	switch (state.event) {
> -	case PM_EVENT_ON:
> -		return PCI_D0;
> -	case PM_EVENT_FREEZE:
> -	case PM_EVENT_PRETHAW:
> -		/* REVISIT both freeze and pre-thaw "should" use D0 */
> +	switch (mesg.event) {
>  	case PM_EVENT_SUSPEND:
>  	case PM_EVENT_HIBERNATE:
> -		return PCI_D3hot;
> +		/*
> +		 * D3hot works on all devices, but may be suboptimal on
> +		 * a given system.  Factors include wakeups, power use,
> +		 * the optional D3hot->D0 reset, latency, and more.
> +		 *
> +		 * If there's a platform_pci_choose_state(), it could
> +		 * provide better answers for this system.  It should
> +		 * only return states where wakeup won't work if:
> +		 *   - !device_may_wakeup(&dev->dev), or
> +		 *   - dev can't wake from the target system state
> +		 */
> +		ret = PCI_D3hot;
> +		if (platform_pci_choose_state) {
> +			ret = platform_pci_choose_state(dev, mesg);
> +			if (ret == PCI_POWER_ERROR)
> +				ret = PCI_D3hot;
> +			else if ((ret == PCI_D1 || ret == PCI_D2)
> +					&& pci_no_d1d2(dev))
> +				ret = PCI_D3hot;

And so this reads "If an error is has been returned, return D3hot.  And by
the way, if D1 or D2 has been returned and we can't use any of them,
return D3hot".  I really don't see why it's better than just one condition, but
whatever.

> +		}
> +		break;
>  	default:
> -		printk("Unrecognized suspend event %d\n", state.event);
> -		BUG();
> +		ret = PCI_D0;
> +		break;
>  	}
> -	return PCI_D0;
> +	return ret;
>  }
> -
>  EXPORT_SYMBOL(pci_choose_state);
>  
>  static int pci_save_pcie_state(struct pci_dev *dev)
> --

Thanks,
Rafael

Re: [linux-pm] [patch 2.6.25-rc6 2/7] acpi_pm_device_sleep_state() cleanup

[Pavel Machek]

On Thu 2008-03-20 14:10:57, David Brownell wrote:
> Get rid of a superfluous acpi_pm_device_sleep_state() parameter.  The
> only legitimate value of that parameter must be derived from the first
> parameter, which is what all the callers already do.  (However, this
> does not address the fact that ACPI still doesn't set up those flags.)
> 
> Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>

ACK, FWIW.

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [patch 2.6.25-rc6 3/7] pci_choose_state() cleanup and fixes

[David Brownell]

Just to wrap this up ... if this #3 isn't going to merge,
then #4 can't merge either (teaching USB to use that call
to talk to ACPI etc).

See below.

However, several of the other patches in that series should
still IMO go upstream... I think I saw only one of them get
into the ACPI tree.


On Saturday 22 March 2008, Rafael J. Wysocki wrote:
> On Saturday, 22 of March 2008, David Brownell wrote:
> > On Friday 21 March 2008, Rafael J. Wysocki wrote:
> > > On Friday, 21 of March 2008, David Brownell wrote:
> > > > All it does is implement the rules that have been defined for
> > > > ages now:  most of the pm_message_t transitions should not
> > > > change device power states.
> > > 
> > > But they do at the moment, so you're going to change how the code
> > > currently works on a quite large scale.
> > 
> > Are you arguing that this bug "should not be fixed"?
> > 
> > Or are you arguing that there's something wrong with
> > the rules, so that they "should not be followed"?
> > 
> > Or something else?
> 
> Something else.  As I said before somewhere, I'd introduce a new function
> imilar to pci_choose_state(), but with the semantics you want it to have.
> Then, I'd make drivers switch to it and remove the original pci_choose_state()
> when it's no longer used.  Clean and safe.

The implementation of pci_choose_state() which first merged was
admittedly very broken.  That bothered me a lot ... since it
didn't adhere to my original proposal for such a function.

So while there's a part of me that would rather see my original
proposal working, rather than this broken version which reused
that name ... I can certainly live with some *other* function
doing the right thing, instead.


> > > > > I'd make it return PCI_D3hot if platform_pci_choose_state()
> > > > > is undefined or fails 
> > > > 
> > > > See above:  this implements the current rules, which say
> > > > that in most cases devices shoudn't change powerstates.
> > > 
> > > Okay, say you're changing pci_choose_state() to follow the
> > > documentation. 
> > 
> > Not just documentation -- the current architecture of the
> > whole suspend process.  Surely it's essential not to have
> > bugs like those at the core of that process?
> > 
> > If that process is broken, but this PCI bug hides it, we'll
> > be having a boatload of unexpected trouble in a while...
> 
> I'm not against fixing bugs, but let's do that without giving
> users and testers a hard time if possible.

I'm opposed to making design bugs in the first place.  I was
quite surprised to see so many functions got changed to use
the broken pci_choose_state() thing!


> > > Are you sure, however, that it won't cause any regressions to appear?
> > 
> > No more than any other bugfix turning up other latent bugs.
> > That's why we have a process which lets fixes cook for a while
> > before they merge.
> 
> In fact if a bugfix turns up any later bugs, then we should fix all of the
> later bugs along with that bugfix, preferably in one series of patches.

Fine.


> > > > +	switch (mesg.event) {
> > > >  	case PM_EVENT_SUSPEND:
> > > >  	case PM_EVENT_HIBERNATE:
> > > > -		return PCI_D3hot;
> > > > +		/* NOTE:  platform_pci_choose_state() should only return
> > > > +		 * states where wakeup won't work if
> > > > +		 *   - !device_may_wakeup(&dev->dev), or
> > > > +		 *   - dev can't wake from the target system state
> > > > +		 */
> > > > +		if (platform_pci_choose_state) {
> > > > +			ret = platform_pci_choose_state(dev, mesg);
> > > > +			if (ret == PCI_POWER_ERROR)
> > > > +				ret = PCI_D3hot;
> > > > +			else if ((ret == PCI_D1 || ret == PCI_D2)
> > > > +					&& pci_no_d1d2(dev))
> > > > +				ret = PCI_D3hot;
> > > > +			break;
> > > > +		}
> > > > +		/* D3hot works, but may be suboptimal */
> > > > +		ret = PCI_D3hot;
> > > > +		break;
> > > 
> > > I would do:
> > > 
> > > +		if (platform_pci_choose_state) {
> > > +			ret = platform_pci_choose_state(dev, mesg);
> > > +			if (ret == PCI_POWER_ERROR || (pci_no_d1d2(dev)
> > > +			    && (ret == PCI_D1 || ret == PCI_D2)))
> > 
> > That's ... phenomenally ugly and incomprehensible! It hides 
> > almost the entire structure of the conditionals.
> 
> I obviously disagree with that.  What exactly is incomprehensible in it?
> "Return D3hot if an error has been returned or a state we can't use has been
> returned".  Quite simple, no?

I've learned over time that complex conditions are *VERY* easily
misunderstood.  And that's pretty complex ... fortunately it has
no negation, but the previous version was simpler.  It clearly
split out the error condition from the "no D1/D2 support", and
that latter condition was easier to see (since it wasn't tangled
up with any unrelated logic).


 
> > > +				ret = PCI_D3hot;
> > > +		} else {
> > > +			/* D3hot works, but may be suboptimal */
> > > +			ret = PCI_D3hot;
> > > +		}
> > 
> > Extra/pointless brackets after "else" ...
> 
> Please have a look at Chapter 3 in Documentation/CodingStyle (just before 3.1).
> 
> > so you think the quickie fix should have removed that first "break"?  Simpler
> > to just have said so.
> > 
> > > +		break;
> > 
> > 
> > ======== CUT HERE
> > Clean up pci_choose_state():
> > 
> >  - pci_choose_state() should only return PCI_D0, unless the system is
> >    entering a suspend (or hibernate) system state.
> > 
> >  - Only use platform_pci_choose_state() when entering a suspend
> >    state ... and avoid PCI_D1 and PCI_D2 when appropriate.
> > 
> >  - Corrrect kerneldoc.
> > 
> > Note that for now only ACPI provides platform_pci_choose_state(), so
> > this could be a minor change in behavior on some non-PC systems:  it
> > avoids D3 except in the final stage of hibernation.
> > 
> > Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
> > ---
> >  drivers/pci/pci.c |   53 ++++++++++++++++++++++++++++++-----------------------
> >  1 file changed, 30 insertions(+), 23 deletions(-)
> > 
> > --- g26.orig/drivers/pci/pci.c	2008-03-22 10:25:48.000000000 -0700
> > +++ g26/drivers/pci/pci.c	2008-03-22 10:44:28.000000000 -0700
> > @@ -523,46 +523,53 @@ pci_set_power_state(struct pci_dev *dev,
> >  }
> >  
> >  pci_power_t (*platform_pci_choose_state)(struct pci_dev *dev, pm_message_t state);
> > - 
> > +
> >  /**
> >   * pci_choose_state - Choose the power state of a PCI device
> >   * @dev: PCI device to be suspended
> > - * @state: target sleep state for the whole system. This is the value
> > - *	that is passed to suspend() function.
> > + * @mesg: value passed to suspend() function.
> >   *
> >   * Returns PCI power state suitable for given device and given system
> > - * message.
> > + * power state transition.
> >   */
> > -
> > -pci_power_t pci_choose_state(struct pci_dev *dev, pm_message_t state)
> > +pci_power_t pci_choose_state(struct pci_dev *dev, pm_message_t mesg)
> >  {
> >  	pci_power_t ret;
> >  
> > +	/* PCI legacy PM? */
> >  	if (!pci_find_capability(dev, PCI_CAP_ID_PM))
> >  		return PCI_D0;
> >  
> > -	if (platform_pci_choose_state) {
> > -		ret = platform_pci_choose_state(dev, state);
> > -		if (ret != PCI_POWER_ERROR)
> > -			return ret;
> > -	}
> > -
> > -	switch (state.event) {
> > -	case PM_EVENT_ON:
> > -		return PCI_D0;
> > -	case PM_EVENT_FREEZE:
> > -	case PM_EVENT_PRETHAW:
> > -		/* REVISIT both freeze and pre-thaw "should" use D0 */
> > +	switch (mesg.event) {
> >  	case PM_EVENT_SUSPEND:
> >  	case PM_EVENT_HIBERNATE:
> > -		return PCI_D3hot;
> > +		/*
> > +		 * D3hot works on all devices, but may be suboptimal on
> > +		 * a given system.  Factors include wakeups, power use,
> > +		 * the optional D3hot->D0 reset, latency, and more.
> > +		 *
> > +		 * If there's a platform_pci_choose_state(), it could
> > +		 * provide better answers for this system.  It should
> > +		 * only return states where wakeup won't work if:
> > +		 *   - !device_may_wakeup(&dev->dev), or
> > +		 *   - dev can't wake from the target system state
> > +		 */
> > +		ret = PCI_D3hot;
> > +		if (platform_pci_choose_state) {
> > +			ret = platform_pci_choose_state(dev, mesg);
> > +			if (ret == PCI_POWER_ERROR)
> > +				ret = PCI_D3hot;
> > +			else if ((ret == PCI_D1 || ret == PCI_D2)
> > +					&& pci_no_d1d2(dev))
> > +				ret = PCI_D3hot;
> 
> And so this reads "If an error is has been returned, return D3hot.  And by
> the way, if D1 or D2 has been returned and we can't use any of them,
> return D3hot".  I really don't see why it's better than just one condition, but
> whatever.

As I said:  One big complex condition is harder to understand.
Particularly compared to two simple standalone conditions.


 
> > +		}
> > +		break;
> >  	default:
> > -		printk("Unrecognized suspend event %d\n", state.event);
> > -		BUG();
> > +		ret = PCI_D0;
> > +		break;
> >  	}
> > -	return PCI_D0;
> > +	return ret;
> >  }
> > -
> >  EXPORT_SYMBOL(pci_choose_state);
> >  
> >  static int pci_save_pcie_state(struct pci_dev *dev)
> > --
> 
> Thanks,
> Rafael
> 

[RESEND patch 2.6.25] acpi_pm_device_sleep_state() cleanup

[David Brownell]

From: David Brownell <dbrownell@users.sourceforge.net>

Get rid of a superfluous acpi_pm_device_sleep_state() parameter.  The
only legitimate value of that parameter must be derived from the first
parameter, which is what all the callers already do.  (However, this
does not address the fact that ACPI still doesn't set up those flags.)

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
Acked-by: Pavel Machek <pavel@ucw.cz>
---
 drivers/acpi/sleep/main.c  |    8 ++++----
 drivers/pci/pci-acpi.c     |    3 +--
 drivers/pnp/pnpacpi/core.c |    4 +---
 include/acpi/acpi_bus.h    |    4 ++--
 4 files changed, 8 insertions(+), 11 deletions(-)

--- g26.orig/drivers/acpi/sleep/main.c	2008-04-18 19:26:19.000000000 -0700
+++ g26/drivers/acpi/sleep/main.c	2008-04-18 20:08:23.000000000 -0700
@@ -369,8 +369,8 @@ int acpi_suspend(u32 acpi_state)
 /**
  *	acpi_pm_device_sleep_state - return preferred power state of ACPI device
  *		in the system sleep state given by %acpi_target_sleep_state
- *	@dev: device to examine
- *	@wake: if set, the device should be able to wake up the system
+ *	@dev: device to examine; its driver model wakeup flags control
+ *		whether it should be able to wake up the system
  *	@d_min_p: used to store the upper limit of allowed states range
  *	Return value: preferred power state of the device on success, -ENODEV on
  *		failure (ie. if there's no 'struct acpi_device' for @dev)
@@ -388,7 +388,7 @@ int acpi_suspend(u32 acpi_state)
  *	via @wake.
  */
 
-int acpi_pm_device_sleep_state(struct device *dev, int wake, int *d_min_p)
+int acpi_pm_device_sleep_state(struct device *dev, int *d_min_p)
 {
 	acpi_handle handle = DEVICE_ACPI_HANDLE(dev);
 	struct acpi_device *adev;
@@ -427,7 +427,7 @@ int acpi_pm_device_sleep_state(struct de
 	 * can wake the system.  _S0W may be valid, too.
 	 */
 	if (acpi_target_sleep_state == ACPI_STATE_S0 ||
-	    (wake && adev->wakeup.state.enabled &&
+	    (device_may_wakeup(dev) && adev->wakeup.state.enabled &&
 	     adev->wakeup.sleep_state <= acpi_target_sleep_state)) {
 		acpi_status status;
 
--- g26.orig/drivers/pci/pci-acpi.c	2008-04-18 19:26:19.000000000 -0700
+++ g26/drivers/pci/pci-acpi.c	2008-04-18 20:08:23.000000000 -0700
@@ -249,8 +249,7 @@ static pci_power_t acpi_pci_choose_state
 {
 	int acpi_state;
 
-	acpi_state = acpi_pm_device_sleep_state(&pdev->dev,
-		device_may_wakeup(&pdev->dev), NULL);
+	acpi_state = acpi_pm_device_sleep_state(&pdev->dev, NULL);
 	if (acpi_state < 0)
 		return PCI_POWER_ERROR;
 
--- g26.orig/drivers/pnp/pnpacpi/core.c	2008-04-18 19:26:19.000000000 -0700
+++ g26/drivers/pnp/pnpacpi/core.c	2008-04-18 20:08:23.000000000 -0700
@@ -132,9 +132,7 @@ static int pnpacpi_suspend(struct pnp_de
 {
 	int power_state;
 
-	power_state = acpi_pm_device_sleep_state(&dev->dev,
-						device_may_wakeup(&dev->dev),
-						NULL);
+	power_state = acpi_pm_device_sleep_state(&dev->dev, NULL);
 	if (power_state < 0)
 		power_state = (state.event == PM_EVENT_ON) ?
 				ACPI_STATE_D0 : ACPI_STATE_D3;
--- g26.orig/include/acpi/acpi_bus.h	2008-04-18 19:26:19.000000000 -0700
+++ g26/include/acpi/acpi_bus.h	2008-04-18 20:08:23.000000000 -0700
@@ -376,9 +376,9 @@ acpi_handle acpi_get_pci_rootbridge_hand
 #define DEVICE_ACPI_HANDLE(dev) ((acpi_handle)((dev)->archdata.acpi_handle))
 
 #ifdef CONFIG_PM_SLEEP
-int acpi_pm_device_sleep_state(struct device *, int, int *);
+int acpi_pm_device_sleep_state(struct device *, int *);
 #else /* !CONFIG_PM_SLEEP */
-static inline int acpi_pm_device_sleep_state(struct device *d, int w, int *p)
+static inline int acpi_pm_device_sleep_state(struct device *d, int *p)
 {
 	if (p)
 		*p = ACPI_STATE_D0;

[RESEND patch 2.6.25] ACPI sets up device.power.can_wakeup flags

[David Brownell]

This exports the "acpi_device.wakeup.flags.valid" flag to the driver
model as "device.power.can_wakeup".  That is, each "ACPI device" in
the /proc/acpi/wakeup file that corresponds to a "real" device will
initialize the can_wakeup flag to true.

A separate patch is needed to make ACPI pay attention to the userspace
input:  devices that can_wakeup have a should_wakeup flag, initially
true, which is settable via sysfs and tested using device_may_wakeup().
That's intended to be the primary policy input to the system, not the
ACPI-specific /proc/acpi/wakeup file.

Note also that the ACPI tables list only motherboard devices, but many
systems include more peripherals than those.  While the USB framework
handles USB peripherals, nothing in Linux currently sets those flags
for wakeup-capable devices on other expansion busses (like PCI).

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
NOTE:  this should probably not merge upstream until the patch to
make ACPI actually *use* these flags gets merged, since otherwise
these flags won't reflect actual behavior.

 drivers/acpi/button.c |    3 +++
 drivers/acpi/glue.c   |    1 +
 drivers/acpi/power.c  |    4 ++++
 3 files changed, 8 insertions(+)

--- g26.orig/drivers/acpi/button.c	2008-03-20 03:02:45.000000000 -0700
+++ g26/drivers/acpi/button.c	2008-03-20 03:27:24.000000000 -0700
@@ -476,6 +476,9 @@ static int acpi_button_add(struct acpi_d
 	if (button->type == ACPI_BUTTON_TYPE_LID)
 		acpi_lid_send_state(button);
 
+	/* for these devices the ACPI node *IS* the "physical" device */
+	device_init_wakeup(&device->dev, device->wakeup.flags.valid);
+
 	if (device->wakeup.flags.valid) {
 		/* Button's GPE is run-wake GPE */
 		acpi_set_gpe_type(device->wakeup.gpe_device,
--- g26.orig/drivers/acpi/glue.c	2008-03-20 03:27:11.000000000 -0700
+++ g26/drivers/acpi/glue.c	2008-03-20 03:27:25.000000000 -0700
@@ -162,6 +162,7 @@ static int acpi_bind_one(struct device *
 	if (!ACPI_FAILURE(status)) {
 		int ret;
 
+		device_init_wakeup(dev, acpi_dev->wakeup.flags.valid);
 		ret = sysfs_create_link(&dev->kobj, &acpi_dev->dev.kobj,
 				"firmware_node");
 		if (ret != 0)
--- g26.orig/drivers/acpi/power.c	2008-03-20 03:02:45.000000000 -0700
+++ g26/drivers/acpi/power.c	2008-03-20 03:27:25.000000000 -0700
@@ -313,6 +313,7 @@ int acpi_enable_wakeup_device_power(stru
 		ret = acpi_power_on(dev->wakeup.resources.handles[i], dev);
 		if (ret) {
 			printk(KERN_ERR PREFIX "Transition power state\n");
+			device_init_wakeup(&dev->dev, 0);
 			dev->wakeup.flags.valid = 0;
 			return -1;
 		}
@@ -322,6 +323,7 @@ int acpi_enable_wakeup_device_power(stru
 	status = acpi_evaluate_object(dev->handle, "_PSW", &arg_list, NULL);
 	if (ACPI_FAILURE(status) && (status != AE_NOT_FOUND)) {
 		printk(KERN_ERR PREFIX "Evaluate _PSW\n");
+		device_init_wakeup(&dev->dev, 0);
 		dev->wakeup.flags.valid = 0;
 		ret = -1;
 	}
@@ -351,6 +353,7 @@ int acpi_disable_wakeup_device_power(str
 	status = acpi_evaluate_object(dev->handle, "_PSW", &arg_list, NULL);
 	if (ACPI_FAILURE(status) && (status != AE_NOT_FOUND)) {
 		printk(KERN_ERR PREFIX "Evaluate _PSW\n");
+		device_init_wakeup(&dev->dev, 0);
 		dev->wakeup.flags.valid = 0;
 		return -1;
 	}
@@ -360,6 +363,7 @@ int acpi_disable_wakeup_device_power(str
 		ret = acpi_power_off_device(dev->wakeup.resources.handles[i], dev);
 		if (ret) {
 			printk(KERN_ERR PREFIX "Transition power state\n");
+			device_init_wakeup(&dev->dev, 0);
 			dev->wakeup.flags.valid = 0;
 			return -1;
 		}

[RESEND patch 2.6.25] ACPI uses device_may_wakeup() policy inputs

[David Brownell]

This imports the driver model device.power.may_wakeup flags to ACPI,
using it to *REPLACE* the /proc/acpi/wakeup flags for some devices.
It depends on the previous patch making device.power.can_wakeup behave.
It does that by:

 - Implementing platform_enable_wakeup(), which is currently invoked only
   by pci_enable_wake().  When that's called -- probably in the driver
   suspend() call -- it updates acpi_device.wakeup.state.enabled flag in
   the same way writing to /proc/acpi/wakeup updates it.
   
 - Updating the usage of the corresponding ACPI flags when turning on
   wakeup power domains and GPEs.

THIS PATCH NEEDS MORE ATTENTION because of the way the ACPI method
invocations have been changing, e.g. the 1.0 vs 2.0 sequencing.

Right now it's not clear to me whether the GPEs are always enabled at
the right time, and for that matter whether the rules haven't changed
so that drivers can no longer effectively control those settings from
suspend() unless acpi_new_pts_ordering is in effect.

ACPI systems see behavioral changes as follows:

   * Wakeup-aware PCI drivers no longer need to have someone override the
     initial system config by writing to /proc/acpi/wakeup; existing calls
     to pci_enable_wake() suffice.  For wakeup-unaware drivers, it's still
     not clearly safe to enable wakeup in /proc/acpi/wakeup.

   * Non-PCI frameworks (as for PS2 serial ports) could call the platform
     hook like PCI does, supporting wakeup-aware drivers.

   * The /sys/devices/.../power/wakeup flags are a different kind of manual
     override.  They _disable_ wakeup for broken hardware or drivers, rather
     than _enabling_ it in the hope that unmodified drivers won't break when
     their hardware triggers wakeup events.

NOT YET SIGNED-OFF ... primarily because of the confusion about
the order in which ACPI methods get called during entry to suspend
states.  Presumably one of the "new style" PM methods calls will
now always work for drivers wanting to enable wakeup methods...

---
 drivers/acpi/sleep/main.c   |    2 +-
 drivers/acpi/sleep/wakeup.c |   44 ++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 43 insertions(+), 3 deletions(-)

--- g26.orig/drivers/acpi/sleep/main.c	2008-02-24 02:07:57.000000000 -0800
+++ g26/drivers/acpi/sleep/main.c	2008-02-24 02:08:04.000000000 -0800
@@ -470,7 +470,7 @@ int acpi_pm_device_sleep_state(struct de
 	 * can wake the system.  _S0W may be valid, too.
 	 */
 	if (acpi_target_sleep_state == ACPI_STATE_S0 ||
-	    (device_may_wakeup(dev) && adev->wakeup.state.enabled &&
+	    (device_may_wakeup(dev) &&
 	     adev->wakeup.sleep_state <= acpi_target_sleep_state)) {
 		acpi_status status;
 
--- g26.orig/drivers/acpi/sleep/wakeup.c	2008-02-24 02:03:27.000000000 -0800
+++ g26/drivers/acpi/sleep/wakeup.c	2008-02-24 02:08:04.000000000 -0800
@@ -35,12 +35,22 @@ void acpi_enable_wakeup_device_prep(u8 s
 		struct acpi_device *dev = container_of(node,
 						       struct acpi_device,
 						       wakeup_list);
+		struct device *ldev;
 
 		if (!dev->wakeup.flags.valid ||
 		    !dev->wakeup.state.enabled ||
 		    (sleep_state > (u32) dev->wakeup.sleep_state))
 			continue;
 
+		ldev = acpi_get_physical_device(dev->handle);
+		if (ldev) {
+			int flag = device_may_wakeup(ldev);
+
+			put_device(ldev);
+			if (!flag)
+				continue;
+		}
+
 		spin_unlock(&acpi_device_lock);
 		acpi_enable_wakeup_device_power(dev);
 		spin_lock(&acpi_device_lock);
@@ -57,8 +67,8 @@ void acpi_enable_wakeup_device(u8 sleep_
 {
 	struct list_head *node, *next;
 
-	/* 
-	 * Caution: this routine must be invoked when interrupt is disabled 
+	/*
+	 * Caution: this routine must be invoked when interrupt is disabled
 	 * Refer ACPI2.0: P212
 	 */
 	ACPI_FUNCTION_TRACE("acpi_enable_wakeup_device");
@@ -107,6 +117,7 @@ void acpi_disable_wakeup_device(u8 sleep
 	list_for_each_safe(node, next, &acpi_wakeup_device_list) {
 		struct acpi_device *dev =
 			container_of(node, struct acpi_device, wakeup_list);
+		struct device *ldev;
 
 		if (!dev->wakeup.flags.valid)
 			continue;
@@ -125,6 +136,15 @@ void acpi_disable_wakeup_device(u8 sleep
 			continue;
 		}
 
+		ldev = acpi_get_physical_device(dev->handle);
+		if (ldev) {
+			int flag = device_may_wakeup(ldev);
+
+			put_device(ldev);
+			if (!flag)
+				continue;
+		}
+
 		spin_unlock(&acpi_device_lock);
 		acpi_disable_wakeup_device_power(dev);
 		/* Never disable run-wake GPE */
@@ -139,6 +159,24 @@ void acpi_disable_wakeup_device(u8 sleep
 	spin_unlock(&acpi_device_lock);
 }
 
+static int acpi_platform_enable_wakeup(struct device *dev, int is_on)
+{
+	struct acpi_device	*adev;
+	int			status;
+
+	if (!device_can_wakeup(dev))
+		return -EINVAL;
+	if (is_on && !device_may_wakeup(dev))
+		return -EINVAL;
+
+	status = acpi_bus_get_device(DEVICE_ACPI_HANDLE(dev), &adev);
+	if (status < 0)
+		return status;
+
+	adev->wakeup.state.enabled = !!is_on;
+	return 0;
+}
+
 static int __init acpi_wakeup_device_init(void)
 {
 	struct list_head *node, *next;
@@ -146,6 +184,8 @@ static int __init acpi_wakeup_device_ini
 	if (acpi_disabled)
 		return 0;
 
+	platform_enable_wakeup = acpi_platform_enable_wakeup;
+
 	spin_lock(&acpi_device_lock);
 	list_for_each_safe(node, next, &acpi_wakeup_device_list) {
 		struct acpi_device *dev = container_of(node,

Re: [RESEND patch 2.6.25] ACPI uses device_may_wakeup() policy inputs

[Zhang Rui]

On Fri, 2008-04-18 at 21:18 -0700, David Brownell wrote:
> This imports the driver model device.power.may_wakeup flags to ACPI,
> using it to *REPLACE* the /proc/acpi/wakeup flags for some devices.
> It depends on the previous patch making device.power.can_wakeup
> behave.
> It does that by:
> 
>  - Implementing platform_enable_wakeup(), which is currently invoked
> only
>    by pci_enable_wake().  When that's called -- probably in the driver
>    suspend() call -- it updates acpi_device.wakeup.state.enabled flag
> in
>    the same way writing to /proc/acpi/wakeup updates it.
>    
>  - Updating the usage of the corresponding ACPI flags when turning on
>    wakeup power domains and GPEs.
> 
> THIS PATCH NEEDS MORE ATTENTION because of the way the ACPI method
> invocations have been changing, e.g. the 1.0 vs 2.0 sequencing.
> 
> Right now it's not clear to me whether the GPEs are always enabled at
> the right time, and for that matter whether the rules haven't changed
> so that drivers can no longer effectively control those settings from
> suspend() unless acpi_new_pts_ordering is in effect.
Sorry. It's such a long sentence which is hard for me to understand. :(

> it's not clear to me whether the GPEs are always enabled at
> the right time
this patch doesn't change the time when GPEs are enabled.
when do you think should be the right time to enable the GPEs?

> 
> ACPI systems see behavioral changes as follows:
> 
>    * Wakeup-aware PCI drivers no longer need to have someone override
> the
>      initial system config by writing to /proc/acpi/wakeup; existing
> calls
>      to pci_enable_wake() suffice.  For wakeup-unaware drivers, it's
> still
>      not clearly safe to enable wakeup in /proc/acpi/wakeup.
> 
>    * Non-PCI frameworks (as for PS2 serial ports) could call the
> platform
>      hook like PCI does, supporting wakeup-aware drivers.
> 
>    * The /sys/devices/.../power/wakeup flags are a different kind of
> manual
>      override.  They _disable_ wakeup for broken hardware or drivers,
> rather
>      than _enabling_ it in the hope that unmodified drivers won't
> break when
>      their hardware triggers wakeup events.
> 
> NOT YET SIGNED-OFF ... primarily because of the confusion about
> the order in which ACPI methods get called during entry to suspend
> states.
I think it's safe to apply this patch.
But I don't know what's the relationship between this patch and the
"acpi_new_pts_ordering" stuff you stated earlier, do I miss something?

thanks,
rui

> Presumably one of the "new style" PM methods calls will
> now always work for drivers wanting to enable wakeup methods...

Re: [RESEND patch 2.6.25] ACPI sets up device.power.can_wakeup flags

[Zhang Rui]

On Fri, 2008-04-18 at 21:14 -0700, David Brownell wrote:
> This exports the "acpi_device.wakeup.flags.valid" flag to the driver
> model as "device.power.can_wakeup".  That is, each "ACPI device" in
> the /proc/acpi/wakeup file that corresponds to a "real" device will
> initialize the can_wakeup flag to true.
> 
> A separate patch is needed to make ACPI pay attention to the userspace
> input:  devices that can_wakeup have a should_wakeup flag, initially
> true, which is settable via sysfs and tested using device_may_wakeup().
> That's intended to be the primary policy input to the system, not the
> ACPI-specific /proc/acpi/wakeup file.
> 
> Note also that the ACPI tables list only motherboard devices, but many
> systems include more peripherals than those.  While the USB framework
> handles USB peripherals, nothing in Linux currently sets those flags
> for wakeup-capable devices on other expansion busses (like PCI).
> 
> Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
Signed-off-by: Zhang Rui <rui.zhang@intel.com>

> ---
> NOTE:  this should probably not merge upstream until the patch to
> make ACPI actually *use* these flags gets merged, since otherwise
> these flags won't reflect actual behavior.
> 
>  drivers/acpi/button.c |    3 +++
>  drivers/acpi/glue.c   |    1 +
>  drivers/acpi/power.c  |    4 ++++
>  3 files changed, 8 insertions(+)
> 
> --- g26.orig/drivers/acpi/button.c	2008-03-20 03:02:45.000000000 -0700
> +++ g26/drivers/acpi/button.c	2008-03-20 03:27:24.000000000 -0700
> @@ -476,6 +476,9 @@ static int acpi_button_add(struct acpi_d
>  	if (button->type == ACPI_BUTTON_TYPE_LID)
>  		acpi_lid_send_state(button);
>  
> +	/* for these devices the ACPI node *IS* the "physical" device */
> +	device_init_wakeup(&device->dev, device->wakeup.flags.valid);
> +
>  	if (device->wakeup.flags.valid) {
>  		/* Button's GPE is run-wake GPE */
>  		acpi_set_gpe_type(device->wakeup.gpe_device,
> --- g26.orig/drivers/acpi/glue.c	2008-03-20 03:27:11.000000000 -0700
> +++ g26/drivers/acpi/glue.c	2008-03-20 03:27:25.000000000 -0700
> @@ -162,6 +162,7 @@ static int acpi_bind_one(struct device *
>  	if (!ACPI_FAILURE(status)) {
>  		int ret;
>  
> +		device_init_wakeup(dev, acpi_dev->wakeup.flags.valid);
>  		ret = sysfs_create_link(&dev->kobj, &acpi_dev->dev.kobj,
>  				"firmware_node");
>  		if (ret != 0)
> --- g26.orig/drivers/acpi/power.c	2008-03-20 03:02:45.000000000 -0700
> +++ g26/drivers/acpi/power.c	2008-03-20 03:27:25.000000000 -0700
> @@ -313,6 +313,7 @@ int acpi_enable_wakeup_device_power(stru
>  		ret = acpi_power_on(dev->wakeup.resources.handles[i], dev);
>  		if (ret) {
>  			printk(KERN_ERR PREFIX "Transition power state\n");
> +			device_init_wakeup(&dev->dev, 0);
>  			dev->wakeup.flags.valid = 0;
>  			return -1;
>  		}
> @@ -322,6 +323,7 @@ int acpi_enable_wakeup_device_power(stru
>  	status = acpi_evaluate_object(dev->handle, "_PSW", &arg_list, NULL);
>  	if (ACPI_FAILURE(status) && (status != AE_NOT_FOUND)) {
>  		printk(KERN_ERR PREFIX "Evaluate _PSW\n");
> +		device_init_wakeup(&dev->dev, 0);
>  		dev->wakeup.flags.valid = 0;
>  		ret = -1;
>  	}
> @@ -351,6 +353,7 @@ int acpi_disable_wakeup_device_power(str
>  	status = acpi_evaluate_object(dev->handle, "_PSW", &arg_list, NULL);
>  	if (ACPI_FAILURE(status) && (status != AE_NOT_FOUND)) {
>  		printk(KERN_ERR PREFIX "Evaluate _PSW\n");
> +		device_init_wakeup(&dev->dev, 0);
>  		dev->wakeup.flags.valid = 0;
>  		return -1;
>  	}
> @@ -360,6 +363,7 @@ int acpi_disable_wakeup_device_power(str
>  		ret = acpi_power_off_device(dev->wakeup.resources.handles[i], dev);
>  		if (ret) {
>  			printk(KERN_ERR PREFIX "Transition power state\n");
> +			device_init_wakeup(&dev->dev, 0);
>  			dev->wakeup.flags.valid = 0;
>  			return -1;
>  		}
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RESEND patch 2.6.25] ACPI uses device_may_wakeup() policy inputs

[Zhao Yakui]

On Fri, 2008-04-18 at 21:18 -0700, David Brownell wrote:
> This imports the driver model device.power.may_wakeup flags to ACPI,
> using it to *REPLACE* the /proc/acpi/wakeup flags for some devices.
> It depends on the previous patch making device.power.can_wakeup behave.
> It does that by:
> 
>  - Implementing platform_enable_wakeup(), which is currently invoked only
>    by pci_enable_wake().  When that's called -- probably in the driver
>    suspend() call -- it updates acpi_device.wakeup.state.enabled flag in
>    the same way writing to /proc/acpi/wakeup updates it.
>    
>  - Updating the usage of the corresponding ACPI flags when turning on
>    wakeup power domains and GPEs.
> 
> THIS PATCH NEEDS MORE ATTENTION because of the way the ACPI method
> invocations have been changing, e.g. the 1.0 vs 2.0 sequencing.
> 
> Right now it's not clear to me whether the GPEs are always enabled at
> the right time, and for that matter whether the rules haven't changed
> so that drivers can no longer effectively control those settings from
> suspend() unless acpi_new_pts_ordering is in effect.
Agree with what Rui said. The patch doesn't change the time when GPEs
are enabled/disabled.

But after the patch is applied, some PCI device(the ACPI device with the
_PRW object) can wake the sleeping system by default. And it is totally
opposite to the current flowchart. 

> ACPI systems see behavioral changes as follows:
> 
>    * Wakeup-aware PCI drivers no longer need to have someone override the
>      initial system config by writing to /proc/acpi/wakeup; existing calls
>      to pci_enable_wake() suffice.  For wakeup-unaware drivers, it's still
>      not clearly safe to enable wakeup in /proc/acpi/wakeup.
> 
>    * Non-PCI frameworks (as for PS2 serial ports) could call the platform
>      hook like PCI does, supporting wakeup-aware drivers.
> 
>    * The /sys/devices/.../power/wakeup flags are a different kind of manual
>      override.  They _disable_ wakeup for broken hardware or drivers, rather
>      than _enabling_ it in the hope that unmodified drivers won't break when
>      their hardware triggers wakeup events.
> 
> NOT YET SIGNED-OFF ... primarily because of the confusion about
> the order in which ACPI methods get called during entry to suspend
> states.  Presumably one of the "new style" PM methods calls will
> now always work for drivers wanting to enable wakeup methods...
> 
> ---
>  drivers/acpi/sleep/main.c   |    2 +-
>  drivers/acpi/sleep/wakeup.c |   44 ++++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 43 insertions(+), 3 deletions(-)
> 
> --- g26.orig/drivers/acpi/sleep/main.c	2008-02-24 02:07:57.000000000 -0800
> +++ g26/drivers/acpi/sleep/main.c	2008-02-24 02:08:04.000000000 -0800
> @@ -470,7 +470,7 @@ int acpi_pm_device_sleep_state(struct de
>  	 * can wake the system.  _S0W may be valid, too.
>  	 */
>  	if (acpi_target_sleep_state == ACPI_STATE_S0 ||
> -	    (device_may_wakeup(dev) && adev->wakeup.state.enabled &&
> +	    (device_may_wakeup(dev) &&
>  	     adev->wakeup.sleep_state <= acpi_target_sleep_state)) {
>  		acpi_status status;
>  
> --- g26.orig/drivers/acpi/sleep/wakeup.c	2008-02-24 02:03:27.000000000 -0800
> +++ g26/drivers/acpi/sleep/wakeup.c	2008-02-24 02:08:04.000000000 -0800
> @@ -35,12 +35,22 @@ void acpi_enable_wakeup_device_prep(u8 s
>  		struct acpi_device *dev = container_of(node,
>  						       struct acpi_device,
>  						       wakeup_list);
> +		struct device *ldev;
>  
>  		if (!dev->wakeup.flags.valid ||
>  		    !dev->wakeup.state.enabled ||
>  		    (sleep_state > (u32) dev->wakeup.sleep_state))
>  			continue;
>  
> +		ldev = acpi_get_physical_device(dev->handle);
> +		if (ldev) {
> +			int flag = device_may_wakeup(ldev);
> +
> +			put_device(ldev);
> +			if (!flag)
> +				continue;
> +		}
> +
>  		spin_unlock(&acpi_device_lock);
>  		acpi_enable_wakeup_device_power(dev);
>  		spin_lock(&acpi_device_lock);
> @@ -57,8 +67,8 @@ void acpi_enable_wakeup_device(u8 sleep_
>  {
>  	struct list_head *node, *next;
>  
> -	/* 
> -	 * Caution: this routine must be invoked when interrupt is disabled 
> +	/*
> +	 * Caution: this routine must be invoked when interrupt is disabled
>  	 * Refer ACPI2.0: P212
>  	 */
>  	ACPI_FUNCTION_TRACE("acpi_enable_wakeup_device");
> @@ -107,6 +117,7 @@ void acpi_disable_wakeup_device(u8 sleep
>  	list_for_each_safe(node, next, &acpi_wakeup_device_list) {
>  		struct acpi_device *dev =
>  			container_of(node, struct acpi_device, wakeup_list);
> +		struct device *ldev;
>  
>  		if (!dev->wakeup.flags.valid)
>  			continue;
> @@ -125,6 +136,15 @@ void acpi_disable_wakeup_device(u8 sleep
>  			continue;
>  		}
>  
> +		ldev = acpi_get_physical_device(dev->handle);
> +		if (ldev) {
> +			int flag = device_may_wakeup(ldev);
> +
> +			put_device(ldev);
> +			if (!flag)
> +				continue;
> +		}
> +
>  		spin_unlock(&acpi_device_lock);
>  		acpi_disable_wakeup_device_power(dev);
>  		/* Never disable run-wake GPE */
> @@ -139,6 +159,24 @@ void acpi_disable_wakeup_device(u8 sleep
>  	spin_unlock(&acpi_device_lock);
>  }
>  
> +static int acpi_platform_enable_wakeup(struct device *dev, int is_on)
> +{
> +	struct acpi_device	*adev;
> +	int			status;
> +
> +	if (!device_can_wakeup(dev))
> +		return -EINVAL;
> +	if (is_on && !device_may_wakeup(dev))
> +		return -EINVAL;
> +
> +	status = acpi_bus_get_device(DEVICE_ACPI_HANDLE(dev), &adev);
> +	if (status < 0)
> +		return status;
> +
> +	adev->wakeup.state.enabled = !!is_on;
> +	return 0;
> +}
> +
>  static int __init acpi_wakeup_device_init(void)
>  {
>  	struct list_head *node, *next;
> @@ -146,6 +184,8 @@ static int __init acpi_wakeup_device_ini
>  	if (acpi_disabled)
>  		return 0;
>  
> +	platform_enable_wakeup = acpi_platform_enable_wakeup;
> +
>  	spin_lock(&acpi_device_lock);
>  	list_for_each_safe(node, next, &acpi_wakeup_device_list) {
>  		struct acpi_device *dev = container_of(node,
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RESEND patch 2.6.25] ACPI uses device_may_wakeup() policy inputs

[David Brownell]

On Monday 21 April 2008, Zhang Rui wrote:
> 
> On Fri, 2008-04-18 at 21:18 -0700, David Brownell wrote:
> > This imports the driver model device.power.may_wakeup flags to ACPI,
> > using it to *REPLACE* the /proc/acpi/wakeup flags for some devices.
> > It depends on the previous patch making device.power.can_wakeup
> > behave. It does that by:
> > 
> >  - Implementing platform_enable_wakeup(), which is currently invoked
> >    only by pci_enable_wake().  When that's called -- probably in the
> >    driver suspend() call -- it updates acpi_device.wakeup.state.enabled
> >    flag in the same way writing to /proc/acpi/wakeup updates it.
> >    
> >  - Updating the usage of the corresponding ACPI flags when turning on
> >    wakeup power domains and GPEs.
> > 
> > THIS PATCH NEEDS MORE ATTENTION because of the way the ACPI method
> > invocations have been changing, e.g. the 1.0 vs 2.0 sequencing.
> > 
> > Right now it's not clear to me whether the GPEs are always enabled at
> > the right time, and for that matter whether the rules haven't changed
> > so that drivers can no longer effectively control those settings from
> > suspend() unless acpi_new_pts_ordering is in effect.
>
> Sorry. It's such a long sentence which is hard for me to understand. :(

Apologies.

On the bright side ... didn't all the new_pts_ordering stuff
get removed?  If that stays gone, it removes the main concern
I had.  That comment was written when I observed what looked
to be troublesome semantic changes from that "new" ordering.


> > it's not clear to me whether the GPEs are always enabled at
> > the right time
>  
> this patch doesn't change the time when GPEs are enabled.

No it doesn't.  Maybe I'm just more paranoid about it than
someone who knows ACPI (and its version-specific issues) a
lot better than me.


> > NOT YET SIGNED-OFF ... primarily because of the confusion about
> > the order in which ACPI methods get called during entry to suspend
> > states.
>
> I think it's safe to apply this patch.

I did this work before the "new_pts_ordering" stuff happened.  Then
after "new_pts_ordering", it looked a bit problematic ... originally,
I would have agreed with you.  Maybe now I can agree again.

- Dave



> thanks,
> rui
> 
> > Presumably one of the "new style" PM methods calls will
> > now always work for drivers wanting to enable wakeup methods...
> 

Re: [RESEND patch 2.6.25] ACPI uses device_may_wakeup() policy inputs

[David Brownell]

On Tuesday 22 April 2008, Zhao Yakui wrote:
> Agree with what Rui said. The patch doesn't change the time when GPEs
> are enabled/disabled.

So you also think my worries were unfounded?  OK, I'm happy
to hear that.  It means the userspace model for wakeup events
on PCs can become one that non-ACPI platforms can use too.

In which case ... I'll resend this patch with a more concise
summary and a signed-off-by line.


> But after the patch is applied, some PCI device(the ACPI device with the
> _PRW object) can wake the sleeping system by default. And it is totally
> opposite to the current flowchart. 

That's not true.  Behavior could only change for devices with
drivers which already call pci_enable_wake()!

What's different is that /proc/acpi/wakeup is being taken
partially out of the decision loop ... in favor of (a) driver
model flags, which work on platforms without ACPI, and also
(b) device driver logic, which in any case really needs to
be prepared to request and otherwise manage the wake events.

If someone sets /proc/acpi/wakeup flags for a device, it
could still be made to issue wake events for devices with
drivers that don't expect (or handle) those events.

- Dave

Re: [RESEND patch 2.6.25] ACPI uses device_may_wakeup() policy inputs

[Zhang Rui]

On Mon, 2008-04-28 at 20:48 +0800, Zhao Yakui wrote:
> On Sat, 2008-04-26 at 12:37 -0700, David Brownell wrote:
> > On Tuesday 22 April 2008, Zhao Yakui wrote:
> > > Agree with what Rui said. The patch doesn't change the time when GPEs
> > > are enabled/disabled.
> > 
> > So you also think my worries were unfounded?  OK, I'm happy
> > to hear that.  It means the userspace model for wakeup events
> > on PCs can become one that non-ACPI platforms can use too.
> > 
> > In which case ... I'll resend this patch with a more concise
> > summary and a signed-off-by line.
> > 
> > 
> > > But after the patch is applied, some PCI device(the ACPI device with the
> > > _PRW object) can wake the sleeping system by default. And it is totally
> > > opposite to the current flowchart. 
> > 
> > That's not true.  Behavior could only change for devices with
> > drivers which already call pci_enable_wake()!
> Yes. But in fact a lot of PCI device will call pci_enable_wake when the
> system enters the suspend state. And the behaviour will be changed.
> > What's different is that /proc/acpi/wakeup is being taken
> > partially out of the decision loop ... in favor of (a) driver
> > model flags, which work on platforms without ACPI, and also
> > (b) device driver logic, which in any case really needs to
> > be prepared to request and otherwise manage the wake events.
> > 
> Yes. What your said is right. If your patch is applied, whether the PCI
> device can wake the sleeping system is controlled by device driver.

This may cause some "regressions", something like system resumes
immediately after enter S3. This should rather be a driver/device
problem which rises a wake up event when it's suspend.
The right thing to do is to disable the wake up flag for this device.
The patch was dropped for this reason last time it's merged and we
should take good care of it this time. :)

Dave,
Maybe we should state this in the change log?

thanks,
rui

Re: [RESEND patch 2.6.25] ACPI uses device_may_wakeup() policy inputs

[Zhao Yakui]

On Sat, 2008-04-26 at 12:37 -0700, David Brownell wrote:
> On Tuesday 22 April 2008, Zhao Yakui wrote:
> > Agree with what Rui said. The patch doesn't change the time when GPEs
> > are enabled/disabled.
> 
> So you also think my worries were unfounded?  OK, I'm happy
> to hear that.  It means the userspace model for wakeup events
> on PCs can become one that non-ACPI platforms can use too.
> 
> In which case ... I'll resend this patch with a more concise
> summary and a signed-off-by line.
> 
> 
> > But after the patch is applied, some PCI device(the ACPI device with the
> > _PRW object) can wake the sleeping system by default. And it is totally
> > opposite to the current flowchart. 
> 
> That's not true.  Behavior could only change for devices with
> drivers which already call pci_enable_wake()!
Yes. But in fact a lot of PCI device will call pci_enable_wake when the
system enters the suspend state. And the behaviour will be changed.
> What's different is that /proc/acpi/wakeup is being taken
> partially out of the decision loop ... in favor of (a) driver
> model flags, which work on platforms without ACPI, and also
> (b) device driver logic, which in any case really needs to
> be prepared to request and otherwise manage the wake events.
> 
Yes. What your said is right. If your patch is applied, whether the PCI
device can wake the sleeping system is controlled by device driver. In
such case the /proc/acpi/wakeup will be useless.

> If someone sets /proc/acpi/wakeup flags for a device, it
> could still be made to issue wake events for devices with
> drivers that don't expect (or handle) those events.
> 
> - Dave

Re: [linux-pm] [RESEND patch 2.6.25] ACPI uses device_may_wakeup() policy inputs

[Alan Stern]

On Mon, 28 Apr 2008, Zhang Rui wrote:

> This may cause some "regressions", something like system resumes
> immediately after enter S3. This should rather be a driver/device
> problem which rises a wake up event when it's suspend.
> The right thing to do is to disable the wake up flag for this device.
> The patch was dropped for this reason last time it's merged and we
> should take good care of it this time. :)

Indeed, it's easy to imagine a situation where somebody suspends their 
laptop and then unplugs a USB mouse, thereby causing a wakeup event.

I suspect many PCI devices should be disabled for remote wakeup during 
system sleeps.

Alan Stern

Re: [RESEND patch 2.6.25] ACPI uses device_may_wakeup() policy inputs

[Henrique de Moraes Holschuh]

On Mon, 28 Apr 2008, Zhao Yakui wrote:
> Yes. What your said is right. If your patch is applied, whether the PCI
> device can wake the sleeping system is controlled by device driver. In
> such case the /proc/acpi/wakeup will be useless.

Which calls for a new standard interface to let userpace control that,
doesn't it?  Issues with wake-up devices are not uncommon, the user
really has to have a way to configure that, and IMHO it better be a
generic one...

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [RESEND patch 2.6.25] ACPI uses device_may_wakeup() policy inputs

[David Brownell]

On Monday 28 April 2008, Henrique de Moraes Holschuh wrote:
> On Mon, 28 Apr 2008, Zhao Yakui wrote:
> > Yes. What your said is right. If your patch is applied, whether the PCI
> > device can wake the sleeping system is controlled by device driver. In
> > such case the /proc/acpi/wakeup will be useless.
> 
> Which calls for a new standard interface to let userpace control that,
> doesn't it?  Issues with wake-up devices are not uncommon, the user
> really has to have a way to configure that, and IMHO it better be a
> generic one...

That "new standard interface" has been in the kernel for quite
some time now:  /sys/devices/.../power/wakeup attributes, which
default to "enabled" for wake-capable devices.  Set to "disabled"
if it's not behaving on your system for some reason.  It's part
of the driver model infrastructure.

These patches have been nudging ACPI switch over to that generic
mechanism, instead of needing an ACPI-specific one... first posted
on the order of 18 months ago, FWIW.


Note that there are (broadly) two categories of issue here.  One
is with the hardware/firmware ... e.g. ACPI, on systems where that
exists.  (By far the minority in terms of Linux platforms, and maybe
in terms of individual systems too, despite the fact that most of
us developers run x86 Linux somewhere.)  The other is inside the driver
itself:  does it arrange for the right things to be wake events, and
handle them OK.

When I last did a survey of what drivers use PCI wakeup mechanisms,
the result was pretty weak:  a handful of network drivers, and the
USB host controllers.  Such support has been, if anything, declining
since we acquired "pm_message_t".  But I believe that most of those
drivers actually behave, at least on non-ACPI systems.

Try "cd linux-2.6; grep -lr pci_enable_wake *" for current results...

- Dave

Re: [RESEND patch 2.6.25] ACPI uses device_may_wakeup() policy inputs

[David Brownell]

On Monday 28 April 2008, Zhao Yakui wrote:
> 
> > > But after the patch is applied, some PCI device(the ACPI device with the
> > > _PRW object) can wake the sleeping system by default. And it is totally
> > > opposite to the current flowchart. 
> > 
> > That's not true.  Behavior could only change for devices with
> > drivers which already call pci_enable_wake()!
> 
> Yes. But in fact a lot of PCI device will call pci_enable_wake when the
> system enters the suspend state. And the behaviour will be changed.

I'm not sure what your point is.  That "behavior change" is a bugfix.
The previous behavior was specific to ACPI, and obviously was not what
the driver writer expected.

And it's *NOT* how things work on non-ACPI systems which have both PCI
and functional system suspend/resume.  (Various PowerPC, MIPS, and ARM
systems meet those requirements, as I understand.)

I sympathize with the desire to avoid driver-related surprises.  In
this case, it's trivial to disable that behavior by modifyi

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

Re: [RESEND patch 2.6.25] ACPI uses device_may_wakeup() policy inputs

[David Brownell]

[ grr, mousepad sent an incomplete message for me ... ]

On Monday 28 April 2008, Zhao Yakui wrote:
> 
> > > But after the patch is applied, some PCI device(the ACPI device with the
> > > _PRW object) can wake the sleeping system by default. And it is totally
> > > opposite to the current flowchart. 
> > 
> > That's not true.  Behavior could only change for devices with
> > drivers which already call pci_enable_wake()!
> 
> Yes. But in fact a lot of PCI device will call pci_enable_wake when the
> system enters the suspend state. And the behaviour will be changed.

I'm not sure what your point is.  That "behavior change" is a bugfix.
The previous behavior was specific to ACPI, and obviously was not what
the driver writer expected.

And it's *NOT* how things work on non-ACPI systems which have both PCI
and functional system suspend/resume.  (Various PowerPC, MIPS, and ARM
systems meet those requirements, as I understand.)

I sympathize with the desire to avoid driver-related surprises.  In
this case, it's trivial to disable that behavior using sysfs.  And I
believe you'll observe that not many drivers could actually turn up
surprises here:  who calls pci_enable_wake()?

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

Re: [RESEND patch 2.6.25] ACPI uses device_may_wakeup() policy inputs

[David Brownell]

On Monday 28 April 2008, Zhang Rui wrote:
> > > 
> > Yes. What your said is right. If your patch is applied, whether the PCI
> > device can wake the sleeping system is controlled by device driver.
> 
> This may cause some "regressions", something like system resumes
> immediately after enter S3. This should rather be a driver/device
> problem which rises a wake up event when it's suspend.
> The right thing to do is to disable the wake up flag for this device.
> The patch was dropped for this reason last time it's merged and we
> should take good care of it this time. :)
> 
> Dave,
> Maybe we should state this in the change log?

I already seem to have added that in the changelog for the last
version of the patch I sent ...

- dave

Re: [RESEND patch 2.6.25] ACPI uses device_may_wakeup() policy inputs

[Henrique de Moraes Holschuh]

On Mon, 28 Apr 2008, David Brownell wrote:
> > Which calls for a new standard interface to let userpace control that,
> > doesn't it?  Issues with wake-up devices are not uncommon, the user

[...]

> That "new standard interface" has been in the kernel for quite
> some time now:  /sys/devices/.../power/wakeup attributes, which
> default to "enabled" for wake-capable devices.  Set to "disabled"
> if it's not behaving on your system for some reason.  It's part
> of the driver model infrastructure.

Ok.  That answers my point.  Can we override the defaults with whatever
information ACPI might have about them?

I certainly would not expect (or appreciate) that this:

$ cat /proc/acpi/wakeup 
Device  S-state   Status   Sysfs node
LID       S3    *enabled   
SLPB      S3    *enabled   
UART      S3     disabled  pnp:00:09
EXP0      S4     disabled  pci:0000:00:1c.0
EXP1      S4     disabled  
EXP2      S4     disabled  pci:0000:00:1c.2
EXP3      S4     disabled  
PCI1      S4     disabled  pci:0000:00:1e.0
USB0      S3     disabled  pci:0000:00:1d.0
USB1      S3     disabled  pci:0000:00:1d.1
USB3      S3     disabled  pci:0000:00:1d.3
USB7      S3     disabled  pci:0000:00:1d.7
AC9M      S4     disabled  

(which is mostly following what I told the BIOS to do), to suddely have
more lines set to "enable" :-)

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [RESEND patch 2.6.25] ACPI uses device_may_wakeup() policy inputs

[David Brownell]

On Monday 28 April 2008, Henrique de Moraes Holschuh wrote:
> On Mon, 28 Apr 2008, David Brownell wrote:
> > > Which calls for a new standard interface to let userpace control that,
> > > doesn't it?  Issues with wake-up devices are not uncommon, the user
> 
> [...]
> 
> > That "new standard interface" has been in the kernel for quite
> > some time now:  /sys/devices/.../power/wakeup attributes, which
> > default to "enabled" for wake-capable devices.  Set to "disabled"
> > if it's not behaving on your system for some reason.  It's part
> > of the driver model infrastructure.
> 
> Ok.  That answers my point.  Can we override the defaults with whatever
> information ACPI might have about them?

Userspace could do that, for the subset of wakeup-capable
PCI devices known to both Linux and ACPI.

I personally would not want to have a second framework
for managing wakeup events though.  I want to move away
from the ACPI stuff as quickly as possible.  Fortunately,
most Linux users have no idea that ACPI could even wake
up the system ... since drivers generally don't handle it,
it's disabled by default, and no userspace tools enable it.


> I certainly would not expect (or appreciate) that this:
> 
> $ cat /proc/acpi/wakeup 
> Device  S-state   Status   Sysfs node
> LID       S3    *enabled   
> SLPB      S3    *enabled   
> UART      S3     disabled  pnp:00:09
> EXP0      S4     disabled  pci:0000:00:1c.0
> EXP1      S4     disabled  
> EXP2      S4     disabled  pci:0000:00:1c.2
> EXP3      S4     disabled  
> PCI1      S4     disabled  pci:0000:00:1e.0
> USB0      S3     disabled  pci:0000:00:1d.0
> USB1      S3     disabled  pci:0000:00:1d.1
> USB3      S3     disabled  pci:0000:00:1d.3
> USB7      S3     disabled  pci:0000:00:1d.7
> AC9M      S4     disabled  
> 
> (which is mostly following what I told the BIOS to do), to suddely have
> more lines set to "enable" :-)

Actually, every one of those particular lines is the default set
up by the Linux ACPI code:  "never use wakeup unless you must".
Not AFAICT from BIOS; most BIOS wakeup settings I've seen relate
to the RTC, or sometimes the keyboard.  (Neither of which shows
in that list above...)

I don't believe ACPI looks at any messages from BIOS there other
than "does device DDDD have a wakeup method".  And as a rule,
no userspace code will ever change those values from "disabled"...

That said, none of those lines are changed by these two patches.

- Dave

Re: [RESEND patch 2.6.25] ACPI uses device_may_wakeup() policy inputs

[David Brownell]

On Monday 28 April 2008, David Brownell wrote:

> When I last did a survey of what drivers use PCI wakeup mechanisms,
> the result was pretty weak:  a handful of network drivers, and the
> USB host controllers.  Such support has been, if anything, declining
> since we acquired "pm_message_t".  But I believe that most of those
> drivers actually behave, at least on non-ACPI systems.
> 
> Try "cd linux-2.6; grep -lr pci_enable_wake *" for current results...

linux-2.6$  grep -lr pci_enable_wake *
Documentation/power/devices.txt
Documentation/power/pci.txt
drivers/memstick/host/jmb38x_ms.c
drivers/message/fusion/mptbase.c
drivers/misc/tifm_7xx1.c
drivers/mmc/host/sdhci.c
drivers/net/3c59x.c
drivers/net/8139cp.c
drivers/net/amd8111e.c
drivers/net/atlx/atl1.c
drivers/net/e100.c
drivers/net/e1000/e1000_main.c
drivers/net/e1000e/netdev.c
drivers/net/forcedeth.c
drivers/net/igb/igb_main.c
drivers/net/ixgbe/ixgbe_main.c
drivers/net/r8169.c
drivers/net/skge.c
drivers/net/sky2.c
drivers/net/tulip/dmfe.c
drivers/net/tulip/uli526x.c
drivers/net/typhoon.c
drivers/net/via-rhine.c
drivers/net/via-velocity.c
drivers/net/wireless/airo.c
drivers/pci/pci.c
drivers/scsi/megaraid/megaraid_sas.c
drivers/scsi/nsp32.c
drivers/usb/core/hcd-pci.c
include/linux/pci.h
linux-2.6$ 

[RE-RESEND patch 2.6.25-git] acpi_pm_device_sleep_state() cleanup

[David Brownell]

From: David Brownell <dbrownell@users.sourceforge.net>

Get rid of a superfluous acpi_pm_device_sleep_state() parameter.  The
only legitimate value of that parameter must be derived from the first
parameter, which is what all the callers already do.  (However, this
does not address the fact that ACPI *still* doesn't set up those flags.)

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
Acked-by: Pavel Machek <pavel@ucw.cz>
---
It seems patches sent to the ACPI list are often ignored.  This patch
was sent against 2.6.25-rc6 for 2.6.26-rc0 merge, then resent 18-apr
against 2.6.25 ... maybe the third time can finally do the trick.

 drivers/acpi/sleep/main.c  |    8 ++++----
 drivers/pci/pci-acpi.c     |    3 +--
 drivers/pnp/pnpacpi/core.c |    4 +---
 include/acpi/acpi_bus.h    |    4 ++--
 4 files changed, 8 insertions(+), 11 deletions(-)

--- g26.orig/drivers/acpi/sleep/main.c	2008-04-18 19:26:19.000000000 -0700
+++ g26/drivers/acpi/sleep/main.c	2008-04-18 20:08:23.000000000 -0700
@@ -369,8 +369,8 @@ int acpi_suspend(u32 acpi_state)
 /**
  *	acpi_pm_device_sleep_state - return preferred power state of ACPI device
  *		in the system sleep state given by %acpi_target_sleep_state
- *	@dev: device to examine
- *	@wake: if set, the device should be able to wake up the system
+ *	@dev: device to examine; its driver model wakeup flags control
+ *		whether it should be able to wake up the system
  *	@d_min_p: used to store the upper limit of allowed states range
  *	Return value: preferred power state of the device on success, -ENODEV on
  *		failure (ie. if there's no 'struct acpi_device' for @dev)
@@ -388,7 +388,7 @@ int acpi_suspend(u32 acpi_state)
  *	via @wake.
  */
 
-int acpi_pm_device_sleep_state(struct device *dev, int wake, int *d_min_p)
+int acpi_pm_device_sleep_state(struct device *dev, int *d_min_p)
 {
 	acpi_handle handle = DEVICE_ACPI_HANDLE(dev);
 	struct acpi_device *adev;
@@ -427,7 +427,7 @@ int acpi_pm_device_sleep_state(struct de
 	 * can wake the system.  _S0W may be valid, too.
 	 */
 	if (acpi_target_sleep_state == ACPI_STATE_S0 ||
-	    (wake && adev->wakeup.state.enabled &&
+	    (device_may_wakeup(dev) && adev->wakeup.state.enabled &&
 	     adev->wakeup.sleep_state <= acpi_target_sleep_state)) {
 		acpi_status status;
 
--- g26.orig/drivers/pci/pci-acpi.c	2008-04-18 19:26:19.000000000 -0700
+++ g26/drivers/pci/pci-acpi.c	2008-04-18 20:08:23.000000000 -0700
@@ -249,8 +249,7 @@ static pci_power_t acpi_pci_choose_state
 {
 	int acpi_state;
 
-	acpi_state = acpi_pm_device_sleep_state(&pdev->dev,
-		device_may_wakeup(&pdev->dev), NULL);
+	acpi_state = acpi_pm_device_sleep_state(&pdev->dev, NULL);
 	if (acpi_state < 0)
 		return PCI_POWER_ERROR;
 
--- g26.orig/drivers/pnp/pnpacpi/core.c	2008-04-18 19:26:19.000000000 -0700
+++ g26/drivers/pnp/pnpacpi/core.c	2008-04-18 20:08:23.000000000 -0700
@@ -132,9 +132,7 @@ static int pnpacpi_suspend(struct pnp_de
 {
 	int power_state;
 
-	power_state = acpi_pm_device_sleep_state(&dev->dev,
-						device_may_wakeup(&dev->dev),
-						NULL);
+	power_state = acpi_pm_device_sleep_state(&dev->dev, NULL);
 	if (power_state < 0)
 		power_state = (state.event == PM_EVENT_ON) ?
 				ACPI_STATE_D0 : ACPI_STATE_D3;
--- g26.orig/include/acpi/acpi_bus.h	2008-04-18 19:26:19.000000000 -0700
+++ g26/include/acpi/acpi_bus.h	2008-04-18 20:08:23.000000000 -0700
@@ -376,9 +376,9 @@ acpi_handle acpi_get_pci_rootbridge_hand
 #define DEVICE_ACPI_HANDLE(dev) ((acpi_handle)((dev)->archdata.acpi_handle))
 
 #ifdef CONFIG_PM_SLEEP
-int acpi_pm_device_sleep_state(struct device *, int, int *);
+int acpi_pm_device_sleep_state(struct device *, int *);
 #else /* !CONFIG_PM_SLEEP */
-static inline int acpi_pm_device_sleep_state(struct device *d, int w, int *p)
+static inline int acpi_pm_device_sleep_state(struct device *d, int *p)
 {
 	if (p)
 		*p = ACPI_STATE_D0;

Re: [RE-RESEND patch 2.6.25-git] acpi_pm_device_sleep_state() cleanup

[Rafael J. Wysocki]

On Tuesday, 29 of April 2008, David Brownell wrote:
> From: David Brownell <dbrownell@users.sourceforge.net>
> 
> Get rid of a superfluous acpi_pm_device_sleep_state() parameter.  The
> only legitimate value of that parameter must be derived from the first
> parameter, which is what all the callers already do.  (However, this
> does not address the fact that ACPI *still* doesn't set up those flags.)

I picked up the previous version of this patch and I'm going to push it for
2.6.27.  Is this version different from the previous one?

Rafael


> Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
> Acked-by: Pavel Machek <pavel@ucw.cz>
> ---
> It seems patches sent to the ACPI list are often ignored.  This patch
> was sent against 2.6.25-rc6 for 2.6.26-rc0 merge, then resent 18-apr
> against 2.6.25 ... maybe the third time can finally do the trick.
> 
>  drivers/acpi/sleep/main.c  |    8 ++++----
>  drivers/pci/pci-acpi.c     |    3 +--
>  drivers/pnp/pnpacpi/core.c |    4 +---
>  include/acpi/acpi_bus.h    |    4 ++--
>  4 files changed, 8 insertions(+), 11 deletions(-)
> 
> --- g26.orig/drivers/acpi/sleep/main.c	2008-04-18 19:26:19.000000000 -0700
> +++ g26/drivers/acpi/sleep/main.c	2008-04-18 20:08:23.000000000 -0700
> @@ -369,8 +369,8 @@ int acpi_suspend(u32 acpi_state)
>  /**
>   *	acpi_pm_device_sleep_state - return preferred power state of ACPI device
>   *		in the system sleep state given by %acpi_target_sleep_state
> - *	@dev: device to examine
> - *	@wake: if set, the device should be able to wake up the system
> + *	@dev: device to examine; its driver model wakeup flags control
> + *		whether it should be able to wake up the system
>   *	@d_min_p: used to store the upper limit of allowed states range
>   *	Return value: preferred power state of the device on success, -ENODEV on
>   *		failure (ie. if there's no 'struct acpi_device' for @dev)
> @@ -388,7 +388,7 @@ int acpi_suspend(u32 acpi_state)
>   *	via @wake.
>   */
>  
> -int acpi_pm_device_sleep_state(struct device *dev, int wake, int *d_min_p)
> +int acpi_pm_device_sleep_state(struct device *dev, int *d_min_p)
>  {
>  	acpi_handle handle = DEVICE_ACPI_HANDLE(dev);
>  	struct acpi_device *adev;
> @@ -427,7 +427,7 @@ int acpi_pm_device_sleep_state(struct de
>  	 * can wake the system.  _S0W may be valid, too.
>  	 */
>  	if (acpi_target_sleep_state == ACPI_STATE_S0 ||
> -	    (wake && adev->wakeup.state.enabled &&
> +	    (device_may_wakeup(dev) && adev->wakeup.state.enabled &&
>  	     adev->wakeup.sleep_state <= acpi_target_sleep_state)) {
>  		acpi_status status;
>  
> --- g26.orig/drivers/pci/pci-acpi.c	2008-04-18 19:26:19.000000000 -0700
> +++ g26/drivers/pci/pci-acpi.c	2008-04-18 20:08:23.000000000 -0700
> @@ -249,8 +249,7 @@ static pci_power_t acpi_pci_choose_state
>  {
>  	int acpi_state;
>  
> -	acpi_state = acpi_pm_device_sleep_state(&pdev->dev,
> -		device_may_wakeup(&pdev->dev), NULL);
> +	acpi_state = acpi_pm_device_sleep_state(&pdev->dev, NULL);
>  	if (acpi_state < 0)
>  		return PCI_POWER_ERROR;
>  
> --- g26.orig/drivers/pnp/pnpacpi/core.c	2008-04-18 19:26:19.000000000 -0700
> +++ g26/drivers/pnp/pnpacpi/core.c	2008-04-18 20:08:23.000000000 -0700
> @@ -132,9 +132,7 @@ static int pnpacpi_suspend(struct pnp_de
>  {
>  	int power_state;
>  
> -	power_state = acpi_pm_device_sleep_state(&dev->dev,
> -						device_may_wakeup(&dev->dev),
> -						NULL);
> +	power_state = acpi_pm_device_sleep_state(&dev->dev, NULL);
>  	if (power_state < 0)
>  		power_state = (state.event == PM_EVENT_ON) ?
>  				ACPI_STATE_D0 : ACPI_STATE_D3;
> --- g26.orig/include/acpi/acpi_bus.h	2008-04-18 19:26:19.000000000 -0700
> +++ g26/include/acpi/acpi_bus.h	2008-04-18 20:08:23.000000000 -0700
> @@ -376,9 +376,9 @@ acpi_handle acpi_get_pci_rootbridge_hand
>  #define DEVICE_ACPI_HANDLE(dev) ((acpi_handle)((dev)->archdata.acpi_handle))
>  
>  #ifdef CONFIG_PM_SLEEP
> -int acpi_pm_device_sleep_state(struct device *, int, int *);
> +int acpi_pm_device_sleep_state(struct device *, int *);
>  #else /* !CONFIG_PM_SLEEP */
> -static inline int acpi_pm_device_sleep_state(struct device *d, int w, int *p)
> +static inline int acpi_pm_device_sleep_state(struct device *d, int *p)
>  {
>  	if (p)
>  		*p = ACPI_STATE_D0;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 



-- 
"Premature optimization is the root of all evil." - Donald Knuth

Re: [RE-RESEND patch 2.6.25-git] acpi_pm_device_sleep_state() cleanup

[David Brownell]

On Tuesday 29 April 2008, Rafael J. Wysocki wrote:
> I picked up the previous version of this patch and I'm going to push it for
> 2.6.27.  Is this version different from the previous one?

I probably regenerated it to cope with line number differences,
but that's all that I recall.


> > It seems patches sent to the ACPI list are often ignored.  This patch
> > was sent against 2.6.25-rc6 for 2.6.26-rc0 merge, then resent 18-apr
> > against 2.6.25 ... maybe the third time can finally do the trick.
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [linux-pm] [RESEND patch 2.6.25] ACPI uses device_may_wakeup() policy inputs

[David Brownell]

On Monday 28 April 2008, Alan Stern wrote:
> On Mon, 28 Apr 2008, Zhang Rui wrote:
> 
> > This may cause some "regressions", something like system resumes
> > immediately after enter S3. This should rather be a driver/device
> > problem which rises a wake up event when it's suspend.
> > The right thing to do is to disable the wake up flag for this device.
> > The patch was dropped for this reason last time it's merged and we
> > should take good care of it this time. :)

Actually that oversimplifies the problem.  The system that
was issuing wake events should not have been issuing them.

That seemed to me at the time, and still does seem to me,
like an ACPI problem ... especially since most other systems
behaved just fine.  I have one observation I'll share in a
separate message, which _might_ shed some light on this.


> Indeed, it's easy to imagine a situation where somebody suspends their 
> laptop and then unplugs a USB mouse, thereby causing a wakeup event.

It's also easy to imagine getting used to unplugging
such devices *first* ... :)


> I suspect many PCI devices should be disabled for remote wakeup during 
> system sleeps.

I wouldn't say that it's "PCI devices".  Remember, these
patches only change ACPI behavior ... making it act more
like non-ACPI systems.  If there's any change, in defaults
it should not be a global "for all PCI devices" one ... it
should be limited (and temporary!) for some ACPI devices.

The situation is that *currently* ACPI is configured so
that it's acts unlike non-ACPI systems:  wakeup isn't
enabled.  And in at least some systems, it doesn't work
when it's enabled either.

One can argue about how to deal with that breakage.  It
seems likely to me that -- since this ACPI wakeup code
has hardly ever been used on Linux -- using it will turn
up bugs in the ACPI stack.  At which point a significant
question is:  how are those bugs ever going to be found
and fixed, if ACPI never actually turns on its wakeup
mechanisms?

- Dave

Re: [RE-RESEND patch 2.6.25-git] acpi_pm_device_sleep_state() cleanup

[Rafael J. Wysocki]

On Wednesday, 30 of April 2008, David Brownell wrote:
> On Tuesday 29 April 2008, Rafael J. Wysocki wrote:
> > I picked up the previous version of this patch and I'm going to push it for
> > 2.6.27.  Is this version different from the previous one?
> 
> I probably regenerated it to cope with line number differences,
> but that's all that I recall.

That's fine.  I rebase my patch queue every day or so on top of the current
-git anyway.

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

Re: [linux-pm] [RESEND patch 2.6.25] ACPI uses device_may_wakeup() policy inputs

[Alan Stern]

On Tue, 29 Apr 2008, David Brownell wrote:

> > Indeed, it's easy to imagine a situation where somebody suspends their 
> > laptop and then unplugs a USB mouse, thereby causing a wakeup event.
> 
> It's also easy to imagine getting used to unplugging
> such devices *first* ... :)

Maybe.  But people aren't likely to unplug the mouse first if
suspending the laptop is done by clicking on a "Suspend" button.  :-)

Or consider the case of a flash drive with a mounted filesystem.  You 
can't safely unplug it before suspending, but you can safely unplug it 
after suspending (provided you remember to plug it back in before 
resuming).

Besides, people will complain that the [insert your choice] operating 
system doesn't make them remember to unplug USB devices before 
suspending, so why should Linux?

> > I suspect many PCI devices should be disabled for remote wakeup during 
> > system sleeps.
> 
> I wouldn't say that it's "PCI devices".  Remember, these
> patches only change ACPI behavior ... making it act more
> like non-ACPI systems.  If there's any change, in defaults
> it should not be a global "for all PCI devices" one ... it
> should be limited (and temporary!) for some ACPI devices.

I wasn't talking about defaults; I was talking about the settings 
people will find most useful.

> The situation is that *currently* ACPI is configured so
> that it's acts unlike non-ACPI systems:  wakeup isn't
> enabled.  And in at least some systems, it doesn't work
> when it's enabled either.
> 
> One can argue about how to deal with that breakage.  It
> seems likely to me that -- since this ACPI wakeup code
> has hardly ever been used on Linux -- using it will turn
> up bugs in the ACPI stack.  At which point a significant
> question is:  how are those bugs ever going to be found
> and fixed, if ACPI never actually turns on its wakeup
> mechanisms?

None of this is in dispute.

I'm simply pointing out that until common userspace tools are available
to change the wakeup behavior from the default "Always on if it's
available", people will run into problems and complain about
"regressions".

Alan Stern

Re: [linux-pm] [RESEND patch 2.6.25] ACPI uses device_may_wakeup() policy inputs

[Pavel Machek]

Hi!

> > > Indeed, it's easy to imagine a situation where somebody suspends their 
> > > laptop and then unplugs a USB mouse, thereby causing a wakeup event.
> > 
> > It's also easy to imagine getting used to unplugging
> > such devices *first* ... :)
> 
> Maybe.  But people aren't likely to unplug the mouse first if
> suspending the laptop is done by clicking on a "Suspend" button.  :-)
> 
> Or consider the case of a flash drive with a mounted filesystem.  You 
> can't safely unplug it before suspending, but you can safely unplug it 
> after suspending (provided you remember to plug it back in before 
> resuming).
> 
> Besides, people will complain that the [insert your choice] operating 
> system doesn't make them remember to unplug USB devices before 
> suspending, so why should Linux?

Besides, its a regression.

So we really should just default to the current behaviour on ACPI
systems: lid and power button wake up the system, other devices don't.
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html