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

[Peter Crosthwaite]

On Tue, Mar 4, 2014 at 8:16 AM, Beniamino Galvani <address@hidden> wrote:
> On Mon, Mar 03, 2014 at 09:08:27PM +1000, Peter Crosthwaite wrote:
>> 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"?
>
> "container" gives me the idea that the TimerContext is a member of the
> AwA10PITState struct, which is not the case.
>

Thinking more about this, it probably should be. There is a movement
(although occasionally disputed) to minimise malloc/pointerified state
and inline structs as much as possible. Currently you just leak the
malloc'ed objects which is not the cleanest style and the leak becomes
real in the event of dynamically added/removed devices (will be a
reality one day). Just inline the structs and do away with the malloc.
Then container will be fitting.

Regards,
Peter

> Is "pit_state" better?
>
>>
>> > +    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.
>
> Ok.
>
> Beniamino
>
>>
>> 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
>> >
>> >
>