Skip to contentSkip to conversation

ACPI/Sleep: pm_power_off need more sanity check to be installed

PATCH Applied Li, Aubrey 2 participants 7 messages Feb 26, 2014 – Mar 2, 2014 ACPI

lore
Share & tools
lore

Li, Aubrey

author
#0 lore raw

PATCH 1/1 ACPI/Sleep: pm_power_off need more sanity check to be installed

Sleep control and status registers need santity check before ACPI install acpi_power_off to pm_power_off hook. The checking code in acpi_enter_sleep_state() is too late, we should not allow a not-working pm_power_off function hooked.

Signed-off-by Aubrey Li <[email protected]>
---
 drivers/acpi/sleep.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
drivers/acpi/sleep.c +5 -2
index b718806..0284d22 100644
@@ -809,8 +809,11 @@ int __init acpi_sleep_init(void)
 	status = acpi_get_sleep_type_data(ACPI_STATE_S5, &type_a, &type_b);
 	if (ACPI_SUCCESS(status)) {
 		sleep_states[ACPI_STATE_S5] = 1;
-		pm_power_off_prepare = acpi_power_off_prepare;
-		pm_power_off = acpi_power_off;
+		if (acpi_gbl_FADT.sleep_control.address &&
+			acpi_gbl_FADT.sleep_status.address) {
+			pm_power_off_prepare = acpi_power_off_prepare;
+			pm_power_off = acpi_power_off;
+		}
 	}
 
 	supported[0] = 0;
Signature · 1 line
1.7.10.4
#1 lore raw
Quoted 15 lines · Aubrey

Sleep control and status registers need santity check before ACPI install acpi_power_off to pm_power_off hook. The checking code in acpi_enter_sleep_state() is too late, we should not allow a not-working pm_power_off function hooked.

Signed-off-by Aubrey Li <[email protected]>
---
 drivers/acpi/sleep.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
drivers/acpi/sleep.c +0 -0
index b718806..0284d22 100644

drivers/acpi/sleep.c int __init acpi_sleep_init(void) · line 811

Quoted 3 lines
drivers/acpi/sleep.c +0 -0
@@ -809,8 +809,11 @@ int __init acpi_sleep_init(void)
 	status = acpi_get_sleep_type_data(ACPI_STATE_S5, &type_a, &type_b);
 	if (ACPI_SUCCESS(status)) {
 		sleep_states[ACPI_STATE_S5] = 1;

Do we still want to set this if the check below fails? If so, then why?

Quoted 11 lines
drivers/acpi/sleep.c +5 -2
-		pm_power_off_prepare = acpi_power_off_prepare;
-		pm_power_off = acpi_power_off;
+		if (acpi_gbl_FADT.sleep_control.address &&
+			acpi_gbl_FADT.sleep_status.address) {
+			pm_power_off_prepare = acpi_power_off_prepare;
+			pm_power_off = acpi_power_off;
+		}
 	}
 
 	supported[0] = 0;
Signature · 2 lines
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
author
#2 lore raw
Quoted 11 lines · Rafael J. Wysocki
Quoted 10 lines · Aubrey

Sleep control and status registers need santity check before ACPI install acpi_power_off to pm_power_off hook. The checking code in acpi_enter_sleep_state() is too late, we should not allow a not-working pm_power_off function hooked.

Signed-off-by Aubrey Li <[email protected]>
---
 drivers/acpi/sleep.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
Quoted 10 lines

drivers/acpi/sleep.c int __init acpi_sleep_init(void) · line 811

Quoted 8 lines
drivers/acpi/sleep.c +0 -0
index b718806..0284d22 100644
@@ -809,8 +809,11 @@ int __init acpi_sleep_init(void)
 	status = acpi_get_sleep_type_data(ACPI_STATE_S5, &type_a, &type_b);
 	if (ACPI_SUCCESS(status)) {
 		sleep_states[ACPI_STATE_S5] = 1;

Do we still want to set this if the check below fails? If so, then why?

We know \_S5_ is valid. The fault is sleep registers, not S5 ACPI object

Thanks,
-Aubrey

Quoted 13 lines
Quoted 12 lines
drivers/acpi/sleep.c +5 -2
-		pm_power_off_prepare = acpi_power_off_prepare;
-		pm_power_off = acpi_power_off;
+		if (acpi_gbl_FADT.sleep_control.address &&
+			acpi_gbl_FADT.sleep_status.address) {
+			pm_power_off_prepare = acpi_power_off_prepare;
+			pm_power_off = acpi_power_off;
+		}
 	}
 
 	supported[0] = 0;
author
#3 lore raw
Quoted 23 lines · Aubrey
Quoted 11 lines · Rafael J. Wysocki
Quoted 10 lines · Aubrey

Sleep control and status registers need santity check before ACPI install acpi_power_off to pm_power_off hook. The checking code in acpi_enter_sleep_state() is too late, we should not allow a not-working pm_power_off function hooked.

Signed-off-by Aubrey Li <[email protected]>
---
 drivers/acpi/sleep.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
Quoted 10 lines

drivers/acpi/sleep.c int __init acpi_sleep_init(void) · line 811

Quoted 8 lines
drivers/acpi/sleep.c +0 -0
index b718806..0284d22 100644
@@ -809,8 +809,11 @@ int __init acpi_sleep_init(void)
 	status = acpi_get_sleep_type_data(ACPI_STATE_S5, &type_a, &type_b);
 	if (ACPI_SUCCESS(status)) {
 		sleep_states[ACPI_STATE_S5] = 1;

Do we still want to set this if the check below fails? If so, then why?

Quoted 1 line

We know \_S5_ is valid. The fault is sleep registers, not S5 ACPI object

Hi Rafael, do you still have any concern?

Thanks,
-Aubrey

Quoted 15 lines
Quoted 14 lines
Quoted 13 lines
drivers/acpi/sleep.c +5 -2
-		pm_power_off_prepare = acpi_power_off_prepare;
-		pm_power_off = acpi_power_off;
+		if (acpi_gbl_FADT.sleep_control.address &&
+			acpi_gbl_FADT.sleep_status.address) {
+			pm_power_off_prepare = acpi_power_off_prepare;
+			pm_power_off = acpi_power_off;
+		}
 	}
 
 	supported[0] = 0;
#4 lore raw
Quoted 24 lines · Aubrey
Quoted 23 lines · Aubrey
Quoted 21 lines · Rafael J. Wysocki
Quoted 15 lines · Aubrey

Sleep control and status registers need santity check before ACPI install acpi_power_off to pm_power_off hook. The checking code in acpi_enter_sleep_state() is too late, we should not allow a not-working pm_power_off function hooked.

Signed-off-by Aubrey Li <[email protected]>
---
 drivers/acpi/sleep.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
drivers/acpi/sleep.c +0 -0
index b718806..0284d22 100644

drivers/acpi/sleep.c int __init acpi_sleep_init(void) · line 811

Quoted 3 lines
drivers/acpi/sleep.c +0 -0
@@ -809,8 +809,11 @@ int __init acpi_sleep_init(void)
 	status = acpi_get_sleep_type_data(ACPI_STATE_S5, &type_a, &type_b);
 	if (ACPI_SUCCESS(status)) {
 		sleep_states[ACPI_STATE_S5] = 1;

Do we still want to set this if the check below fails? If so, then why?

Quoted 3 lines
Quoted 1 line

We know \_S5_ is valid. The fault is sleep registers, not S5 ACPI object

Hi Rafael, do you still have any concern?

Well, I simply don't think we should say that it is "supported" if we aren't going to do anything with it.

Quoted 19 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
drivers/acpi/sleep.c +5 -2
-		pm_power_off_prepare = acpi_power_off_prepare;
-		pm_power_off = acpi_power_off;
+		if (acpi_gbl_FADT.sleep_control.address &&
+			acpi_gbl_FADT.sleep_status.address) {
+			pm_power_off_prepare = acpi_power_off_prepare;
+			pm_power_off = acpi_power_off;
+		}
 	}
Quoted 4 lines
Quoted 3 lines

drivers/acpi/sleep.c int __init acpi_sleep_init(void) · line 819

Quoted 2 lines
drivers/acpi/sleep.c +0 -0
 	supported[0] = 0;

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

Signature · 2 lines
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
author
#5 lore raw
Quoted 10 lines · Rafael J. Wysocki
Quoted 5 lines · Aubrey
Quoted 3 lines
Quoted 1 line

Do we still want to set this if the check below fails? If so, then why?

We know \_S5_ is valid. The fault is sleep registers, not S5 ACPI object

Hi Rafael, do you still have any concern?

Well, I simply don't think we should say that it is "supported" if we aren't going to do anything with it.

Make sense to me. Patch refined as below:

Sleep control and status registers need santity check as well before ACPI install acpi_power_off to pm_power_off hook. The checking code in acpi_enter_sleep_state() is too late, we should not allow a not-working pm_power_off function hooked.

Signed-off-by Aubrey Li <[email protected]>
---
 drivers/acpi/sleep.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/acpi/sleep.c +6 -1
index b718806..0abfbb1 100644
@@ -807,7 +807,12 @@ int __init acpi_sleep_init(void)
 	acpi_sleep_hibernate_setup();
 
 	status = acpi_get_sleep_type_data(ACPI_STATE_S5, &type_a, &type_b);
-	if (ACPI_SUCCESS(status)) {
+	/*
+	 * Check both ACPI S5 object and ACPI sleep registers to
+	 * install pm_power_off_prepare/pm_power_off hook
+	 */
+	if (ACPI_SUCCESS(status) && acpi_gbl_FADT.sleep_control.address &&
+		acpi_gbl_FADT.sleep_status.address) {
 		sleep_states[ACPI_STATE_S5] = 1;
 		pm_power_off_prepare = acpi_power_off_prepare;
 		pm_power_off = acpi_power_off;

-- 1.7.10.4

Applied
#6 lore raw
Quoted 12 lines · Aubrey
Quoted 10 lines · Rafael J. Wysocki
Quoted 5 lines · Aubrey
Quoted 3 lines
Quoted 1 line

Do we still want to set this if the check below fails? If so, then why?

We know \_S5_ is valid. The fault is sleep registers, not S5 ACPI object

Hi Rafael, do you still have any concern?

Well, I simply don't think we should say that it is "supported" if we aren't going to do anything with it.

Quoted 8 lines

Make sense to me. Patch refined as below:

Sleep control and status registers need santity check as well before ACPI install acpi_power_off to pm_power_off hook. The checking code in acpi_enter_sleep_state() is too late, we should not allow a not-working pm_power_off function hooked.

Signed-off-by Aubrey Li <[email protected]>

Queued up for 3.15 (with minor changes), thanks!

Quoted 28 lines
---
 drivers/acpi/sleep.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/acpi/sleep.c +6 -3
index b718806..0abfbb1 100644
@@ -807,7 +807,12 @@ int __init acpi_sleep_init(void)
 	acpi_sleep_hibernate_setup();
 
 	status = acpi_get_sleep_type_data(ACPI_STATE_S5, &type_a, &type_b);
-	if (ACPI_SUCCESS(status)) {
+	/*
+	 * Check both ACPI S5 object and ACPI sleep registers to
+	 * install pm_power_off_prepare/pm_power_off hook
+	 */
+	if (ACPI_SUCCESS(status) && acpi_gbl_FADT.sleep_control.address &&
+		acpi_gbl_FADT.sleep_status.address) {
 		sleep_states[ACPI_STATE_S5] = 1;
 		pm_power_off_prepare = acpi_power_off_prepare;
 		pm_power_off = acpi_power_off;
-- 1.7.10.4
--

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

Signature · 2 lines
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.