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

[Peter Crosthwaite]

On Mon, Mar 3, 2014 at 12:06 AM, Beniamino Galvani <address@hidden> 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.
>

Nice catch! I think there was a conditional in the earlier versions of
the original series that caught this but it got lost in the later
reivew stages. I do however prefer this de-looped/coreified
implementation.

> Signed-off-by: Beniamino Galvani <address@hidden>
> ---
>  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;

"state" is a bit ambiguous for a variable name. I have seen "parent"
used in this context before (although I must confess that may get
confused with QOM terms)."container"?


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

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

... is a local variable to this for loop iteration. So it can be
defined here for easier reading.

Otherwise:

Reviewed-by: Peter Crosthwaite <address@hidden>

Regards,
Peter

> -        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);
>      }
> --
> 1.7.10.4
>
>