Re: [PATCH 03/15] hw/timer/arm_timer: Add missing sp804_unrealize() handler

[Peter Maydell]

On Wed, 31 May 2023 at 21:36, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Release the IRQs allocated in sp804_realize() in the
> corresponding sp804_unrealize() handler.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/timer/arm_timer.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/hw/timer/arm_timer.c b/hw/timer/arm_timer.c
> index 36f6586f80..5caf42649a 100644
> --- a/hw/timer/arm_timer.c
> +++ b/hw/timer/arm_timer.c
> @@ -309,6 +309,15 @@ static void sp804_realize(DeviceState *dev, Error **errp)
>      s->timer[1]->irq = qemu_allocate_irq(sp804_set_irq, s, 1);
>  }
>
> +static void sp804_unrealize(DeviceState *dev)
> +{
> +    SP804State *s = SP804(dev);
> +
> +    for (unsigned i = 0; i < ARRAY_SIZE(s->timer); i++) {
> +        qemu_free_irq(s->timer[i]->irq);
> +    }
> +}

I don't really see the purpose in this. It doesn't actually
avoid a leak if we ever destroy an SP804, because
s->timer[i] itself is memory allocated by arm_timer_init()
which we don't clean up (the arm_timer_state not being
a qdev). If we did convert arm_timer_state to qdev
then these interrupts should turn into being sysbus irqs
and gpio inputs on the relevant devices. (In fact if you
were willing to take the migration-compat hit you
could just have the sp804 create a 2 input OR gate and
wire the arm_timer irqs up to it and use the output
of the OR gate as the sp804 outbound interrupt line.)

thanks
-- PMM