| Message ID | 20230531203559.29140-4-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/timer/arm_timer: QOM'ify ARM_TIMER and correct sysbus/irq in ICP_PIT | expand |
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
On 8/6/23 16:41, Peter Maydell wrote: > 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(+) >> +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.) Thank for the suggestion, I didn't notice the OR.
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); + } +} + static Property sp804_properties[] = { DEFINE_PROP_UINT32("freq0", SP804State, freq0, 1000000), DEFINE_PROP_UINT32("freq1", SP804State, freq1, 1000000), @@ -320,6 +329,7 @@ static void sp804_class_init(ObjectClass *klass, void *data) DeviceClass *k = DEVICE_CLASS(klass); k->realize = sp804_realize; + k->unrealize = sp804_unrealize; device_class_set_props(k, sp804_properties); k->vmsd = &vmstate_sp804; }
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(+)