From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35464) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bHvlv-000691-89 for qemu-devel@nongnu.org; Tue, 28 Jun 2016 12:22:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bHvlq-0006Dl-Sb for qemu-devel@nongnu.org; Tue, 28 Jun 2016 12:21:58 -0400 Received: from mail-oi0-x241.google.com ([2607:f8b0:4003:c06::241]:35955) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bHvlq-0006Dh-Md for qemu-devel@nongnu.org; Tue, 28 Jun 2016 12:21:54 -0400 Received: by mail-oi0-x241.google.com with SMTP id x6so3537857oif.3 for ; Tue, 28 Jun 2016 09:21:54 -0700 (PDT) Sender: Corey Minyard Reply-To: minyard@acm.org References: <1467065065-3980-1-git-send-email-minyard@acm.org> From: Corey Minyard Message-ID: <5772A420.7030203@acm.org> Date: Tue, 28 Jun 2016 11:21:52 -0500 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] i2c: Fix SMBus read transactions to avoid double events List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alistair Francis Cc: "qemu-devel@nongnu.org Developers" , cminyard@mvista.com, Peter Maydell , Peter Crosthwaite , Kwon , KONRAD Frederic On 06/27/2016 07:43 PM, Alistair Francis wrote: > On Mon, Jun 27, 2016 at 3:04 PM, 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. >> >> This fix adds a third state to the i2c_start_transfer() recv >> parameter, a read continued that will not scan for devices >> and add them to current_devs. It also adds #defines for all >> the values for the recv parameter. >> >> 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 | 24 +++++++++++------------- >> hw/i2c/smbus.c | 22 +++++++++++----------- >> include/hw/i2c/i2c.h | 9 +++++++++ >> 3 files changed, 31 insertions(+), 24 deletions(-) >> >> I considered a couple of ways to do this. I thought about adding a >> separate function to do a "intermediate end" of the transaction, but >> that seemed like too much work. I also thought about adding a >> bool saing whether you are currently in a transaction and not rescan >> the bus if you are. However, that would require that the bool be in >> the vmstate, and that would be complicated. >> >> On that note, the current_devs list is not in the vmstate. That means >> that a migrate done in the middle of an I2C transaction will cause the >> I2C transaction to fail, right? Maybe this whole broadcast thing is >> a bad idea, or needs a different implementation? Looking at it a little closer, migration does appear to be handled correctly. So I'll stick with this patch. >> diff --git a/hw/i2c/core.c b/hw/i2c/core.c >> index abb3efb..53069dd 100644 >> --- a/hw/i2c/core.c >> +++ b/hw/i2c/core.c >> @@ -101,15 +101,17 @@ 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 (recv != I2C_START_CONTINUED_READ_TRANSFER) { >> + 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 +136,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) { >> diff --git a/hw/i2c/smbus.c b/hw/i2c/smbus.c >> index 3979b3d..f63799d 100644 >> --- a/hw/i2c/smbus.c >> +++ b/hw/i2c/smbus.c >> @@ -222,7 +222,7 @@ int smbus_receive_byte(I2CBus *bus, uint8_t addr) >> { >> uint8_t data; >> >> - if (i2c_start_transfer(bus, addr, 1)) { >> + if (i2c_start_transfer(bus, addr, I2C_START_READ_TRANSFER)) { >> return -1; >> } >> data = i2c_recv(bus); >> @@ -233,7 +233,7 @@ int smbus_receive_byte(I2CBus *bus, uint8_t addr) >> >> int smbus_send_byte(I2CBus *bus, uint8_t addr, uint8_t data) >> { >> - if (i2c_start_transfer(bus, addr, 0)) { >> + if (i2c_start_transfer(bus, addr, I2C_START_WRITE_TRANSFER)) { >> return -1; >> } >> i2c_send(bus, data); >> @@ -244,11 +244,11 @@ int smbus_send_byte(I2CBus *bus, uint8_t addr, uint8_t data) >> int smbus_read_byte(I2CBus *bus, uint8_t addr, uint8_t command) >> { >> uint8_t data; >> - if (i2c_start_transfer(bus, addr, 0)) { >> + if (i2c_start_transfer(bus, addr, I2C_START_WRITE_TRANSFER)) { >> return -1; >> } >> i2c_send(bus, command); >> - i2c_start_transfer(bus, addr, 1); >> + i2c_start_transfer(bus, addr, I2C_START_CONTINUED_READ_TRANSFER); >> data = i2c_recv(bus); >> i2c_nack(bus); >> i2c_end_transfer(bus); >> @@ -257,7 +257,7 @@ int smbus_read_byte(I2CBus *bus, uint8_t addr, uint8_t command) >> >> int smbus_write_byte(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t data) >> { >> - if (i2c_start_transfer(bus, addr, 0)) { >> + if (i2c_start_transfer(bus, addr, I2C_START_WRITE_TRANSFER)) { >> return -1; >> } >> i2c_send(bus, command); >> @@ -269,11 +269,11 @@ int smbus_write_byte(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t data) >> int smbus_read_word(I2CBus *bus, uint8_t addr, uint8_t command) >> { >> uint16_t data; >> - if (i2c_start_transfer(bus, addr, 0)) { >> + if (i2c_start_transfer(bus, addr, I2C_START_WRITE_TRANSFER)) { >> return -1; >> } >> i2c_send(bus, command); >> - i2c_start_transfer(bus, addr, 1); >> + i2c_start_transfer(bus, addr, I2C_START_CONTINUED_READ_TRANSFER); >> data = i2c_recv(bus); >> data |= i2c_recv(bus) << 8; >> i2c_nack(bus); >> @@ -283,7 +283,7 @@ int smbus_read_word(I2CBus *bus, uint8_t addr, uint8_t command) >> >> int smbus_write_word(I2CBus *bus, uint8_t addr, uint8_t command, uint16_t data) >> { >> - if (i2c_start_transfer(bus, addr, 0)) { >> + if (i2c_start_transfer(bus, addr, I2C_START_WRITE_TRANSFER)) { >> return -1; >> } >> i2c_send(bus, command); >> @@ -298,11 +298,11 @@ int smbus_read_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data) >> int len; >> int i; >> >> - if (i2c_start_transfer(bus, addr, 0)) { >> + if (i2c_start_transfer(bus, addr, I2C_START_WRITE_TRANSFER)) { >> return -1; >> } >> i2c_send(bus, command); >> - i2c_start_transfer(bus, addr, 1); >> + i2c_start_transfer(bus, addr, I2C_START_CONTINUED_READ_TRANSFER); >> len = i2c_recv(bus); >> if (len > 32) { >> len = 0; >> @@ -323,7 +323,7 @@ int smbus_write_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data, >> if (len > 32) >> len = 32; >> >> - if (i2c_start_transfer(bus, addr, 0)) { >> + if (i2c_start_transfer(bus, addr, I2C_START_WRITE_TRANSFER)) { >> return -1; >> } >> i2c_send(bus, command); >> diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h >> index c4085aa..16c910e 100644 >> --- a/include/hw/i2c/i2c.h >> +++ b/include/hw/i2c/i2c.h >> @@ -50,6 +50,15 @@ struct I2CSlave >> uint8_t address; >> }; >> >> +/* For the recv value in i2c_start_transfer. The first two values >> + correspond to false and true for the recv value. The third is a >> + special value that is used to tell i2c_start_transfer that this is >> + a continuation of the previous transfer, so don't rescan the bus >> + for devices to send to, continue with the current set of devices. */ > This comment is a little confusing, I don't think you need to explain > what the read/write values correspond to but you clear up the > explanation about the continued read value. Something like this I like > is clearer: > > The continued read is a special value that is used to tell the > i2c_start_transfer() function that this is a continuation of the > previous transfer so we don't rescan the bus for devices to send to > and instead just continue with the current set of devices. I did think about doing it this way, but it seemed clearer to me the other way. But I think you are right. -corey > Otherwise: > Reviewed-by: Alistair Francis > > Thanks, > > Alistair > > >> +#define I2C_START_WRITE_TRANSFER 0 >> +#define I2C_START_READ_TRANSFER 1 >> +#define I2C_START_CONTINUED_READ_TRANSFER 2 >> + >> I2CBus *i2c_init_bus(DeviceState *parent, const char *name); >> void i2c_set_slave_address(I2CSlave *dev, uint8_t address); >> int i2c_bus_busy(I2CBus *bus); >> -- >> 2.7.4 >> >>