misc pm wake patches

From: David Brownell <[email protected]>

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 <[email protected]>

Cc Zhang Rui <[email protected]>

---
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(+)

drivers/acpi/glue.c +27 -1

@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

From: David Brownell <[email protected]>

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 <[email protected]>

Cc Zhang Rui <[email protected]>

---
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(+)

drivers/acpi/glue.c +27 -1

@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 10 lines · David Brownell

From: David Brownell <[email protected]>

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/

Quoted 3 lines

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

Quoted 10 lines · David Brownell

From: David Brownell <[email protected]>

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/

Quoted 3 lines

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

Quoted 15 lines · Zhao Yakui

Quoted 13 lines · David Brownell

From: David Brownell <[email protected]>

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.

Quoted 4 lines

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?

Quoted 3 lines

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

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

Quoted 15 lines · Zhao Yakui

Quoted 13 lines · David Brownell

From: David Brownell <[email protected]>

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.

Quoted 4 lines

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?

Quoted 3 lines

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

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

Quoted 9 lines · David Brownell

Quoted 8 lines

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 3 lines

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 ...

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.

Quoted 9 lines · David Brownell

Quoted 8 lines

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 3 lines

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 ...

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.

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 <[email protected]>

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 <[email protected]>

From: David Brownell <[email protected]>

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 <[email protected]>

Acked-by Pavel Machek <[email protected]>

---
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(-)

drivers/acpi/sleep/main.c +4 -4

@@ -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;
 

drivers/pci/pci-acpi.c +1 -2

@@ -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;
 

drivers/pnp/pnpacpi/core.c +1 -3

@@ -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;

include/acpi/acpi_bus.h +2 -3

@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

From: David Brownell <[email protected]>

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 <[email protected]>

Acked-by Pavel Machek <[email protected]>

---
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(-)

drivers/acpi/sleep/main.c +4 -4

@@ -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;
 

drivers/pci/pci-acpi.c +1 -2

@@ -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;
 

drivers/pnp/pnpacpi/core.c +1 -3

@@ -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;

include/acpi/acpi_bus.h +2 -3

@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

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?

Quoted 3 lines

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.

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?

Quoted 3 lines

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.

Quoted 2 lines · Rafael J. Wysocki

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.

Quoted 2 lines · Rafael J. Wysocki

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.

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 <[email protected]>

---
 drivers/pci/pci.c |   47 ++++++++++++++++++++++++++---------------------
 1 file changed, 26 insertions(+), 21 deletions(-)

drivers/pci/pci.c +14 -17

@@ -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)

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 <[email protected]>

---
 drivers/pci/pci.c |   47 ++++++++++++++++++++++++++---------------------
 1 file changed, 26 insertions(+), 21 deletions(-)

drivers/pci/pci.c +14 -17

@@ -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)

Quoted 15 lines

drivers/pci/pci.c +14 -0

+	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;

drivers/pci/pci.c pci_set_power_state(struct pci_dev *dev, · line 562

Quoted 5 lines

drivers/pci/pci.c +3 -1

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

Quoted 11 lines

drivers/pci/pci.c +4 -3

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

Quoted 1 line

EXPORT_SYMBOL(pci_choose_state);

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

Quoted 15 lines

drivers/pci/pci.c +14 -0

+	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;

drivers/pci/pci.c pci_set_power_state(struct pci_dev *dev, · line 562

Quoted 5 lines

drivers/pci/pci.c +3 -1

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

Quoted 11 lines

drivers/pci/pci.c +4 -3

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

Quoted 1 line

EXPORT_SYMBOL(pci_choose_state);

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

Quoted 15 lines

Quoted 15 lines

drivers/pci/pci.c +14 -0

+	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;

Quoted 8 lines

drivers/pci/pci.c pci_set_power_state(struct pci_dev *dev, · line 562

Quoted 5 lines

drivers/pci/pci.c +3 -1

+		}
+		/* 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?

Quoted 11 lines

Quoted 11 lines

drivers/pci/pci.c +4 -3

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

Quoted 3 lines

Quoted 1 line

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.

Quoted 15 lines

Quoted 15 lines

drivers/pci/pci.c +14 -0

+	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;

Quoted 8 lines

drivers/pci/pci.c pci_set_power_state(struct pci_dev *dev, · line 562

Quoted 5 lines

drivers/pci/pci.c +3 -1

+		}
+		/* 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?

Quoted 11 lines

Quoted 11 lines

drivers/pci/pci.c +4 -3

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

Quoted 3 lines

Quoted 1 line

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.

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(). :-)

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

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(). :-)

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

Quoted 3 lines · Rafael J. Wysocki

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.

Quoted 3 lines · Rafael J. Wysocki

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.

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.

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

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.

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

Quoted 2 lines · Rafael J. Wysocki

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.)

Quoted 3 lines

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?

Quoted 2 lines

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

Quoted 2 lines · Rafael J. Wysocki

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.)

Quoted 3 lines

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?

Quoted 2 lines

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

Quoted 3 lines

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.

Quoted 5 lines

Quoted 2 lines

    [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()?

Quoted 3 lines

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.

Quoted 5 lines

Quoted 2 lines

    [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()?

Quoted 8 lines · Rafael J. Wysocki

Quoted 3 lines

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.

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

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

Quoted 2 lines

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()?

Quoted 8 lines · Rafael J. Wysocki

Quoted 3 lines

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.

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

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

Quoted 2 lines

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()?

Quoted 4 lines

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.

Quoted 4 lines

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.

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

Quoted 4 lines

Quoted 1 line

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.

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

Quoted 4 lines

Quoted 1 line

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.

Quoted 3 lines · Rafael J. Wysocki

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.

Quoted 1 line

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.

Quoted 2 lines

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.

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

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 <[email protected]>

---
 drivers/pci/pci.c |   49 ++++++++++++++++++++++++++-----------------------
 1 file changed, 26 insertions(+), 23 deletions(-)

drivers/pci/pci.c +13 -19

@@ -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);

drivers/pci/pci.c +0 -2

 	default:
-		printk("Unrecognized suspend event %d\n", state.event);
-		BUG();

Quoted 3 lines · Rafael J. Wysocki

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.

Quoted 1 line

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.

Quoted 2 lines

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.

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

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 <[email protected]>

---
 drivers/pci/pci.c |   49 ++++++++++++++++++++++++++-----------------------
 1 file changed, 26 insertions(+), 23 deletions(-)

drivers/pci/pci.c +13 -19

@@ -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);

drivers/pci/pci.c +0 -2

 	default:
-		printk("Unrecognized suspend event %d\n", state.event);
-		BUG();

Quoted 3 lines · David Brownell

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.

Quoted 5 lines

Quoted 2 lines

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?

Quoted 11 lines

(quoted diff) +8 -1

+	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);

Review

Quoted 10 lines

(quoted diff) +10 -0

+			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:

Quoted 3 lines · David Brownell

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.

Quoted 5 lines

Quoted 2 lines

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?

Quoted 11 lines

(quoted diff) +8 -1

+	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);

Review

Quoted 10 lines

(quoted diff) +10 -0

+			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:

Quoted 7 lines · Rafael J. Wysocki

Quoted 3 lines · David Brownell

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?

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

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...

Quoted 1 line

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.

Quoted 24 lines

Quoted 11 lines

drivers/pci/pci.c +8 -1

+	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);

drivers/pci/pci.c pci_set_power_state(struct pci_dev *dev, · line 569

Quoted 10 lines

drivers/pci/pci.c +10 -0

+			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:

drivers/pci/pci.c pci_set_power_state(struct pci_dev *dev,

Quoted 4 lines

drivers/pci/pci.c +4 -0

+		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.

drivers/pci/pci.c pci_set_power_state(struct pci_dev *dev, · line 565

Quoted 5 lines

drivers/pci/pci.c +5 -0

+				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.

Review

Quoted 1 line

(quoted diff) +1 -0

+		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 <[email protected]>

---
 drivers/pci/pci.c |   53 ++++++++++++++++++++++++++++++-----------------------
 1 file changed, 30 insertions(+), 23 deletions(-)

drivers/pci/pci.c +15 -19

@@ -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

drivers/pci/pci.c +3 -2

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

Quoted 7 lines · Rafael J. Wysocki

Quoted 3 lines · David Brownell

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?

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

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...

Quoted 1 line

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.

Quoted 24 lines

Quoted 11 lines

drivers/pci/pci.c +8 -1

+	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);

drivers/pci/pci.c pci_set_power_state(struct pci_dev *dev, · line 569

Quoted 10 lines

drivers/pci/pci.c +10 -0

+			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:

drivers/pci/pci.c pci_set_power_state(struct pci_dev *dev,

Quoted 4 lines

drivers/pci/pci.c +4 -0

+		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.

drivers/pci/pci.c pci_set_power_state(struct pci_dev *dev, · line 565

Quoted 5 lines

drivers/pci/pci.c +5 -0

+				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.

Review

Quoted 1 line

(quoted diff) +1 -0

+		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 <[email protected]>

---
 drivers/pci/pci.c |   53 ++++++++++++++++++++++++++++++-----------------------
 1 file changed, 30 insertions(+), 23 deletions(-)

drivers/pci/pci.c +15 -19

@@ -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

drivers/pci/pci.c +3 -2

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

Quoted 14 lines · David Brownell

Quoted 7 lines · Rafael J. Wysocki

Quoted 3 lines · David Brownell

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"?

Quoted 1 line

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.

Quoted 13 lines

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

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?

Quoted 2 lines

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.

Quoted 5 lines

Quoted 1 line

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.

Quoted 24 lines

Quoted 24 lines

Quoted 11 lines

drivers/pci/pci.c +8 -1

+	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);

drivers/pci/pci.c pci_set_power_state(struct pci_dev *dev, · line 569

Quoted 10 lines

drivers/pci/pci.c +10 -0

+			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:

Quoted 7 lines

drivers/pci/pci.c pci_set_power_state(struct pci_dev *dev,

Quoted 4 lines

drivers/pci/pci.c +4 -0

+		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?

Quoted 7 lines

drivers/pci/pci.c pci_set_power_state(struct pci_dev *dev, · line 565

Quoted 5 lines

drivers/pci/pci.c +5 -0

+				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).

Quoted 87 lines

so you think the quickie fix should have removed that first "break"? Simpler to just have said so.

Review

Quoted 1 line

(quoted diff) +1 -0

+		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 <[email protected]>

---
 drivers/pci/pci.c |   53 ++++++++++++++++++++++++++++++-----------------------
 1 file changed, 30 insertions(+), 23 deletions(-)

drivers/pci/pci.c +22 -19

@@ -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;