diff mbox

[v2,3/7] allwinner-a10-pit: avoid generation of spurious interrupts

Message ID 1393769202-4551-4-git-send-email-b.galvani@gmail.com
State New
Headers show

Commit Message

Beniamino Galvani March 2, 2014, 2:06 p.m. UTC 
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.

Signed-off-by: Beniamino Galvani <b.galvani@gmail.com>
---
 hw/timer/allwinner-a10-pit.c |   30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

Comments

Peter Crosthwaite March 3, 2014, 11:08 a.m. UTC | #1 
On Mon, Mar 3, 2014 at 12:06 AM, Beniamino Galvani <b.galvani@gmail.com> 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 <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;

"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 <peter.crosthwaite@xilinx.com>

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
>
>
Beniamino Galvani March 3, 2014, 10:16 p.m. UTC | #2 
On Mon, Mar 03, 2014 at 09:08:27PM +1000, Peter Crosthwaite wrote:
> On Mon, Mar 3, 2014 at 12:06 AM, Beniamino Galvani <b.galvani@gmail.com> 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 <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;
> 
> "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.

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 <peter.crosthwaite@xilinx.com>
> 
> 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
> >
> >
Peter Crosthwaite March 4, 2014, 11:30 a.m. UTC | #3 
On Tue, Mar 4, 2014 at 8:16 AM, Beniamino Galvani <b.galvani@gmail.com> 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 <b.galvani@gmail.com> 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 <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;
>>
>> "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 <peter.crosthwaite@xilinx.com>
>>
>> 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
>> >
>> >
>
diff mbox

Patch

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