[Qemu-devel] [PATCH] i2c: Fix SMBus read transactions to avoid double events

[minyard]

From: Corey Minyard <address@hidden>

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 <address@hidden>
Cc: Alistair Francis <address@hidden>
Cc: Peter Crosthwaite <address@hidden>
Cc: Kwon <address@hidden>
Cc: Peter Maydell <address@hidden>
Signed-off-by: Corey Minyard <address@hidden>
---
 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?

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. */
+#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