diff mbox series

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

Message ID 20240126005541.1839038-2-komlodi@google.com (mailing list archive)
State New, archived
Headers show
Series hw/i2c: smbus: Reset fixes | expand

Commit Message

Joe Komlodi Jan. 26, 2024, 12:55 a.m. UTC 
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(-)

Comments

Peter Maydell Feb. 1, 2024, 3:24 p.m. UTC | #1 
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
Joe Komlodi Feb. 2, 2024, 8:44 p.m. UTC | #2 
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
diff mbox series

Patch

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;