Re: [Qemu-devel] [PATCH 3/7] allwinner-a10-pit: avoid generation of spurious interrupts

[Beniamino Galvani <b.galvani@gmail.com>]

On Tue, Feb 18, 2014 at 12:17:04PM +0800, Li Guang wrote:
> Beniamino Galvani wrote:
> >The model was generating interrupts for all enabled timers after the
> >expiration of one of them. Avoid this by passing to the timer callback
> >function a structure containing the index of the expired timer.
> >
> did you detect spurious?
> didn't by my limited test,
> code will disable any expired timer, unless be re-armed,
> so it will never generate interrupt any more.

Yes, when both timer0 and timer1 were enabled and timer0 expired, the
previous implementation raised two interrupts because the callback
function iterated over the array of timers. Instead it should consider
only the timer that generated the event.

Beniamino

> >Signed-off-by: Beniamino Galvani<b.galvani@gmail.com>
> >---
> >  hw/timer/allwinner-a10-pit.c |   30 +++++++++++++++++++-----------
> >  1 file changed, 19 insertions(+), 11 deletions(-)
> >
> >diff --git a/hw/timer/allwinner-a10-pit.c b/hw/timer/allwinner-a10-pit.c
> >index b27fce8..3e1c183 100644
> >--- a/hw/timer/allwinner-a10-pit.c
> >+++ b/hw/timer/allwinner-a10-pit.c
> >@@ -19,6 +19,11 @@
> >  #include "sysemu/sysemu.h"
> >  #include "hw/timer/allwinner-a10-pit.h"
> >
> >+typedef struct TimerContext {
> >+    AwA10PITState *state;
> >+    int index;
> >+} TimerContext;
> >+
> >  static uint64_t a10_pit_read(void *opaque, hwaddr offset, unsigned size)
> >  {
> >      AwA10PITState *s = AW_A10_PIT(opaque);
> >@@ -193,18 +198,17 @@ static void a10_pit_reset(DeviceState *dev)
> >
> >  static void a10_pit_timer_cb(void *opaque)
> >  {
> >-    AwA10PITState *s = AW_A10_PIT(opaque);
> >-    uint8_t i;
> >+    TimerContext *tc = opaque;
> >+    AwA10PITState *s = tc->state;
> >+    uint8_t i = tc->index;
> >
> >-    for (i = 0; i<  AW_A10_PIT_TIMER_NR; i++) {
> >-        if (s->control[i]&  AW_A10_PIT_TIMER_EN) {
> >-            s->irq_status |= 1<<  i;
> >-            if (s->control[i]&  AW_A10_PIT_TIMER_MODE) {
> >-                ptimer_stop(s->timer[i]);
> >-                s->control[i]&= ~AW_A10_PIT_TIMER_EN;
> >-            }
> >-            qemu_irq_pulse(s->irq[i]);
> >+    if (s->control[i]&  AW_A10_PIT_TIMER_EN) {
> >+        s->irq_status |= 1<<  i;
> >+        if (s->control[i]&  AW_A10_PIT_TIMER_MODE) {
> >+            ptimer_stop(s->timer[i]);
> >+            s->control[i]&= ~AW_A10_PIT_TIMER_EN;
> >          }
> >+        qemu_irq_pulse(s->irq[i]);
> >      }
> >  }
> >
> >@@ -213,6 +217,7 @@ static void a10_pit_init(Object *obj)
> >      AwA10PITState *s = AW_A10_PIT(obj);
> >      SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> >      QEMUBH * bh[AW_A10_PIT_TIMER_NR];
> >+    TimerContext *tc;
> >      uint8_t i;
> >
> >      for (i = 0; i<  AW_A10_PIT_TIMER_NR; i++) {
> >@@ -223,7 +228,10 @@ static void a10_pit_init(Object *obj)
> >      sysbus_init_mmio(sbd,&s->iomem);
> >
> >      for (i = 0; i<  AW_A10_PIT_TIMER_NR; i++) {
> >-        bh[i] = qemu_bh_new(a10_pit_timer_cb, s);
> >+        tc = g_malloc(sizeof(TimerContext));
> >+        tc->state = s;
> >+        tc->index = i;
> >+        bh[i] = qemu_bh_new(a10_pit_timer_cb, tc);
> >          s->timer[i] = ptimer_init(bh[i]);
> >          ptimer_set_freq(s->timer[i], 240000);
> >      }
>