|
From: | Corey Minyard |
Subject: | Re: [Qemu-devel] [PATCH v2] i2c: Fix SMBus read transactions to avoid double events |
Date: | Fri, 21 Oct 2016 13:21:35 -0500 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 |
On 10/21/2016 12:57 PM, Peter Maydell wrote:
On 2 August 2016 at 17:00, <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. 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 <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> Reviewed-by: KONRAD Frederic <address@hidden> Tested-by: KONRAD Frederic <address@hidden>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.4thanks -- PMM
[Prev in Thread] | Current Thread | [Next in Thread] |