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

[Alistair Francis]

On Mon, Jun 27, 2016 at 3:04 PM,  <address@hidden> wrote:
> 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. */

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.

Otherwise:
Reviewed-by: Alistair Francis <address@hidden>

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
>
>