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

[Corey Minyard]

On 06/27/2016 07:43 PM, Alistair Francis wrote:

On Mon, Jun 27, 2016 at 3:04 PM,  <miny...@acm.org> wrote:

From: Corey Minyard <cminy...@mvista.com>

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 <fred.kon...@greensocs.com>
Cc: Alistair Francis <alistair.fran...@xilinx.com>
Cc: Peter Crosthwaite <crosthwaite.pe...@gmail.com>
Cc: Kwon <hyun.k...@xilinx.com>
Cc: Peter Maydell <peter.mayd...@linaro.org>
Signed-off-by: Corey Minyard <cminy...@mvista.com>
---
  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 <alistair.fran...@xilinx.com>

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