Re: [Qemu-devel] [PATCH v5 05/15] qdev: allow multiple qdev_init_gpio_in() calls

[Peter Crosthwaite]

On Mon, Aug 6, 2012 at 7:38 PM, Peter Maydell <address@hidden> wrote:
> On 6 August 2012 03:16, Peter A. G. Crosthwaite
> <address@hidden> wrote:
>> Allow multiple qdev_init_gpio_in() calls for the one device. The first call 
>> will
>> define GPIOs 0-N-1, the next GPIOs N- ... . Allows different GPIOs to be 
>> handled
>> with different handlers. Needed when two levels of the QOM class heirachy 
>> both
>> define GPIO functionality, as a single GPIO handler with an index selecter is
>> not possible.
>
> I generally like this idea and I think there are a few devices in the
> existing codebase that could profitably use this rather than trying
> to sort things out in the handler function. (Long term we would ideally
> replace the single gpio array with a set of named arrays of Pins but
> this is useful in the meantime.)
>
>> Signed-off-by: Peter A. G. Crosthwaite <address@hidden>
>> ---
>>  hw/qdev.c |   16 +++++++++++++---
>>  1 files changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/qdev.c b/hw/qdev.c
>> index b5b74b9..ce91a72 100644
>> --- a/hw/qdev.c
>> +++ b/hw/qdev.c
>> @@ -293,9 +293,19 @@ BusState *qdev_get_parent_bus(DeviceState *dev)
>>
>>  void qdev_init_gpio_in(DeviceState *dev, qemu_irq_handler handler, int n)
>>  {
>> -    assert(dev->num_gpio_in == 0);
>> -    dev->num_gpio_in = n;
>> -    dev->gpio_in = qemu_allocate_irqs(handler, dev, n);
>> +    qemu_irq *new_irqs = qemu_allocate_irqs(handler, dev, n);

RE comment below, this should be in the else condition, ill move it.

>> +
>> +    if (dev->num_gpio_in == 0) {
>> +        dev->gpio_in = qemu_allocate_irqs(handler, dev, n);
>
> In this case we've just called qemu_allocate_irqs() twice and leaked
> the copy in new_irqs.
>
>> +    } else {
>> +        qemu_irq *all_irqs = g_new(qemu_irq, n + dev->num_gpio_in);
>> +        memcpy(all_irqs, dev->gpio_in, sizeof(*all_irqs) * 
>> dev->num_gpio_in);
>> +        g_free(dev->gpio_in);
>> +        memcpy(&all_irqs[dev->num_gpio_in], new_irqs, sizeof(*all_irqs) * 
>> n),
>> +        g_free(new_irqs);
>
> I think this is rather looking into private details of what qemu_allocate_irqs
> does, and it would be better to define a new qemu_reallocate_irqs() in irq.c
> so we can use it here. (when doing so, consider whether g_renew() might help
> in producing nice looking code)

My understanding is Anthony is doing major refactoring in the GPIO
area anyways. Long term, will this code in qdev.c/irq.c even going to
exist? In this patch, I took something of a path of least resistance
to just make this series work, as I suspect this patch in its current
form will be short lived due to continued QOM development.

cc Andreas and Anthony.

Regards,
Peter

>
>
>> +        dev->gpio_in = all_irqs;
>> +    }
>> +    dev->num_gpio_in += n;
>>  }
>>
>>  void qdev_init_gpio_out(DeviceState *dev, qemu_irq *pins, int n)
>> --
>> 1.7.0.4
>>
>
>
> -- PMM