From patchwork Mon Oct 24 17:25:21 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Peter Maydell X-Patchwork-Id: 686022 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3t2k4v4fGGz9s65 for ; Tue, 25 Oct 2016 04:37:35 +1100 (AEDT) Received: from localhost ([::1]:48513 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1byjBk-00007h-8p for incoming@patchwork.ozlabs.org; Mon, 24 Oct 2016 13:37:32 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35231) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1byj0J-0006RK-8U for qemu-devel@nongnu.org; Mon, 24 Oct 2016 13:25:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1byj0I-0007ST-08 for qemu-devel@nongnu.org; Mon, 24 Oct 2016 13:25:43 -0400 Received: from orth.archaic.org.uk ([2001:8b0:1d0::2]:47423) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1byj0H-0007Rd-K9 for qemu-devel@nongnu.org; Mon, 24 Oct 2016 13:25:41 -0400 Received: from pm215 by orth.archaic.org.uk with local (Exim 4.84_2) (envelope-from ) id 1byj0G-0002QV-Nv for qemu-devel@nongnu.org; Mon, 24 Oct 2016 18:25:40 +0100 From: Peter Maydell To: qemu-devel@nongnu.org Date: Mon, 24 Oct 2016 18:25:21 +0100 Message-Id: <1477329928-26414-26-git-send-email-peter.maydell@linaro.org> X-Mailer: git-send-email 2.7.4 In-Reply-To: <1477329928-26414-1-git-send-email-peter.maydell@linaro.org> References: <1477329928-26414-1-git-send-email-peter.maydell@linaro.org> X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 2001:8b0:1d0::2 Subject: [Qemu-devel] [PULL 25/32] 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: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.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 Message-id: 1470153614-6657-1-git-send-email-minyard@acm.org Signed-off-by: Peter Maydell --- hw/i2c/core.c | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/hw/i2c/core.c b/hw/i2c/core.c index 4afbe0b..bd8f167 100644 --- a/hw/i2c/core.c +++ b/hw/i2c/core.c @@ -104,15 +104,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; + } } } } @@ -137,10 +147,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) {