From patchwork Sun Jun 15 00:41:17 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Lv Zheng X-Patchwork-Id: 4353771 Return-Path: X-Original-To: patchwork-linux-acpi@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 1D9B6BEEAA for ; Sun, 15 Jun 2014 00:41:53 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 2E0D92028D for ; Sun, 15 Jun 2014 00:41:52 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 1B8092025A for ; Sun, 15 Jun 2014 00:41:51 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751389AbaFOAle (ORCPT ); Sat, 14 Jun 2014 20:41:34 -0400 Received: from mail-pa0-f50.google.com ([209.85.220.50]:34217 "EHLO mail-pa0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750909AbaFOAld (ORCPT ); Sat, 14 Jun 2014 20:41:33 -0400 Received: by mail-pa0-f50.google.com with SMTP id bj1so1495894pad.37 for ; Sat, 14 Jun 2014 17:41:33 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=OMoHaPkieCM61Hqi7nJqnngIg3T4oiOb0LOnMSsFCmM=; b=Fru8zaYVGbUqbUnWUpgirfw5qT6ICbTHBTNEXngBtyi17Mtl+JoN+Dt3Jg7ExzVU+V fVS6r0REYNzHbZcijO34RtjnqjaqJcXw08GQqUj3co+twh/ZpXCSJwikWg+v/PEpaxcQ V6IoYZr5y72lod37aNpIZk57QFFjCjgBIsbXw+h6WIOJziUyr3VSrlqksTIBPPaeyLjN Kk4fA5GjDtKi6P0ofsmlT3k/ut6oJfyEsvFAW11vvVSJIzB62C75IynXQoKFi9BH59au KxRCVHrPaEZwMDB2mra89O3Xrtdx0/0Nu8VwnQzrjgz9Jr63Ljzs5LHW8MFMNuWBsaoI wMXg== X-Received: by 10.66.120.99 with SMTP id lb3mr14009258pab.2.1402792893241; Sat, 14 Jun 2014 17:41:33 -0700 (PDT) Received: from localhost.localdomain ([180.169.136.70]) by mx.google.com with ESMTPSA id dz4sm44452059pab.47.2014.06.14.17.41.29 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sat, 14 Jun 2014 17:41:32 -0700 (PDT) From: Lv Zheng To: "Rafael J. Wysocki" , Len Brown Cc: Lv Zheng , Lv Zheng , , linux-acpi@vger.kernel.org Subject: [PATCH v2 1/7] ACPI/EC: Fix an issue that advance_transaction() processes stale hardware status. Date: Sun, 15 Jun 2014 08:41:17 +0800 Message-Id: <469c34a869cfb1b3035e06d58d6eb2b01973bc20.1402792240.git.lv.zheng@intel.com> X-Mailer: git-send-email 1.7.10 In-Reply-To: References: Sender: linux-acpi-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-acpi@vger.kernel.org X-Spam-Status: No, score=-7.4 required=5.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, T_DKIM_INVALID, UNPARSEABLE_RELAY autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP From: Lv Zheng The advance_transaction() will be invoked from the IRQ context GPE handler and the task context ec_poll(). The handling of this function is locked so that the EC state machine are ensured to be advanced sequentially. But there is a problem. Before invoking advance_transaction(), EC_SC(R) is read. Then for advance_transaction(), there could be race condition around the lock from both contexts. The first one reading the register could fail this race and when it passes the stale register value to the state machine advancement code, the hardware condition is totally different from when the register is read. And the hardware accesses determined from the wrong hardware status can break the EC state machine. And there could be cases that the functionalities of the platform firmware are seriously affected. For example: 1. When 2 EC_DATA(W) writes compete the IBF=0, the 2nd EC_DATA(W) write may be invalid due to IBF=1 after the 1st EC_DATA(W) write. Then the hardware will either refuse to respond a next EC_SC(W) write of the next command or discard the current WR_EC command when it receives a EC_SC(W) write of the next command. 2. When 1 EC_SC(W) write and 1 EC_DATA(W) write compete the IBF=0, the EC_DATA(W) write may be invalid due to IBF=1 after the EC_SC(W) write. The next EC_DATA(R) could never be responded by the hardware. This is the root cause of the reported issue. This patch fixes this issue by moving the EC_SC(R) access into the lock so that we can ensure that the state machine is advanced under the right condition. Reference: https://bugzilla.kernel.org/show_bug.cgi?id=70891 Signed-off-by: Lv Zheng Reported-and-tested-by: Gareth Williams Tested-by: Steffen Weber [zetalog: first affected by:] Cc: # 2.6.11: 7c6db4e0: ACPI: EC: do transaction from interrupt context [zetalog: cleanly applying to:] Cc: # 3.14.x: 42b946bb: ACPI / EC: disable GPE before removing GPE handler --- drivers/acpi/ec.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c index ad11ba4..762b4cc 100644 --- a/drivers/acpi/ec.c +++ b/drivers/acpi/ec.c @@ -168,12 +168,15 @@ static void start_transaction(struct acpi_ec *ec) acpi_ec_write_cmd(ec, ec->curr->command); } -static void advance_transaction(struct acpi_ec *ec, u8 status) +static void advance_transaction(struct acpi_ec *ec) { unsigned long flags; struct transaction *t; + u8 status; spin_lock_irqsave(&ec->lock, flags); + pr_debug("===== %s =====\n", in_interrupt() ? "IRQ" : "TASK"); + status = acpi_ec_read_status(ec); t = ec->curr; if (!t) goto unlock; @@ -236,7 +239,7 @@ static int ec_poll(struct acpi_ec *ec) msecs_to_jiffies(1))) return 0; } - advance_transaction(ec, acpi_ec_read_status(ec)); + advance_transaction(ec); } while (time_before(jiffies, delay)); pr_debug("controller reset, restart transaction\n"); spin_lock_irqsave(&ec->lock, flags); @@ -635,11 +638,8 @@ static u32 acpi_ec_gpe_handler(acpi_handle gpe_device, u32 gpe_number, void *data) { struct acpi_ec *ec = data; - u8 status = acpi_ec_read_status(ec); - - pr_debug("~~~> interrupt, status:0x%02x\n", status); - advance_transaction(ec, status); + advance_transaction(ec); if (ec_transaction_done(ec) && (acpi_ec_read_status(ec) & ACPI_EC_FLAG_IBF) == 0) { wake_up(&ec->wait);