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

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

[minyard]

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>
Reviewed-by: KONRAD Frederic <fred.konrad@greensocs.com>
Tested-by: KONRAD Frederic <fred.konrad@greensocs.com>
---
 hw/i2c/core.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

Can we get this in for the next release?  SMBus is completely
broken without it.

diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index abb3efb..8fd329b 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -101,15 +101,25 @@ 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.
+     *
+     * This happens with any SMBus transaction, even on a pure I2C
+     * device.  The interface does a transaction start without
+     * terminating the previous transaction.
+     */
+    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 +144,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) {
-- 
2.7.4

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

[Peter Maydell]

On 2 August 2016 at 17:00,  <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.
>
> 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>
> Reviewed-by: KONRAD Frederic <fred.konrad@greensocs.com>
> Tested-by: KONRAD Frederic <fred.konrad@greensocs.com>

Hi. Did this patch get lost, or was the bug fixed in a different
way instead?

I got here because Coverity complains that the
i2c_start_transfer() calls in smbus.c don't check their
return value. That suggests to me that we'd be better off
having a different function (i2c_restart_transfer() ??)
for the "I know we already did this once, so don't try to
re-determine who to send this to" case, rather than trying to
handle both cases in the same function.

> ---
>  hw/i2c/core.c | 32 +++++++++++++++++++-------------
>  1 file changed, 19 insertions(+), 13 deletions(-)
>
> Can we get this in for the next release?  SMBus is completely
> broken without it.

Is there an easy test case?

> diff --git a/hw/i2c/core.c b/hw/i2c/core.c
> index abb3efb..8fd329b 100644
> --- a/hw/i2c/core.c
> +++ b/hw/i2c/core.c
> @@ -101,15 +101,25 @@ 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.
> +     *
> +     * This happens with any SMBus transaction, even on a pure I2C
> +     * device.  The interface does a transaction start without
> +     * terminating the previous transaction.
> +     */
> +    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 +144,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) {
> --
> 2.7.4

thanks
-- PMM

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

[Corey Minyard]

On 10/21/2016 12:57 PM, Peter Maydell wrote:
> On 2 August 2016 at 17:00,  <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.
>>
>> 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>
>> Reviewed-by: KONRAD Frederic <fred.konrad@greensocs.com>
>> Tested-by: KONRAD Frederic <fred.konrad@greensocs.com>
> Hi. Did this patch get lost, or was the bug fixed in a different
> way instead?

It was lost, I'm probably doing something wrong, I'm not getting
much traction on getting patches into qemu.

>
> I got here because Coverity complains that the
> i2c_start_transfer() calls in smbus.c don't check their
> return value. That suggests to me that we'd be better off
> having a different function (i2c_restart_transfer() ??)
> for the "I know we already did this once, so don't try to
> re-determine who to send this to" case, rather than trying to
> handle both cases in the same function.

Perhaps so.  Or maybe i2c_continue_transfer().  That would
be more clear.  The second operation can't fail, but relying
on that is frail.

>> ---
>>   hw/i2c/core.c | 32 +++++++++++++++++++-------------
>>   1 file changed, 19 insertions(+), 13 deletions(-)
>>
>> Can we get this in for the next release?  SMBus is completely
>> broken without it.
> Is there an easy test case?
>

I was using an IPMI I2C device that I have developed, But that's not
in mainstream.  You should be able to just do an eeprom operation.
I haven't tested that, though.

-corey

>> diff --git a/hw/i2c/core.c b/hw/i2c/core.c
>> index abb3efb..8fd329b 100644
>> --- a/hw/i2c/core.c
>> +++ b/hw/i2c/core.c
>> @@ -101,15 +101,25 @@ 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.
>> +     *
>> +     * This happens with any SMBus transaction, even on a pure I2C
>> +     * device.  The interface does a transaction start without
>> +     * terminating the previous transaction.
>> +     */
>> +    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 +144,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) {
>> --
>> 2.7.4
> thanks
> -- PMM

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

[Peter Maydell]

On 21 October 2016 at 19:21, Corey Minyard <minyard@acm.org> wrote:
> On 10/21/2016 12:57 PM, Peter Maydell wrote:
>>
>> On 2 August 2016 at 17:00,  <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. Did this patch get lost, or was the bug fixed in a different
>> way instead?
>
>
> It was lost, I'm probably doing something wrong, I'm not getting
> much traction on getting patches into qemu.

The way our system works (for better or for worse) is that
until the patch is actively picked up by some subsystem
maintainer it's still "owned" by the patch submitter, so
you have to keep prodding people (by sending periodic 'ping'
emails, etc) until that happens. There is no mechanism for
automatically finding patches that got sent and never
applied. This is particularly important for patches like
this that don't obviously belong to any particular
well-maintained subsystem and will otherwise fall through
the gaps :-(

>> I got here because Coverity complains that the
>> i2c_start_transfer() calls in smbus.c don't check their
>> return value. That suggests to me that we'd be better off
>> having a different function (i2c_restart_transfer() ??)
>> for the "I know we already did this once, so don't try to
>> re-determine who to send this to" case, rather than trying to
>> handle both cases in the same function.
>
>
> Perhaps so.  Or maybe i2c_continue_transfer().  That would
> be more clear.  The second operation can't fail, but relying
> on that is frail.

i2c_continue_transfer() sounds like a better name, yes.
Would you like to write a patch that takes that approach?
If you cc me I'll review it and put it through the
target-arm tree.

thanks
-- PMM

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

[Corey Minyard]

On 10/22/2016 04:20 AM, Peter Maydell wrote:
>>> I got here because Coverity complains that the
>>> i2c_start_transfer() calls in smbus.c don't check their
>>> return value. That suggests to me that we'd be better off
>>> having a different function (i2c_restart_transfer() ??)
>>> for the "I know we already did this once, so don't try to
>>> re-determine who to send this to" case, rather than trying to
>>> handle both cases in the same function.
>>
>> Perhaps so.  Or maybe i2c_continue_transfer().  That would
>> be more clear.  The second operation can't fail, but relying
>> on that is frail.
> i2c_continue_transfer() sounds like a better name, yes.
> Would you like to write a patch that takes that approach?
> If you cc me I'll review it and put it through the
> target-arm tree.

I was working on this and looking at old information, and realized that
this won't work.  The same problem exists in I2C devices when
doing SMBus emulation, the device driver in the OS will cause a
i2c_start_transfer() to be done twice without an intervening
i2c_end_transfer().  Though a continue function would fix the smbus
case, it won't fix the i2c case.

I can't think of a better way to do it than I have done.

Where do you want to go on this?

-corey

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

[Peter Maydell]

On 23 October 2016 at 22:38, Corey Minyard <tcminyard@gmail.com> wrote:
> On 10/22/2016 04:20 AM, Peter Maydell wrote:
>>>>
>>>> I got here because Coverity complains that the
>>>> i2c_start_transfer() calls in smbus.c don't check their
>>>> return value. That suggests to me that we'd be better off
>>>> having a different function (i2c_restart_transfer() ??)
>>>> for the "I know we already did this once, so don't try to
>>>> re-determine who to send this to" case, rather than trying to
>>>> handle both cases in the same function.
>>>
>>>
>>> Perhaps so.  Or maybe i2c_continue_transfer().  That would
>>> be more clear.  The second operation can't fail, but relying
>>> on that is frail.
>>
>> i2c_continue_transfer() sounds like a better name, yes.
>> Would you like to write a patch that takes that approach?
>> If you cc me I'll review it and put it through the
>> target-arm tree.
>
>
> I was working on this and looking at old information, and realized that
> this won't work.  The same problem exists in I2C devices when
> doing SMBus emulation, the device driver in the OS will cause a
> i2c_start_transfer() to be done twice without an intervening
> i2c_end_transfer().  Though a continue function would fix the smbus
> case, it won't fix the i2c case.
>
> I can't think of a better way to do it than I have done.

OK, thanks for looking into this. I'll apply this patch to
target-arm.next, and we can think about something else for
the coverity issue. (Perhaps just assert() that i2c_start_transfer()
returns success? The bus contents shouldn't be able to change
I think...)

thanks
-- PMM

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

[Corey Minyard]

On 10/24/2016 09:14 AM, Peter Maydell wrote:
> On 23 October 2016 at 22:38, Corey Minyard <tcminyard@gmail.com> wrote:
>> On 10/22/2016 04:20 AM, Peter Maydell wrote:
>>>>> I got here because Coverity complains that the
>>>>> i2c_start_transfer() calls in smbus.c don't check their
>>>>> return value. That suggests to me that we'd be better off
>>>>> having a different function (i2c_restart_transfer() ??)
>>>>> for the "I know we already did this once, so don't try to
>>>>> re-determine who to send this to" case, rather than trying to
>>>>> handle both cases in the same function.
>>>>
>>>> Perhaps so.  Or maybe i2c_continue_transfer().  That would
>>>> be more clear.  The second operation can't fail, but relying
>>>> on that is frail.
>>> i2c_continue_transfer() sounds like a better name, yes.
>>> Would you like to write a patch that takes that approach?
>>> If you cc me I'll review it and put it through the
>>> target-arm tree.
>>
>> I was working on this and looking at old information, and realized that
>> this won't work.  The same problem exists in I2C devices when
>> doing SMBus emulation, the device driver in the OS will cause a
>> i2c_start_transfer() to be done twice without an intervening
>> i2c_end_transfer().  Though a continue function would fix the smbus
>> case, it won't fix the i2c case.
>>
>> I can't think of a better way to do it than I have done.
> OK, thanks for looking into this. I'll apply this patch to
> target-arm.next, and we can think about something else for
> the coverity issue. (Perhaps just assert() that i2c_start_transfer()
> returns success? The bus contents shouldn't be able to change
> I think...)

An assert should work fine, you are right, the bus list can't change
for the second call.  I'll do a patch and some tests to be sure.

-corey

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

[Corey Minyard]

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

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

[Frederic Konrad]

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

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

[minyard]

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.

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) {
-- 
2.7.4