run resume code only for S1-S4 states

Andriy Gapon avg at freebsd.org
Tue Apr 14 09:11:23 PDT 2009


Guys,
could you please review the attached patch?

Its main idea is to make control flow of acpi_EnterSleepState similar
to that of acpi_ReqSleepState: reject invalid state parameter immediately and
handle special S5 as early as possible. Primary purpose is to avoid running resume
code when it is not necessary - e.g. shutdown_nice() typically returns immediately
after initiating a graceful shutdown by sending a signal to init.

As such, S5 is handled right after checking/disabling re-entry.
switch becomes unneeded, because all remaining possibilities are grouped
into a single case. I decided to use do-while(0) statement in the place of the
switch for the following reasons:
1. minimize diff by preserving indentation
2. minimize diff by preserving control flow that depends on break statement
But I am not sure how this while(0) corresponds with style(9), I couldn't find any
reference in the manual page.

There is also a concern about calling shutdown_nice() outside of the Giant lock
and binding to CPU 0. I am not sure about the pre-requisites for this function.
John, maybe you could help me here?

-- 
Andriy Gapon

-------------- next part --------------
diff --git a/sys/dev/acpica/acpi.c b/sys/dev/acpica/acpi.c
index 50b84a5..ac654d2 100644
--- a/sys/dev/acpica/acpi.c
+++ b/sys/dev/acpica/acpi.c
@@ -2482,6 +2482,9 @@ acpi_EnterSleepState(struct acpi_softc *sc, int state)
 
     ACPI_FUNCTION_TRACE_U32((char *)(uintptr_t)__func__, state);
 
+    if (state < ACPI_STATE_S1 || state > ACPI_STATE_S5)
+	return_ACPI_STATUS (AE_BAD_PARAMETER);
+
     /* Re-entry once we're suspending is not allowed. */
     status = acpi_sleep_disable(sc);
     if (ACPI_FAILURE(status)) {
@@ -2489,6 +2492,15 @@ acpi_EnterSleepState(struct acpi_softc *sc, int state)
 	return (status);
     }
 
+    if (state == ACPI_STATE_S5) {
+	/*
+	 * Shut down cleanly and power off.  This will call us back through the
+	 * shutdown handlers.
+	 */
+	shutdown_nice(RB_POWEROFF);
+	return_ACPI_STATUS (AE_OK);
+    }
+
 #ifdef SMP
     thread_lock(curthread);
     sched_bind(curthread, 0);
@@ -2502,11 +2514,7 @@ acpi_EnterSleepState(struct acpi_softc *sc, int state)
     mtx_lock(&Giant);
 
     slp_state = ACPI_SS_NONE;
-    switch (state) {
-    case ACPI_STATE_S1:
-    case ACPI_STATE_S2:
-    case ACPI_STATE_S3:
-    case ACPI_STATE_S4:
+    do {
 	status = AcpiGetSleepTypeData(state, &TypeA, &TypeB);
 	if (status == AE_NOT_FOUND) {
 	    device_printf(sc->acpi_dev,
@@ -2569,20 +2577,7 @@ acpi_EnterSleepState(struct acpi_softc *sc, int state)
 	    }
 	}
 	slp_state = ACPI_SS_SLEPT;
-	break;
-    case ACPI_STATE_S5:
-	/*
-	 * Shut down cleanly and power off.  This will call us back through the
-	 * shutdown handlers.
-	 */
-	shutdown_nice(RB_POWEROFF);
-	status = AE_OK;
-	break;
-    case ACPI_STATE_S0:
-    default:
-	status = AE_BAD_PARAMETER;
-	break;
-    }
+    } while (0);
 
     /*
      * Back out state according to how far along we got in the suspend
@@ -2609,8 +2604,7 @@ acpi_EnterSleepState(struct acpi_softc *sc, int state)
 #endif
 
     /* Allow another sleep request after a while. */
-    if (state != ACPI_STATE_S5)
-	timeout(acpi_sleep_enable, sc, hz * ACPI_MINIMUM_AWAKETIME);
+    timeout(acpi_sleep_enable, sc, hz * ACPI_MINIMUM_AWAKETIME);
 
     /* Run /etc/rc.resume after we are back. */
     if (devctl_process_running())


More information about the freebsd-acpi mailing list