[PATCH v2 0/3] hw/i2c: smbus: Reset fixes

[PATCH v2 0/3] hw/i2c: smbus: Reset fixes

[Joe Komlodi]

Changelog:

v1 -> v2
- Dropped 4th patch "hw/i2c: smbus: mux: Reset SMBusDevice state
on reset". After more testing and Corey's comment, I realized it
wasn't needed.

Original message:

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 (3):
  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/core.c        | 30 +++++++++++++++++++++++++-----
 hw/i2c/smbus_slave.c | 17 +++++++++++++++--
 include/hw/i2c/i2c.h |  6 +++++-
 3 files changed, 45 insertions(+), 8 deletions(-)

-- 
2.43.0.429.g432eaa2c6b-goog

[PATCH v2 1/3] 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.429.g432eaa2c6b-goog

Re: [PATCH v2 1/3] hw/i2c: core: Add reset

[Peter Maydell]

On Fri, 26 Jan 2024 at 00:56, Joe Komlodi <komlodi@google.com> wrote:
>
> 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);
> +    }

Doesn't it confuse the device that's partway through a
transaction if we just forget about the transaction entirely
without terminating it somehow? I'm not sure what real hardware
does in this situation, though.

> +}
> +
> +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,
> + };

Looks like you have stray extra spaces in front of this
type definition (which has then caused 'diff' to not notice
that you're only adding fields to the existing struct).

>
>  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;
> +};

This isn't correct -- a FooBusClass's parent_class field
should be a BusClass. But since you don't define any new
fields in it, you don't need to define the struct at all.

Instead, your TypeInfo for the TYPE_I2C_BUS can add a
.class_init member, and leave the .class_size unset
(it will then inherit the class-size from the parent
class, which will be sizeof(BusClass)).

> +
>  struct I2CBus {
>      BusState qbus;
>      I2CNodeList current_devs;
> --
> 2.43.0.429.g432eaa2c6b-goog

thanks
-- PMM

Re: [PATCH v2 1/3] hw/i2c: core: Add reset

[Joe Komlodi]

Hi peter,

On Thu, Feb 1, 2024 at 7:24 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Fri, 26 Jan 2024 at 00:56, Joe Komlodi <komlodi@google.com> wrote:
> >
> > 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);
> > +    }
>
> Doesn't it confuse the device that's partway through a
> transaction if we just forget about the transaction entirely
> without terminating it somehow? I'm not sure what real hardware
> does in this situation, though.
>

It could. Ideally all devices on the bus would have reset functions
implemented as well, so their state can reset.
With adding a bus-wide reset, what could end up happening is devices
without resets implemented end up in the wrong state compared to the
bus, while before they would stay in the same state as the bus.
However with the bus-wide reset, devices with resets now match their
state with the bus's state, while before there could be a mismatch.

Fixed the comments in this patch and in the other 2 patches in v3.

Thanks,
Joe

> > +}
> > +
> > +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,
> > + };
>
> Looks like you have stray extra spaces in front of this
> type definition (which has then caused 'diff' to not notice
> that you're only adding fields to the existing struct).
>
> >
> >  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;
> > +};
>
> This isn't correct -- a FooBusClass's parent_class field
> should be a BusClass. But since you don't define any new
> fields in it, you don't need to define the struct at all.
>
> Instead, your TypeInfo for the TYPE_I2C_BUS can add a
> .class_init member, and leave the .class_size unset
> (it will then inherit the class-size from the parent
> class, which will be sizeof(BusClass)).
>
> > +
> >  struct I2CBus {
> >      BusState qbus;
> >      I2CNodeList current_devs;
> > --
> > 2.43.0.429.g432eaa2c6b-goog
>
> thanks
> -- PMM

[PATCH v2 2/3] 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.429.g432eaa2c6b-goog

Re: [PATCH v2 2/3] hw/i2c/smbus_slave: Add object path on error prints

[Peter Maydell]

On Fri, 26 Jan 2024 at 00:56, Joe Komlodi <komlodi@google.com> wrote:
>
> 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

Ideally the uses of these macros should all be
tracepoints or uses of qemu_log_mask(LOG_GUEST_ERROR, ...),
but I'm OK with just making a minor improvement to the
existing macros.

However, object_get_canonical_path() returns a pointer
to allocated memory which the caller needs to free, so
this is leaking memory. You can fix that by making the
macros something like

#define BADF(fmt, ...) \
    do {               \
        g_autofree char *qom_path = object_get_canonical_path(OBJECT(dev)); \
        fprintf(stderr, etc); \
    } while (0)

The g_autofree will arrange for the memory to be freed when
execution leaves the {} block.

thanks
-- PMM

[PATCH v2 3/3] 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.429.g432eaa2c6b-goog

Re: [PATCH v2 3/3] hw/i2c: smbus_slave: Reset state on reset

[Peter Maydell]

On Fri, 26 Jan 2024 at 00:56, Joe Komlodi <komlodi@google.com> 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(+)
>
> 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;
> -

This should probably be the 'hold' phase of reset, unless there's
a strong reason to do it in the 'enter' phase.

thanks
-- PMM