[PATCH 0/4] hw/i2c: smbus: Reset fixes

[PATCH 0/4] hw/i2c: smbus: Reset fixes

[Joe Komlodi]

Hi all,

This series adds some resets for SMBus and for the I2C core. Along with
it, we make SMBus slave error printing a little more helpful.

These reset issues were very infrequent, they would maybe occur in 1 out
of hundreds of resets in our testing, but the way they happen is pretty
straightforward.
Basically as long as a reset happens in the middle of a transaction, the
state of the old transaction would still partially be there after the
reset. Once a new transaction comes in, the partial stale state can
cause the new transaction to incorrectly fail.

Thanks,
Joe

Joe Komlodi (4):
  hw/i2c: core: Add reset
  hw/i2c/smbus_slave: Add object path on error prints
  hw/i2c: smbus_slave: Reset state on reset
  hw/i2c: smbus: mux: Reset SMBusDevice state on reset

 hw/i2c/core.c                | 30 +++++++++++++++++++++++++-----
 hw/i2c/i2c_mux_pca954x.c     |  5 +++++
 hw/i2c/smbus_slave.c         | 20 ++++++++++++++++++--
 include/hw/i2c/i2c.h         |  6 +++++-
 include/hw/i2c/smbus_slave.h |  1 +
 5 files changed, 54 insertions(+), 8 deletions(-)

-- 
2.43.0.472.g3155946c3a-goog

[PATCH 1/4] 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        | 30 +++++++++++++++++++++++++-----
 include/hw/i2c/i2c.h |  6 +++++-
 2 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index 4cf30b2c86..def4f134d0 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -23,11 +23,31 @@ static Property i2c_props[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
-static const TypeInfo i2c_bus_info = {
-    .name = TYPE_I2C_BUS,
-    .parent = TYPE_BUS,
-    .instance_size = sizeof(I2CBus),
-};
+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),
+     .class_size = sizeof(I2CBusClass),
+     .class_init = i2c_bus_class_init,
+ };
 
 static int i2c_bus_pre_save(void *opaque)
 {
diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
index 2a3abacd1b..420868a269 100644
--- a/include/hw/i2c/i2c.h
+++ b/include/hw/i2c/i2c.h
@@ -64,7 +64,7 @@ struct I2CSlave {
 };
 
 #define TYPE_I2C_BUS "i2c-bus"
-OBJECT_DECLARE_SIMPLE_TYPE(I2CBus, I2C_BUS)
+OBJECT_DECLARE_TYPE(I2CBus, I2CBusClass, I2C_BUS)
 
 typedef struct I2CNode I2CNode;
 
@@ -83,6 +83,10 @@ struct I2CPendingMaster {
 typedef QLIST_HEAD(I2CNodeList, I2CNode) I2CNodeList;
 typedef QSIMPLEQ_HEAD(I2CPendingMasters, I2CPendingMaster) I2CPendingMasters;
 
+struct I2CBusClass {
+    DeviceClass parent_class;
+};
+
 struct I2CBus {
     BusState qbus;
     I2CNodeList current_devs;
-- 
2.43.0.472.g3155946c3a-goog

[PATCH 2/4] hw/i2c/smbus_slave: Add object path on error prints

[Joe Komlodi]

The current logging doesn't tell us which specific smbus device is an
error state.

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

diff --git a/hw/i2c/smbus_slave.c b/hw/i2c/smbus_slave.c
index 1300c9ec72..e24a1ef472 100644
--- a/hw/i2c/smbus_slave.c
+++ b/hw/i2c/smbus_slave.c
@@ -25,11 +25,15 @@
 #define DPRINTF(fmt, ...) \
 do { printf("smbus(%02x): " fmt , dev->i2c.address, ## __VA_ARGS__); } while (0)
 #define BADF(fmt, ...) \
-do { fprintf(stderr, "smbus: error: " fmt , ## __VA_ARGS__); exit(1);} while (0)
+do { fprintf(stderr, "%s: smbus: error: " fmt , \
+             object_get_canonical_path(OBJECT(dev)), ## __VA_ARGS__); \
+             exit(1); } while (0)
 #else
 #define DPRINTF(fmt, ...) do {} while(0)
 #define BADF(fmt, ...) \
-do { fprintf(stderr, "smbus: error: " fmt , ## __VA_ARGS__);} while (0)
+do { fprintf(stderr, "%s: smbus: error: " fmt , \
+             object_get_canonical_path(OBJECT(dev)), ## __VA_ARGS__); \
+             } while (0)
 #endif
 
 enum {
-- 
2.43.0.472.g3155946c3a-goog

[PATCH 3/4] 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 e24a1ef472..58abde29de 100644
--- a/hw/i2c/smbus_slave.c
+++ b/hw/i2c/smbus_slave.c
@@ -201,10 +201,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.43.0.472.g3155946c3a-goog

[PATCH 4/4] hw/i2c: smbus: mux: Reset SMBusDevice state on reset

[Joe Komlodi]

When a reset happens, both the SMBusDevice and PCA954x class do their
variable resetting on an enter reset. Because of this, only the PCA954x
has its reset called, which can leave the SMBusDevice in a bad state if
it was in the middle of a transaction.

To fix this we add parent reset functions for the SMBusDevice class, and
have the mux class invoke it when it resets.

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

diff --git a/hw/i2c/i2c_mux_pca954x.c b/hw/i2c/i2c_mux_pca954x.c
index db5db956a6..307359a518 100644
--- a/hw/i2c/i2c_mux_pca954x.c
+++ b/hw/i2c/i2c_mux_pca954x.c
@@ -159,8 +159,13 @@ static uint8_t pca954x_read_byte(SMBusDevice *d)
 static void pca954x_enter_reset(Object *obj, ResetType type)
 {
     Pca954xState *s = PCA954X(obj);
+    Pca954xClass *pc = PCA954X_GET_CLASS(obj);
+
     /* Reset will disable all channels. */
     pca954x_write(s, 0);
+    if (pc->parent.parent_phases.enter) {
+        pc->parent.parent_phases.enter(obj, type);
+    }
 }
 
 I2CBus *pca954x_i2c_get_bus(I2CSlave *mux, uint8_t channel)
diff --git a/hw/i2c/smbus_slave.c b/hw/i2c/smbus_slave.c
index 58abde29de..81adab8f77 100644
--- a/hw/i2c/smbus_slave.c
+++ b/hw/i2c/smbus_slave.c
@@ -212,8 +212,11 @@ static void smbus_device_class_init(ObjectClass *klass, void *data)
 {
     I2CSlaveClass *sc = I2C_SLAVE_CLASS(klass);
     ResettableClass *rc = RESETTABLE_CLASS(klass);
+    SMBusDeviceClass *sdc = SMBUS_DEVICE_CLASS(klass);
 
     rc->phases.enter = smbus_device_enter_reset;
+    resettable_class_set_parent_phases(rc, smbus_device_enter_reset, NULL, NULL,
+                                       &sdc->parent_phases);
     sc->event = smbus_i2c_event;
     sc->recv = smbus_i2c_recv;
     sc->send = smbus_i2c_send;
diff --git a/include/hw/i2c/smbus_slave.h b/include/hw/i2c/smbus_slave.h
index 86bfe0a79e..0dd14be178 100644
--- a/include/hw/i2c/smbus_slave.h
+++ b/include/hw/i2c/smbus_slave.h
@@ -35,6 +35,7 @@ OBJECT_DECLARE_TYPE(SMBusDevice, SMBusDeviceClass,
 
 struct SMBusDeviceClass {
     I2CSlaveClass parent_class;
+    ResettablePhases parent_phases;
 
     /*
      * An operation with no data, special in SMBus.
-- 
2.43.0.472.g3155946c3a-goog

Re: [PATCH 0/4] hw/i2c: smbus: Reset fixes

[Joe Komlodi]

+cminyard

Accidentally typed Corey's email address wrong in the initial send, oops.



On Wed, Jan 10, 2024 at 1:26 PM Joe Komlodi <komlodi@google.com> wrote:
>
> Hi all,
>
> This series adds some resets for SMBus and for the I2C core. Along with
> it, we make SMBus slave error printing a little more helpful.
>
> These reset issues were very infrequent, they would maybe occur in 1 out
> of hundreds of resets in our testing, but the way they happen is pretty
> straightforward.
> Basically as long as a reset happens in the middle of a transaction, the
> state of the old transaction would still partially be there after the
> reset. Once a new transaction comes in, the partial stale state can
> cause the new transaction to incorrectly fail.
>
> Thanks,
> Joe
>
> Joe Komlodi (4):
>   hw/i2c: core: Add reset
>   hw/i2c/smbus_slave: Add object path on error prints
>   hw/i2c: smbus_slave: Reset state on reset
>   hw/i2c: smbus: mux: Reset SMBusDevice state on reset
>
>  hw/i2c/core.c                | 30 +++++++++++++++++++++++++-----
>  hw/i2c/i2c_mux_pca954x.c     |  5 +++++
>  hw/i2c/smbus_slave.c         | 20 ++++++++++++++++++--
>  include/hw/i2c/i2c.h         |  6 +++++-
>  include/hw/i2c/smbus_slave.h |  1 +
>  5 files changed, 54 insertions(+), 8 deletions(-)
>
> --
> 2.43.0.472.g3155946c3a-goog
>

Re: [PATCH 0/4] hw/i2c: smbus: Reset fixes

[Corey Minyard]

On Wed, Jan 10, 2024 at 09:26:37PM +0000, Joe Komlodi wrote:
> Hi all,
> 
> This series adds some resets for SMBus and for the I2C core. Along with
> it, we make SMBus slave error printing a little more helpful.
> 
> These reset issues were very infrequent, they would maybe occur in 1 out
> of hundreds of resets in our testing, but the way they happen is pretty
> straightforward.
> Basically as long as a reset happens in the middle of a transaction, the
> state of the old transaction would still partially be there after the
> reset. Once a new transaction comes in, the partial stale state can
> cause the new transaction to incorrectly fail.

This seems wrong to me.  In a real system, the reset would be done on
the smbus master and not necessarily on the mux (though I looked at a
few of the PCA954x devices and they appear to have reset lines, but
different systems may drive that reset differently).

It seems to me that the bug is the smbus master device isn't getting
reset in a system reset.  Just adding the reset logic there would be
easier and more consistent with the real hardware.

-corey

> 
> Thanks,
> Joe
> 
> Joe Komlodi (4):
>   hw/i2c: core: Add reset
>   hw/i2c/smbus_slave: Add object path on error prints
>   hw/i2c: smbus_slave: Reset state on reset
>   hw/i2c: smbus: mux: Reset SMBusDevice state on reset
> 
>  hw/i2c/core.c                | 30 +++++++++++++++++++++++++-----
>  hw/i2c/i2c_mux_pca954x.c     |  5 +++++
>  hw/i2c/smbus_slave.c         | 20 ++++++++++++++++++--
>  include/hw/i2c/i2c.h         |  6 +++++-
>  include/hw/i2c/smbus_slave.h |  1 +
>  5 files changed, 54 insertions(+), 8 deletions(-)
> 
> -- 
> 2.43.0.472.g3155946c3a-goog
> 
> 

Re: [PATCH 0/4] hw/i2c: smbus: Reset fixes

[Joe Komlodi]

On Thu, Jan 11, 2024 at 6:03 AM Corey Minyard <minyard@acm.org> wrote:
>
> On Wed, Jan 10, 2024 at 09:26:37PM +0000, Joe Komlodi wrote:
> > Hi all,
> >
> > This series adds some resets for SMBus and for the I2C core. Along with
> > it, we make SMBus slave error printing a little more helpful.
> >
> > These reset issues were very infrequent, they would maybe occur in 1 out
> > of hundreds of resets in our testing, but the way they happen is pretty
> > straightforward.
> > Basically as long as a reset happens in the middle of a transaction, the
> > state of the old transaction would still partially be there after the
> > reset. Once a new transaction comes in, the partial stale state can
> > cause the new transaction to incorrectly fail.
>
> This seems wrong to me.  In a real system, the reset would be done on
> the smbus master and not necessarily on the mux (though I looked at a
> few of the PCA954x devices and they appear to have reset lines, but
> different systems may drive that reset differently).
>
> It seems to me that the bug is the smbus master device isn't getting
> reset in a system reset.  Just adding the reset logic there would be
> easier and more consistent with the real hardware.
>
> -corey
>
Oops, sorry, missed this in my inbox.

That sounds good to me, I'll send up a v2 that resets the SMBus master
instead of the mux.

Thanks,
Joe

> >
> > Thanks,
> > Joe
> >
> > Joe Komlodi (4):
> >   hw/i2c: core: Add reset
> >   hw/i2c/smbus_slave: Add object path on error prints
> >   hw/i2c: smbus_slave: Reset state on reset
> >   hw/i2c: smbus: mux: Reset SMBusDevice state on reset
> >
> >  hw/i2c/core.c                | 30 +++++++++++++++++++++++++-----
> >  hw/i2c/i2c_mux_pca954x.c     |  5 +++++
> >  hw/i2c/smbus_slave.c         | 20 ++++++++++++++++++--
> >  include/hw/i2c/i2c.h         |  6 +++++-
> >  include/hw/i2c/smbus_slave.h |  1 +
> >  5 files changed, 54 insertions(+), 8 deletions(-)
> >
> > --
> > 2.43.0.472.g3155946c3a-goog
> >
> >