[PATCH 0/2] hw/i2c: Reset fixes

[PATCH 0/2] hw/i2c: Reset fixes

[Joe Komlodi]

Hi all,

This series fixes some I2C state variables not being reset when a reset
would happen.

These stale variables would infrequently cause issues, something around
the order of 5/1000 runs, since the machine would have to be reset at a
point where they would be in a state that would cause problems.

Thanks!
Joe

Joe Komlodi (2):
  hw/i2c: smbus_slave: Reset state on reset
  hw/i2c: core: Add reset

 hw/i2c/core.c        | 25 ++++++++++++++++++++++---
 hw/i2c/smbus_slave.c |  9 +++++++++
 2 files changed, 31 insertions(+), 3 deletions(-)

-- 
2.40.0.rc2.332.ga46443480c-goog

[PATCH 1/2] hw/i2c: smbus_slave: Reset state on reset

[Joe Komlodi]

If a reset comes while the SMBus device is not in its idle state, it's
possible for it to get confused on valid transactions post-reset.

Signed-off-by: Joe Komlodi <komlodi@google.com>
---
 hw/i2c/smbus_slave.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/i2c/smbus_slave.c b/hw/i2c/smbus_slave.c
index feb3ec6333..7b9d8780ae 100644
--- a/hw/i2c/smbus_slave.c
+++ b/hw/i2c/smbus_slave.c
@@ -197,10 +197,19 @@ static int smbus_i2c_send(I2CSlave *s, uint8_t data)
     return 0;
 }
 
+static void smbus_device_enter_reset(Object *obj, ResetType type)
+{
+    SMBusDevice *dev = SMBUS_DEVICE(obj);
+    dev->mode = SMBUS_IDLE;
+    dev->data_len = 0;
+}
+
 static void smbus_device_class_init(ObjectClass *klass, void *data)
 {
     I2CSlaveClass *sc = I2C_SLAVE_CLASS(klass);
+    ResettableClass *rc = RESETTABLE_CLASS(klass);
 
+    rc->phases.enter = smbus_device_enter_reset;
     sc->event = smbus_i2c_event;
     sc->recv = smbus_i2c_recv;
     sc->send = smbus_i2c_send;
-- 
2.40.0.rc2.332.ga46443480c-goog

Re: [PATCH 1/2] hw/i2c: smbus_slave: Reset state on reset

[Philippe Mathieu-Daudé]

On 20/3/23 23:14, Joe Komlodi wrote:
> If a reset comes while the SMBus device is not in its idle state, it's
> possible for it to get confused on valid transactions post-reset.
> 
> Signed-off-by: Joe Komlodi <komlodi@google.com>
> ---
>   hw/i2c/smbus_slave.c | 9 +++++++++
>   1 file changed, 9 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 2/2] hw/i2c: core: Add reset

[Joe Komlodi]

It's possible for a reset to come in the middle of a transaction, which
causes the bus to be in an old state when a new transaction comes in.

Signed-off-by: Joe Komlodi <komlodi@google.com>
---
 hw/i2c/core.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index bed594fe59..2aecbfb334 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -23,10 +23,29 @@ static Property i2c_props[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
+static void i2c_bus_enter_reset(Object *obj, ResetType type)
+{
+    I2CBus *bus = I2C_BUS(obj);
+    I2CNode *node, *next;
+
+    bus->broadcast = false;
+    QLIST_FOREACH_SAFE(node, &bus->current_devs, next, next) {
+        QLIST_REMOVE(node, next);
+        g_free(node);
+    }
+}
+
+static void i2c_bus_class_init(ObjectClass *klass, void *data)
+{
+    ResettableClass *rc = RESETTABLE_CLASS(klass);
+    rc->phases.enter = i2c_bus_enter_reset;
+}
+
 static const TypeInfo i2c_bus_info = {
-    .name = TYPE_I2C_BUS,
-    .parent = TYPE_BUS,
-    .instance_size = sizeof(I2CBus),
+   .name = TYPE_I2C_BUS,
+   .parent = TYPE_BUS,
+   .instance_size = sizeof(I2CBus),
+   .class_init = i2c_bus_class_init,
 };
 
 static int i2c_bus_pre_save(void *opaque)
-- 
2.40.0.rc2.332.ga46443480c-goog

Re: [PATCH 0/2] hw/i2c: Reset fixes

[Corey Minyard]

[-- Attachment #1: Type: text/plain, Size: 1184 bytes --]

On Mon, Mar 20, 2023 at 10:14:17PM +0000, Joe Komlodi wrote:
> Hi all,
> 
> This series fixes some I2C state variables not being reset when a reset
> would happen.
> 
> These stale variables would infrequently cause issues, something around
> the order of 5/1000 runs, since the machine would have to be reset at a
> point where they would be in a state that would cause problems.

These look good to me.  Definitely a missing needed function.  Looking
through the way it's handled, I think the proper things are being reset
and the proper ones are being left alone.  There's no checking of the
reset type, but there's only one reset type right now, so I guess any
changes due to reset type will have to come when new types come.

Acked-by: Corey Minyard <cminyard@mvista.com>

for another tree, or I can take them.

Thanks,

-corey

> 
> Thanks!
> Joe
> 
> Joe Komlodi (2):
>   hw/i2c: smbus_slave: Reset state on reset
>   hw/i2c: core: Add reset
> 
>  hw/i2c/core.c        | 25 ++++++++++++++++++++++---
>  hw/i2c/smbus_slave.c |  9 +++++++++
>  2 files changed, 31 insertions(+), 3 deletions(-)
> 
> -- 
> 2.40.0.rc2.332.ga46443480c-goog
> 

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3426 bytes --]