From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60735) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bKlfO-0003ic-5A for qemu-devel@nongnu.org; Wed, 06 Jul 2016 08:10:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bKlfI-0001w4-Jp for qemu-devel@nongnu.org; Wed, 06 Jul 2016 08:10:56 -0400 Received: from mail-pa0-x241.google.com ([2607:f8b0:400e:c03::241]:35916) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bKlfI-0001vz-8N for qemu-devel@nongnu.org; Wed, 06 Jul 2016 08:10:52 -0400 Received: by mail-pa0-x241.google.com with SMTP id ib6so4892166pad.3 for ; Wed, 06 Jul 2016 05:10:52 -0700 (PDT) Sender: Corey Minyard Reply-To: minyard@acm.org References: <1467142212-29810-1-git-send-email-minyard@acm.org> <577CABF6.7010502@greensocs.com> From: Corey Minyard Message-ID: <577CF547.6040509@acm.org> Date: Wed, 6 Jul 2016 07:10:47 -0500 MIME-Version: 1.0 In-Reply-To: <577CABF6.7010502@greensocs.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2] i2c: Fix SMBus read transactions to avoid double events List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Frederic Konrad , qemu-devel@nongnu.org, cminyard@mvista.com Cc: Peter Maydell , Peter Crosthwaite , Alistair Francis , Kwon On 07/06/2016 01:57 AM, Frederic Konrad wrote: > On 06/28/2016 09:30 PM, minyard@acm.org wrote: >> 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. > Hi Corey, > > I didn't know that we can do two i2c_start_transfer without an > end_transfert in the middle. Maybe worth a comment in the code? I added: * This happens with any SMBus transaction, even on a pure I2C * device. The interface does a transaction start without * terminating the previous transaction. Thanks for checking this for me. -corey > Otherwise: > Reviewed-by: KONRAD Frederic > Tested-by: KONRAD Frederic > > Thanks, > Fred > >> 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 >> --- >> hw/i2c/core.c | 28 +++++++++++++++------------- >> 1 file changed, 15 insertions(+), 13 deletions(-) >> >> This fix should work with I2C devices as well as SMBus devices. >> >> Sorry for not thinking it through all the way before. >> >> diff --git a/hw/i2c/core.c b/hw/i2c/core.c >> index abb3efb..6313d31 100644 >> --- a/hw/i2c/core.c >> +++ b/hw/i2c/core.c >> @@ -101,15 +101,21 @@ 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. >> + */ >> + 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 +140,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) { >>