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

[Corey Minyard <minyard@acm.org>]

On 06/27/2016 07:43 PM, Alistair Francis wrote:
> On Mon, Jun 27, 2016 at 3:04 PM,  <minyard@acm.org> wrote:
>> From: Corey Minyard <cminyard@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.konrad@greensocs.com>
>> Cc: Alistair Francis <alistair.francis@xilinx.com>
>> Cc: Peter Crosthwaite <crosthwaite.peter@gmail.com>
>> Cc: Kwon <hyun.kwon@xilinx.com>
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Signed-off-by: Corey Minyard <cminyard@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.francis@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
>>
>>