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

[Peter Maydell <peter.maydell@linaro.org>]

On 8 May 2014 07:59, Peter Crosthwaite <peter.crosthwaite@xilinx.com> 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 <peter.crosthwaite@xilinx.com>
> ---
> 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.

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

> +    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?

>      }
>      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.

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

thanks
-- PMM