From owner-freebsd-acpi@freebsd.org Tue Nov 29 15:52:08 2016 Return-Path: Delivered-To: freebsd-acpi@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 4B84AC5C300 for ; Tue, 29 Nov 2016 15:52:08 +0000 (UTC) (envelope-from hps@selasky.org) Received: from mail.turbocat.net (heidi.turbocat.net [88.198.202.214]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id D5A81122B for ; Tue, 29 Nov 2016 15:52:07 +0000 (UTC) (envelope-from hps@selasky.org) Received: from hps2016.home.selasky.org (unknown [62.141.129.119]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.turbocat.net (Postfix) with ESMTPSA id D18861FE156 for ; Tue, 29 Nov 2016 16:52:04 +0100 (CET) To: freebsd-acpi@freebsd.org From: Hans Petter Selasky Subject: Recursion in ACPI EC causes GPE lockup Message-ID: Date: Tue, 29 Nov 2016 16:51:46 +0100 User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="------------C1023E7E159D68047EF9349A" X-BeenThere: freebsd-acpi@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: ACPI and power management development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 29 Nov 2016 15:52:08 -0000 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--