diff mbox

[v2] i2c: Fix SMBus read transactions to avoid double events

Message ID 1467142212-29810-1-git-send-email-minyard@acm.org (mailing list archive)
State New, archived
Headers show

Commit Message

Corey Minyard June 28, 2016, 7:30 p.m. UTC 
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.

Note that this happens even with pure I2C devices when simulating
SMBus over I2C.

This fix only scans the bus if the current set of devices is empty.
This means that the current set of devices stays fixed until
i2c_end_transfer() is called, which is really what you want.

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 | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

This fix should work with I2C devices as well as SMBus devices.

Sorry for not thinking it through all the way before.

Comments

KONRAD Frédéric July 6, 2016, 6:57 a.m. UTC | #1 
On 06/28/2016 09:30 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.

Hi Corey,

I didn't know that we can do two i2c_start_transfer without an
end_transfert in the middle. Maybe worth a comment in the code?

Otherwise:
Reviewed-by: KONRAD Frederic <fred.konrad@greensocs.com>
Tested-by: KONRAD Frederic <fred.konrad@greensocs.com>

Thanks,
Fred

> 
> Note that this happens even with pure I2C devices when simulating
> SMBus over I2C.
> 
> This fix only scans the bus if the current set of devices is empty.
> This means that the current set of devices stays fixed until
> i2c_end_transfer() is called, which is really what you want.
> 
> 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 | 28 +++++++++++++++-------------
>  1 file changed, 15 insertions(+), 13 deletions(-)
> 
> This fix should work with I2C devices as well as SMBus devices.
> 
> Sorry for not thinking it through all the way before.
> 
> diff --git a/hw/i2c/core.c b/hw/i2c/core.c
> index abb3efb..6313d31 100644
> --- a/hw/i2c/core.c
> +++ b/hw/i2c/core.c
> @@ -101,15 +101,21 @@ 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 there are already devices in the list, that means we are in
> +     * the middle of a transaction and we shouldn't rescan the bus.
> +     */
> +    if (QLIST_EMPTY(&bus->current_devs)) {
> +        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 +140,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) {
>
Corey Minyard July 6, 2016, 12:10 p.m. UTC | #2 
On 07/06/2016 01:57 AM, Frederic Konrad wrote:
> On 06/28/2016 09:30 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.
> Hi Corey,
>
> I didn't know that we can do two i2c_start_transfer without an
> end_transfert in the middle. Maybe worth a comment in the code?

I added:

      * This happens with any SMBus transaction, even on a pure I2C
      * device.  The interface does a transaction start without
      * terminating the previous transaction.


Thanks for checking this for me.

-corey

> Otherwise:
> Reviewed-by: KONRAD Frederic <fred.konrad@greensocs.com>
> Tested-by: KONRAD Frederic <fred.konrad@greensocs.com>
>
> Thanks,
> Fred
>
>> Note that this happens even with pure I2C devices when simulating
>> SMBus over I2C.
>>
>> This fix only scans the bus if the current set of devices is empty.
>> This means that the current set of devices stays fixed until
>> i2c_end_transfer() is called, which is really what you want.
>>
>> 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 | 28 +++++++++++++++-------------
>>   1 file changed, 15 insertions(+), 13 deletions(-)
>>
>> This fix should work with I2C devices as well as SMBus devices.
>>
>> Sorry for not thinking it through all the way before.
>>
>> diff --git a/hw/i2c/core.c b/hw/i2c/core.c
>> index abb3efb..6313d31 100644
>> --- a/hw/i2c/core.c
>> +++ b/hw/i2c/core.c
>> @@ -101,15 +101,21 @@ 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 there are already devices in the list, that means we are in
>> +     * the middle of a transaction and we shouldn't rescan the bus.
>> +     */
>> +    if (QLIST_EMPTY(&bus->current_devs)) {
>> +        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 +140,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 mbox

Patch

diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index abb3efb..6313d31 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -101,15 +101,21 @@  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 there are already devices in the list, that means we are in
+     * the middle of a transaction and we shouldn't rescan the bus.
+     */
+    if (QLIST_EMPTY(&bus->current_devs)) {
+        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 +140,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) {