From patchwork Tue Aug 2 16:00:14 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Corey Minyard X-Patchwork-Id: 73167 Delivered-To: patch@linaro.org Received: by 10.140.29.52 with SMTP id a49csp244832qga; Tue, 2 Aug 2016 09:06:41 -0700 (PDT) X-Received: by 10.55.11.12 with SMTP id 12mr23166478qkl.160.1470153998270; Tue, 02 Aug 2016 09:06:38 -0700 (PDT) Return-Path: Received: from lists.gnu.org (lists.gnu.org. [2001:4830:134:3::11]) by mx.google.com with ESMTPS id h7si1902423qtd.128.2016.08.02.09.06.38 for (version=TLS1 cipher=AES128-SHA bits=128/128); Tue, 02 Aug 2016 09:06:38 -0700 (PDT) Received-SPF: pass (google.com: domain of qemu-devel-bounces+patch=linaro.org@nongnu.org designates 2001:4830:134:3::11 as permitted sender) client-ip=2001:4830:134:3::11; Authentication-Results: mx.google.com; dkim=fail header.i=@gmail.com; spf=pass (google.com: domain of qemu-devel-bounces+patch=linaro.org@nongnu.org designates 2001:4830:134:3::11 as permitted sender) smtp.mailfrom=qemu-devel-bounces+patch=linaro.org@nongnu.org Received: from localhost ([::1]:57527 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bUcDF-0006gJ-Pm for patch@linaro.org; Tue, 02 Aug 2016 12:06:37 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39629) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bUc7A-0002Dc-U1 for qemu-devel@nongnu.org; Tue, 02 Aug 2016 12:00:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bUc78-0006mh-PV for qemu-devel@nongnu.org; Tue, 02 Aug 2016 12:00:19 -0400 Received: from mail-oi0-x241.google.com ([2607:f8b0:4003:c06::241]:34224) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bUc78-0006mc-JV for qemu-devel@nongnu.org; Tue, 02 Aug 2016 12:00:18 -0400 Received: by mail-oi0-x241.google.com with SMTP id c199so17277292oig.1 for ; Tue, 02 Aug 2016 09:00:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=sender:from:to:cc:subject:date:message-id; bh=y72NsaCngMAcLP15Xwzx8//D7br0EueK1k3olaNPqSs=; b=qEAGyGWGfFKQ0WqlF7lo9/7xfUDtnIAtKBr44PpyOJAB3DwABkKtYT9Mox1gJ2sTeY C76bYLOqMU3V2VCdwqXjfxAfMGlcxH1F7DiYwYBcmQ/b2VopWxJrR3lQO2O9EgOHltuK D/9Qi0xIZn9Zg3nrZ9ODlFcmXdPs+4kNmGbAey17V5JhRbiEPGJA7bqrQwqDryzBWaHa 7VIDeCnFlvbOeIaHWrbU8nRA/D7PN/bcpMfIrUM5qClfev2itn0yqe+yyeX2ol6rRCIV wxwcJNTr45BHu42+zBD6/gno9kcemOrNlZUO5SkpEDaG7SfPfcOumZJExskabTpqXgrh PS/w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:sender:from:to:cc:subject:date:message-id; bh=y72NsaCngMAcLP15Xwzx8//D7br0EueK1k3olaNPqSs=; b=Ik/TDtfKWXfSGHi4Spg9vscPi9YjdAdRFwTQIAtaaxt/h3lKKNmoTCKpsoBPBiLUh9 OMqrpqNQnWRMgtDb2gnJIYcByfiESsD5II2JS0KmdaWkUEX8SVz9gxGZR8sb3MwThcQA ukdapVmSu5tGExeyk14xqIzSJi6Y22AivF+lYvh5flDuHduYEVT1ugbRGwxXoIsMJXOw 8cJlDHLJj/U1xEFZcVXcZGvGPUo8LEmudt76jshGzAB1rxucz9mmfwVX6xRYPCJKbEd3 U9is/rhFd7os3SbriDOrAD7k4uQXWzpYarVNfkmKuZcY/X2M26orAdR66X9HeWYcLEok IMwg== X-Gm-Message-State: AEkoouuRc9OoynzYFExRoHelvPlQK9v4jZtXZHEFhC1PrG9JO2Gsu5eBPmrR0SM4iSUlvA== X-Received: by 10.157.52.200 with SMTP id t8mr40615476otd.90.1470153618008; Tue, 02 Aug 2016 09:00:18 -0700 (PDT) Received: from serve.minyard.net (serve.minyard.net. [2001:470:b8f6:1b::1]) by smtp.gmail.com with ESMTPSA id d25sm1510358otc.29.2016.08.02.09.00.17 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 02 Aug 2016 09:00:17 -0700 (PDT) Received: from t430.minyard.net (unknown [IPv6:2001:470:b8f6:1b:d6f:e2fa:cd5a:adde]) by serve.minyard.net (Postfix) with ESMTPA id 4A9FD6D3; Tue, 2 Aug 2016 11:00:16 -0500 (CDT) Received: by t430.minyard.net (Postfix, from userid 1000) id 11AF430053D; Tue, 2 Aug 2016 11:00:16 -0500 (CDT) From: minyard@acm.org To: qemu-devel@nongnu.org, minyard@acm.org Date: Tue, 2 Aug 2016 11:00:14 -0500 Message-Id: <1470153614-6657-1-git-send-email-minyard@acm.org> X-Mailer: git-send-email 2.7.4 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 2607:f8b0:4003:c06::241 Subject: [Qemu-devel] [PATCH v2] i2c: Fix SMBus read transactions to avoid double events X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Peter Maydell , Kwon , Corey Minyard , Peter Crosthwaite , Alistair Francis , KONRAD Frederic Errors-To: qemu-devel-bounces+patch=linaro.org@nongnu.org Sender: "Qemu-devel" From: Corey Minyard Change 2293c27faddf (i2c: implement broadcast write) added broadcast capability to the I2C bus, but it broke SMBus read transactions. An SMBus read transaction does two i2c_start_transaction() calls without an intervening i2c_end_transfer() call. This will result in i2c_start_transfer() adding the same device to the current_devs list twice, and then the ->event() for the same device gets called twice in the second call to i2c_start_transfer(), resulting in the smbus code getting confused. Note that this happens even with pure I2C devices when simulating SMBus over I2C. This fix only scans the bus if the current set of devices is empty. This means that the current set of devices stays fixed until i2c_end_transfer() is called, which is really what you want. This also deletes the empty check from the top of i2c_end_transfer(). It's unnecessary, and it prevents the broadcast variable from being set to false at the end of the transaction if no devices were on the bus. Cc: KONRAD Frederic Cc: Alistair Francis Cc: Peter Crosthwaite Cc: Kwon Cc: Peter Maydell Signed-off-by: Corey Minyard Reviewed-by: KONRAD Frederic Tested-by: KONRAD Frederic --- hw/i2c/core.c | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-) Can we get this in for the next release? SMBus is completely broken without it. -- 2.7.4 diff --git a/hw/i2c/core.c b/hw/i2c/core.c index abb3efb..8fd329b 100644 --- a/hw/i2c/core.c +++ b/hw/i2c/core.c @@ -101,15 +101,25 @@ int i2c_start_transfer(I2CBus *bus, uint8_t address, int recv) bus->broadcast = true; } - QTAILQ_FOREACH(kid, &bus->qbus.children, sibling) { - DeviceState *qdev = kid->child; - I2CSlave *candidate = I2C_SLAVE(qdev); - if ((candidate->address == address) || (bus->broadcast)) { - node = g_malloc(sizeof(struct I2CNode)); - node->elt = candidate; - QLIST_INSERT_HEAD(&bus->current_devs, node, next); - if (!bus->broadcast) { - break; + /* + * If there are already devices in the list, that means we are in + * the middle of a transaction and we shouldn't rescan the bus. + * + * This happens with any SMBus transaction, even on a pure I2C + * device. The interface does a transaction start without + * terminating the previous transaction. + */ + if (QLIST_EMPTY(&bus->current_devs)) { + QTAILQ_FOREACH(kid, &bus->qbus.children, sibling) { + DeviceState *qdev = kid->child; + I2CSlave *candidate = I2C_SLAVE(qdev); + if ((candidate->address == address) || (bus->broadcast)) { + node = g_malloc(sizeof(struct I2CNode)); + node->elt = candidate; + QLIST_INSERT_HEAD(&bus->current_devs, node, next); + if (!bus->broadcast) { + break; + } } } } @@ -134,10 +144,6 @@ void i2c_end_transfer(I2CBus *bus) I2CSlaveClass *sc; I2CNode *node, *next; - if (QLIST_EMPTY(&bus->current_devs)) { - return; - } - QLIST_FOREACH_SAFE(node, &bus->current_devs, next, next) { sc = I2C_SLAVE_GET_CLASS(node->elt); if (sc->event) {