Re: [Qemu-devel] [PATCH v3 1/2] qdev: Implement named GPIOs

[Peter Crosthwaite]

On Sat, May 10, 2014 at 2:52 AM, Peter Maydell <address@hidden> wrote:
> On 8 May 2014 07:59, Peter Crosthwaite <address@hidden> wrote:
>> Implement named GPIOs on the Device layer. Listifies the existing GPIOs
>> stuff using string keys. Legacy un-named GPIOs are preserved by using
>> a NULL name string - they are just a single matchable element in the
>> name list.
>>
>> Signed-off-by: Peter Crosthwaite <address@hidden>
>> ---
>> Changed since v2:
>> Reordered fn args to be name-before-number (PMM review)
>> changed since v1:
>> Use QLIST instead of RYO list implementation (AF review)
>> Reorder struct fields for consistency
>>
>>  hw/core/qdev.c         | 70 
>> ++++++++++++++++++++++++++++++++++++++++++--------
>>  include/hw/qdev-core.h | 24 ++++++++++++++---
>>  qdev-monitor.c         | 14 ++++++----
>>  qtest.c                | 15 +++++++----
>>  4 files changed, 99 insertions(+), 24 deletions(-)
>
> So, first of all: having thought a bit about it I think we
> should go ahead with the approach this patch uses. Even
> if we later come up with some fancy QOM-object based way
> of doing GPIO lines, it should be easy enough to convert
> or put in back-compatible functions, and we'll be ahead
> if we've already done the effort of splitting single GPIO
> arrays into more sensible multiple named arrays.
>
> Some review follows.
>
>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> index 936eae6..8b54db2 100644
>> --- a/hw/core/qdev.c
>> +++ b/hw/core/qdev.c
>> @@ -312,30 +312,79 @@ BusState *qdev_get_parent_bus(DeviceState *dev)
>>      return dev->parent_bus;
>>  }
>>
>> +static NamedGPIOList *qdev_get_named_gpio_list(DeviceState *dev,
>> +                                               const char *name)
>> +{
>> +    NamedGPIOList *ngl;
>> +
>> +    QLIST_FOREACH(ngl, &dev->gpios, node) {
>> +        if (ngl->name == name ||
>> +                (name && ngl->name && !strcmp(name, ngl->name))) {
>
> This slightly odd looking check is because NULL is a
> valid name, yes? That could probably use a comment.
>

Added a comment. Also change conditional to be more self documenting
around NULL matching:

if (!(ngl->name && !name) ||

>> +            return ngl;
>> +        }
>> +    }
>> +
>> +    ngl = g_malloc0(sizeof(*ngl));
>> +    ngl->name = g_strdup(name);
>
> We're going to leak these when the device is deinited.
>
> The current gpio handling code is not exactly an airtight
> leakfree design, but let's see if we can improve things
> while we're here.
>
> I think device_finalize() needs to:
>  * iterate through the gpio list array
>  ** call qemu_free_irqs(ngl->in)
>  ** don't do anything with ngl->out because the caller of
>     qdev_init_gpio_out() owns that [usually embedded in their
>     QOM object struct]
>  ** free ngl->name
>  ** free the ngl node itself
>

Implemented.

>> +    QLIST_INSERT_HEAD(&dev->gpios, ngl, node);
>> +    return ngl;
>> +}
>> +
>> +void qdev_init_gpio_in_named(DeviceState *dev, qemu_irq_handler handler,
>> +                             const char *name, int n)
>> +{
>> +    NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
>> +
>> +    gpio_list->in = qemu_extend_irqs(gpio_list->in, gpio_list->num_in, 
>> handler,
>> +                                     dev, n);
>> +    gpio_list->num_in += n;
>> +}
>> +
>>  void qdev_init_gpio_in(DeviceState *dev, qemu_irq_handler handler, int n)
>>  {
>> -    dev->gpio_in = qemu_extend_irqs(dev->gpio_in, dev->num_gpio_in, handler,
>> -                                        dev, n);
>> -    dev->num_gpio_in += n;
>> +    qdev_init_gpio_in_named(dev, handler, NULL, n);
>> +}
>> +
>> +void qdev_init_gpio_out_named(DeviceState *dev, qemu_irq *pins,
>> +                              const char *name, int n)
>> +{
>> +    NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
>> +
>> +    assert(gpio_list->num_out == 0);
>> +    gpio_list->num_out = n;
>> +    gpio_list->out = pins;
>>  }
>>
>>  void qdev_init_gpio_out(DeviceState *dev, qemu_irq *pins, int n)
>>  {
>> -    assert(dev->num_gpio_out == 0);
>> -    dev->num_gpio_out = n;
>> -    dev->gpio_out = pins;
>> +    qdev_init_gpio_out_named(dev, pins, NULL, n);
>> +}
>> +
>> +qemu_irq qdev_get_gpio_in_named(DeviceState *dev, const char *name, int n)
>> +{
>> +    NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
>> +
>> +    assert(n >= 0 && n < gpio_list->num_in);
>> +    return gpio_list->in[n];
>>  }
>>
>>  qemu_irq qdev_get_gpio_in(DeviceState *dev, int n)
>>  {
>> -    assert(n >= 0 && n < dev->num_gpio_in);
>> -    return dev->gpio_in[n];
>> +    return qdev_get_gpio_in_named(dev, NULL, n);
>> +}
>> +
>> +void qdev_connect_gpio_out_named(DeviceState *dev, const char *name, int n,
>> +                                 qemu_irq pin)
>> +{
>> +    NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
>> +
>> +    assert(n >= 0 && n < gpio_list->num_out);
>> +    gpio_list->out[n] = pin;
>>  }
>>
>>  void qdev_connect_gpio_out(DeviceState * dev, int n, qemu_irq pin)
>>  {
>> -    assert(n >= 0 && n < dev->num_gpio_out);
>> -    dev->gpio_out[n] = pin;
>> +    qdev_connect_gpio_out_named(dev, NULL, n, pin);
>>  }
>>
>>  BusState *qdev_get_child_bus(DeviceState *dev, const char *name)
>> @@ -844,6 +893,7 @@ static void device_initfn(Object *obj)
>>      object_property_add_link(OBJECT(dev), "parent_bus", TYPE_BUS,
>>                               (Object **)&dev->parent_bus, NULL, 0,
>>                               &error_abort);
>> +    QLIST_INIT(&dev->gpios);
>>  }
>>
>>  static void device_post_init(Object *obj)
>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
>> index dbe473c..04fca4e 100644
>> --- a/include/hw/qdev-core.h
>> +++ b/include/hw/qdev-core.h
>> @@ -131,6 +131,17 @@ typedef struct DeviceClass {
>>      const char *bus_type;
>>  } DeviceClass;
>>
>> +typedef struct NamedGPIOList NamedGPIOList;
>> +
>> +struct NamedGPIOList {
>> +    const char *name;
>> +    qemu_irq *in;
>> +    int num_in;
>> +    qemu_irq *out;
>> +    int num_out;
>> +    QLIST_ENTRY(NamedGPIOList) node;
>> +};
>> +
>>  /**
>>   * DeviceState:
>>   * @realized: Indicates whether the device has been fully constructed.
>> @@ -148,10 +159,7 @@ struct DeviceState {
>>      QemuOpts *opts;
>>      int hotplugged;
>>      BusState *parent_bus;
>> -    int num_gpio_out;
>> -    qemu_irq *gpio_out;
>> -    int num_gpio_in;
>> -    qemu_irq *gpio_in;
>> +    QLIST_HEAD(, NamedGPIOList) gpios;
>>      QLIST_HEAD(, BusState) child_bus;
>>      int num_child_bus;
>>      int instance_id_alias;
>> @@ -252,7 +260,11 @@ void qdev_machine_creation_done(void);
>>  bool qdev_machine_modified(void);
>>
>>  qemu_irq qdev_get_gpio_in(DeviceState *dev, int n);
>> +qemu_irq qdev_get_gpio_in_named(DeviceState *dev, const char *name, int n);
>> +
>>  void qdev_connect_gpio_out(DeviceState *dev, int n, qemu_irq pin);
>> +void qdev_connect_gpio_out_named(DeviceState *dev, const char *name, int n,
>> +                                 qemu_irq pin);
>>
>>  BusState *qdev_get_child_bus(DeviceState *dev, const char *name);
>>
>> @@ -262,6 +274,10 @@ BusState *qdev_get_child_bus(DeviceState *dev, const 
>> char *name);
>>  /* GPIO inputs also double as IRQ sinks.  */
>>  void qdev_init_gpio_in(DeviceState *dev, qemu_irq_handler handler, int n);
>>  void qdev_init_gpio_out(DeviceState *dev, qemu_irq *pins, int n);
>> +void qdev_init_gpio_in_named(DeviceState *dev, qemu_irq_handler handler,
>> +                             const char *name, int n);
>> +void qdev_init_gpio_out_named(DeviceState *dev, qemu_irq *pins,
>> +                              const char *name, int n);
>>
>>  BusState *qdev_get_parent_bus(DeviceState *dev);
>>
>> diff --git a/qdev-monitor.c b/qdev-monitor.c
>> index 02cbe43..bb4c007 100644
>> --- a/qdev-monitor.c
>> +++ b/qdev-monitor.c
>> @@ -613,14 +613,18 @@ static void qdev_print(Monitor *mon, DeviceState *dev, 
>> int indent)
>>  {
>>      ObjectClass *class;
>>      BusState *child;
>> +    NamedGPIOList *ngl;
>> +
>>      qdev_printf("dev: %s, id \"%s\"\n", object_get_typename(OBJECT(dev)),
>>                  dev->id ? dev->id : "");
>>      indent += 2;
>> -    if (dev->num_gpio_in) {
>> -        qdev_printf("gpio-in %d\n", dev->num_gpio_in);
>> -    }
>> -    if (dev->num_gpio_out) {
>> -        qdev_printf("gpio-out %d\n", dev->num_gpio_out);
>> +    QLIST_FOREACH(ngl, &dev->gpios, node) {
>> +        if (ngl->num_in) {
>> +            qdev_printf("gpio-in \"%s\" %d\n", ngl->name, ngl->num_in);
>> +        }
>> +        if (ngl->num_out) {
>> +            qdev_printf("gpio-out \"%s\" %d\n", ngl->name, ngl->num_out);
>> +        }
>
> This is going to hand printf a NULL pointer for the name
> if it's printing the default gpio arrays, isn't it?
>

Fixed with a ? : check.

>>      }
>>      class = object_get_class(OBJECT(dev));
>>      do {
>> diff --git a/qtest.c b/qtest.c
>> index 2aba20d..36c317a 100644
>> --- a/qtest.c
>> +++ b/qtest.c
>> @@ -233,7 +233,8 @@ static void qtest_process_command(CharDriverState *chr, 
>> gchar **words)
>>      g_assert(command);
>>      if (strcmp(words[0], "irq_intercept_out") == 0
>>          || strcmp(words[0], "irq_intercept_in") == 0) {
>> -       DeviceState *dev;
>> +        DeviceState *dev;
>> +        NamedGPIOList *ngl;
>>
>>          g_assert(words[1]);
>>          dev = DEVICE(object_resolve_path(words[1], NULL));
>> @@ -253,10 +254,14 @@ static void qtest_process_command(CharDriverState 
>> *chr, gchar **words)
>>             return;
>>          }
>>
>> -        if (words[0][14] == 'o') {
>> -            qemu_irq_intercept_out(&dev->gpio_out, qtest_irq_handler, 
>> dev->num_gpio_out);
>> -        } else {
>> -            qemu_irq_intercept_in(dev->gpio_in, qtest_irq_handler, 
>> dev->num_gpio_in);
>> +        QLIST_FOREACH(ngl, &dev->gpios, node) {
>> +            if (words[0][14] == 'o') {
>> +                qemu_irq_intercept_out(&ngl->out, qtest_irq_handler,
>> +                                       ngl->num_out);
>> +            } else {
>> +                qemu_irq_intercept_in(ngl->in, qtest_irq_handler,
>> +                                      ngl->num_in);
>> +            }
>
> This means that if the qtest intercepts GPIOs then it
> won't be able to tell which GPIO array the IRQ raise/lower
> messages it gets correspond to (if you look at qtest_irq_handler
> the message that is emitted is just "IRQ raise %d" or
> "IRQ lower %d").
>
> I would suggest that you make this patch just retain
> the current behaviour of intercepting only the default
> gpio in/out arrays (ie the name==NULL ones). Then you
> can have a separate followup patch which fixes the
> qtest protocol messages (both the 'irq_intercept_{in,out}'
> ones from the test and the IRQ raise/lower replies) to
> include the gpio array name.
>

Done.

Respin on list. Thanks for the review.

Regards,
Peter

>>          }
>>          irq_intercept_dev = dev;
>>          qtest_send_prefix(chr);
>> --
>> 1.9.2.1.g06c4abd
>>
>
> thanks
> -- PMM
>