ACPI: Fix acpi_pm_device_sleep_state()

PATCH In review Rafael J. Wysocki 2 participants 4 messages Jan 10, 2008 – Jan 12, 2008 ACPI

lore

Share & tools

lore

Rafael J. Wysocki

author

2008-01-10 23:08 UTC #0 lore raw

PATCH 1/1 ACPI: Fix acpi_pm_device_sleep_state()

From: Rafael J. Wysocki <[email protected]>

Fix acpi_pm_device_sleep_state() to return the value returned by _SxD if the device is supposed to wake up the system from given sleep state and the evaluation of _SxW fails (e.g. _SxW is not present).

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/acpi/sleep/main.c |   15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

drivers/acpi/sleep/main.c +12 -3

Index: linux-2.6/drivers/acpi/sleep/main.c
===================================================================
@@ -472,11 +472,20 @@ int acpi_pm_device_sleep_state(struct de
 	if (acpi_target_sleep_state == ACPI_STATE_S0 ||
 	    (wake && adev->wakeup.state.enabled &&
 	     adev->wakeup.sleep_state <= acpi_target_sleep_state)) {
+		acpi_status status;
+
 		acpi_method[3] = 'W';
-		acpi_evaluate_integer(handle, acpi_method, NULL, &d_max);
-		/* Sanity check */
-		if (d_max < d_min)
+		status = acpi_evaluate_integer(handle, acpi_method, NULL,
+						&d_max);
+		if (ACPI_FAILURE(status)) {
+			d_max = d_min;
+		} else if (d_max < d_min) {
+			/* Warn the user of the broken DSDT */
+			printk(KERN_WARNING "ACPI: Wrong value from %s\n",
+				acpi_method);
+			/* Sanitize it */
 			d_min = d_max;
+		}
 	}
 
 	if (d_min_p)

Len Brown

reviewer

2008-01-12 04:46 UTC #1 lore raw

Quoted 31 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>
Fix acpi_pm_device_sleep_state() to return the value returned by _SxD if the device is supposed to wake up the system from given sleep state and the evaluation of _SxW fails (e.g. _SxW is not present).
Signed-off-by Rafael J. Wysocki <[email protected]>
---
 drivers/acpi/sleep/main.c |   15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)
Index: linux-2.6/drivers/acpi/sleep/main.c
===================================================================
drivers/acpi/sleep/main.c +6 -3
@@ -472,11 +472,20 @@ int acpi_pm_device_sleep_state(struct de
 	if (acpi_target_sleep_state == ACPI_STATE_S0 ||
 	    (wake && adev->wakeup.state.enabled &&
 	     adev->wakeup.sleep_state <= acpi_target_sleep_state)) {
+		acpi_status status;
+
 		acpi_method[3] = 'W';
-		acpi_evaluate_integer(handle, acpi_method, NULL, &d_max);
-		/* Sanity check */
-		if (d_max < d_min)
+		status = acpi_evaluate_integer(handle, acpi_method, NULL,
+						&d_max);
+		if (ACPI_FAILURE(status)) {
+			d_max = d_min;

drivers/acpi/sleep/main.c int acpi_pm_device_sleep_state(struct de · line 491

Quoted 10 lines

drivers/acpi/sleep/main.c +6 -0

+		} else if (d_max < d_min) {
+			/* Warn the user of the broken DSDT */
+			printk(KERN_WARNING "ACPI: Wrong value from %s\n",
+				acpi_method);
+			/* Sanitize it */
 			d_min = d_max;
+		}
 	}
 
 	if (d_min_p)

this patch looks correct, it fixes the bug where _SxD=2, _PRW=x, _SxW was absent, we would return D3 as legal, when we should have returned D2.

applied to acpi test.

BTW. We currently don't use *d_min_p
and I can't imagine when and why we ever would,
so it would be fine with me if you simplify by deleting it.

Also, the last line of this block comment can be deleted, though that was true before this patch also:

acpi_pci_choose_state() block comment can now be updated to delete this line:

* currently we simply return _SxD, if present.

thanks,
-Len

Rafael J. Wysocki

author

2008-01-12 11:34 UTC #2 lore raw

Quoted 47 lines · Len Brown

Quoted 31 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
Fix acpi_pm_device_sleep_state() to return the value returned by _SxD if the device is supposed to wake up the system from given sleep state and the evaluation of _SxW fails (e.g. _SxW is not present).
Signed-off-by Rafael J. Wysocki <[email protected]>
---
 drivers/acpi/sleep/main.c |   15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)
Index: linux-2.6/drivers/acpi/sleep/main.c
===================================================================
drivers/acpi/sleep/main.c +6 -3
@@ -472,11 +472,20 @@ int acpi_pm_device_sleep_state(struct de
 	if (acpi_target_sleep_state == ACPI_STATE_S0 ||
 	    (wake && adev->wakeup.state.enabled &&
 	     adev->wakeup.sleep_state <= acpi_target_sleep_state)) {
+		acpi_status status;
+
 		acpi_method[3] = 'W';
-		acpi_evaluate_integer(handle, acpi_method, NULL, &d_max);
-		/* Sanity check */
-		if (d_max < d_min)
+		status = acpi_evaluate_integer(handle, acpi_method, NULL,
+						&d_max);
+		if (ACPI_FAILURE(status)) {
+			d_max = d_min;
drivers/acpi/sleep/main.c int acpi_pm_device_sleep_state(struct de · line 491
Quoted 10 lines
drivers/acpi/sleep/main.c +6 -0
+		} else if (d_max < d_min) {
+			/* Warn the user of the broken DSDT */
+			printk(KERN_WARNING "ACPI: Wrong value from %s\n",
+				acpi_method);
+			/* Sanitize it */
 			d_min = d_max;
+		}
 	}
 
 	if (d_min_p)
this patch looks correct, it fixes the bug where _SxD=2, _PRW=x, _SxW was absent, we would return D3 as legal, when we should have returned D2.

Quoted 5 lines

applied to acpi test.
BTW. We currently don't use *d_min_p
and I can't imagine when and why we ever would,
so it would be fine with me if you simplify by deleting it.

It was requested by someone. I can imagine that if _SxW > _SxD, a driver can decide which state to choose on the basis of some policy we don't know of.

Quoted 2 lines

Also, the last line of this block comment can be deleted, though that was true before this patch also:

Not sure what you mean here ...

Quoted 3 lines

acpi_pci_choose_state() block comment can now be updated to delete this line:
* currently we simply return _SxD, if present.

Yes, I'll do that in a separate patch.

Thanks,
Rafael

Len Brown

reviewer

2008-01-12 23:25 UTC #3 lore raw

Quoted 7 lines

Quoted 4 lines
BTW. We currently don't use *d_min_p
and I can't imagine when and why we ever would,
so it would be fine with me if you simplify by deleting it.
It was requested by someone. I can imagine that if _SxW > _SxD, a driver can decide which state to choose on the basis of some policy we don't know of.

I think that for the S3 and S4 states, we'll always use the deepest state that the constraints allow while not breaking system wake.

I haven't thought much about "device wake" -- ie _PRW in S0. Maybe some non-trivial device specific policy would be useful there some day. Of course the device would have to know the relative latency of the different D-states for such a policy to make a useful. And I guess they can infer that Dx state latency on a class basis...

-Len

Quoted 18 lines

Quoted 2 lines
Also, the last line of this block comment can be deleted, though that was true before this patch also:
Not sure what you mean here ...
Quoted 3 lines
acpi_pci_choose_state() block comment can now be updated to delete this line:
* currently we simply return _SxD, if present.
Yes, I'll do that in a separate patch.
Thanks,
Rafael
-
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