Skip site navigation (1)Skip section navigation (2)
Date:      Tue, 29 Nov 2016 16:51:46 +0100
From:      Hans Petter Selasky <hps@selasky.org>
To:        freebsd-acpi@freebsd.org
Subject:   Recursion in ACPI EC causes GPE lockup
Message-ID:  <f519f355-c851-bf58-7daf-fb45bc7b69e0@selasky.org>

next in thread | raw e-mail | index | archive | help
This is a multi-part message in MIME format.
--------------C1023E7E159D68047EF9349A
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit

Hi,

I've managed to debug a problem in acpi_ec.c with a recent skylake laptop:

In EcSpaceHandler() there is a call to EcGpeQueryHandler(). This causes 
EcSpaceHandler() to recurse during cold boot.

When EcGpeQueryHandler() is recursing it appears that because the 
completion of a GPE interrupt also invokes EcSpaceHandler() again, we go 
into an infinite loop, and the ACPI just generate more and more commands 
that EcGpeQueryHandler() never completes, because it is reading the 
command queue first all the way.

Solution: Don't allow EcGpeQueryHandler() to recurse this way. The 
attached patch entirely fixes my problem.

Can someone here review the attached patch?

--HPS

--------------C1023E7E159D68047EF9349A
Content-Type: text/x-patch;
 name="acpi_ec.c.diff"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="acpi_ec.c.diff"

diff --git a/sys/dev/acpica/acpi_ec.c b/sys/dev/acpica/acpi_ec.c
index 6482b3f..7721e10 100644
--- a/sys/dev/acpica/acpi_ec.c
+++ b/sys/dev/acpica/acpi_ec.c
@@ -647,12 +647,11 @@ EcGpeQueryHandler(void *Context)
 	    EC_EVENT_INPUT_BUFFER_EMPTY)))
 	    break;
     }
-    sc->ec_sci_pend = FALSE;
     if (ACPI_FAILURE(Status)) {
 	EcUnlock(sc);
 	device_printf(sc->ec_dev, "GPE query failed: %s\n",
 	    AcpiFormatException(Status));
-	return;
+	goto done;
     }
     Data = EC_GET_DATA(sc);
 
@@ -666,7 +665,7 @@ EcGpeQueryHandler(void *Context)
     /* Ignore the value for "no outstanding event". (13.3.5) */
     CTR2(KTR_ACPI, "ec query ok,%s running _Q%02X", Data ? "" : " not", Data);
     if (Data == 0)
-	return;
+	goto done;
 
     /* Evaluate _Qxx to respond to the controller. */
     snprintf(qxx, sizeof(qxx), "_Q%02X", Data);
@@ -676,6 +675,9 @@ EcGpeQueryHandler(void *Context)
 	device_printf(sc->ec_dev, "evaluation of query method %s failed: %s\n",
 	    qxx, AcpiFormatException(Status));
     }
+done:
+    /* Allow EcGpeQueryHandler() to be called again */
+    atomic_cmpset_int(&sc->ec_sci_pend, 1, 0);
 }
 
 /*
@@ -706,13 +708,14 @@ EcGpeHandler(ACPI_HANDLE GpeDevice, UINT32 GpeNumber, void *Context)
      * It will run the query and _Qxx method later, under the lock.
      */
     EcStatus = EC_GET_CSR(sc);
-    if ((EcStatus & EC_EVENT_SCI) && !sc->ec_sci_pend) {
+    if ((EcStatus & EC_EVENT_SCI) &&
+	atomic_cmpset_int(&sc->ec_sci_pend, 0, 1)) {
 	CTR0(KTR_ACPI, "ec gpe queueing query handler");
 	Status = AcpiOsExecute(OSL_GPE_HANDLER, EcGpeQueryHandler, Context);
-	if (ACPI_SUCCESS(Status))
-	    sc->ec_sci_pend = TRUE;
-	else
+	if (ACPI_FAILURE(Status)) {
 	    printf("EcGpeHandler: queuing GPE query handler failed\n");
+	    atomic_cmpset_int(&sc->ec_sci_pend, 1, 0);
+	}
     }
     return (ACPI_REENABLE_GPE);
 }
@@ -759,7 +762,8 @@ EcSpaceHandler(UINT32 Function, ACPI_PHYSICAL_ADDRESS Address, UINT32 Width,
      * we call it directly here since our thread taskq is not active yet.
      */
     if (cold || rebooting || sc->ec_suspending) {
-	if ((EC_GET_CSR(sc) & EC_EVENT_SCI)) {
+	if ((EC_GET_CSR(sc) & EC_EVENT_SCI) &&
+	    atomic_cmpset_int(&sc->ec_sci_pend, 0, 1)) {
 	    CTR0(KTR_ACPI, "ec running gpe handler directly");
 	    EcGpeQueryHandler(sc);
 	}

--------------C1023E7E159D68047EF9349A--



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?f519f355-c851-bf58-7daf-fb45bc7b69e0>