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>