[Qemu-devel] [PATCH] hw/pl031: Actually raise interrupt on timer expiry

[Qemu-devel] [PATCH] hw/pl031: Actually raise interrupt on timer expiry

[Peter Maydell]

Fix a typo in pl031_interrupt() which meant we were setting a bit
in the interrupt mask rather than the interrupt status register
and thus not actually raising an interrupt. This fix allows the
rtctest program from the kernel's Documentation/rtc.txt to pass
rather than hanging.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Looks like our PL031 has always had this bug since it was added
in 2007... Daniel Forsgren reported this, suggested the fix and
pointed me at the test program. Thanks!
 https://bugs.launchpad.net/qemu-linaro/+bug/931940

 hw/pl031.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/pl031.c b/hw/pl031.c
index 8416a60..f06b5ae 100644
--- a/hw/pl031.c
+++ b/hw/pl031.c
@@ -76,7 +76,7 @@ static void pl031_interrupt(void * opaque)
 {
     pl031_state *s = (pl031_state *)opaque;
 
-    s->im = 1;
+    s->is = 1;
     DPRINTF("Alarm raised\n");
     pl031_update(s);
 }
-- 
1.7.1

Re: [Qemu-devel] [PATCH] hw/pl031: Actually raise interrupt on timer expiry

[Andreas Färber]

Am 14.02.2012 18:40, schrieb Peter Maydell:
> Fix a typo in pl031_interrupt() which meant we were setting a bit
> in the interrupt mask rather than the interrupt status register
> and thus not actually raising an interrupt. This fix allows the
> rtctest program from the kernel's Documentation/rtc.txt to pass
> rather than hanging.
> 

Reported-by: Daniel Forsgren <daniel.forsgren@enea.com>

> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Looks like our PL031 has always had this bug since it was added
> in 2007... Daniel Forsgren reported this, suggested the fix and
> pointed me at the test program. Thanks!

Down here the credit for the find gets lost.

>  https://bugs.launchpad.net/qemu-linaro/+bug/931940
> 
>  hw/pl031.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/pl031.c b/hw/pl031.c
> index 8416a60..f06b5ae 100644
> --- a/hw/pl031.c
> +++ b/hw/pl031.c
> @@ -76,7 +76,7 @@ static void pl031_interrupt(void * opaque)
>  {
>      pl031_state *s = (pl031_state *)opaque;
>  
> -    s->im = 1;
> +    s->is = 1;
>      DPRINTF("Alarm raised\n");
>      pl031_update(s);
>  }

So on RTC_ICR write s->is = 0; but it was never set elsewhere, so
RTC_RIS would always return 0.

Acked-by: Andreas Färber <afaerber@suse.de>

However, to facilitate future review of these non-telling fields I
propose the following documentation patch as a follow-up:

diff --git a/hw/pl031.c b/hw/pl031.c
index 8416a60..a20c625 100644
--- a/hw/pl031.c
+++ b/hw/pl031.c
@@ -32,6 +32,11 @@ do { printf("pl031: " fmt , ## __VA_ARGS__); } while (0)
 #define RTC_MIS     0x18    /* Masked interrupt status register */
 #define RTC_ICR     0x1c    /* Interrupt clear register */

+/**
+ * pl031_state:
+ * @im: Interrupt mask.
+ * @is: Interrupt state.
+ */
 typedef struct {
     SysBusDevice busdev;
     MemoryRegion iomem;

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH] hw/pl031: Actually raise interrupt on timer expiry

[Peter Maydell]

On 15 February 2012 11:23, Andreas Färber <afaerber@suse.de> wrote:
> Am 14.02.2012 18:40, schrieb Peter Maydell:
>> Fix a typo in pl031_interrupt() which meant we were setting a bit
>> in the interrupt mask rather than the interrupt status register
>> and thus not actually raising an interrupt. This fix allows the
>> rtctest program from the kernel's Documentation/rtc.txt to pass
>> rather than hanging.
>>
>
> Reported-by: Daniel Forsgren <daniel.forsgren@enea.com>
>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>> Looks like our PL031 has always had this bug since it was added
>> in 2007... Daniel Forsgren reported this, suggested the fix and
>> pointed me at the test program. Thanks!
>
> Down here the credit for the find gets lost.
>
>>  https://bugs.launchpad.net/qemu-linaro/+bug/931940
>>
>>  hw/pl031.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/hw/pl031.c b/hw/pl031.c
>> index 8416a60..f06b5ae 100644
>> --- a/hw/pl031.c
>> +++ b/hw/pl031.c
>> @@ -76,7 +76,7 @@ static void pl031_interrupt(void * opaque)
>>  {
>>      pl031_state *s = (pl031_state *)opaque;
>>
>> -    s->im = 1;
>> +    s->is = 1;
>>      DPRINTF("Alarm raised\n");
>>      pl031_update(s);
>>  }
>
> So on RTC_ICR write s->is = 0; but it was never set elsewhere, so
> RTC_RIS would always return 0.
>
> Acked-by: Andreas Färber <afaerber@suse.de>
>
> However, to facilitate future review of these non-telling fields I
> propose the following documentation patch as a follow-up:

> +/**
> + * pl031_state:
> + * @im: Interrupt mask.
> + * @is: Interrupt state.
> + */

If they're that cryptic we should just rename them to int_mask
and int_status. Alternatively there's a good argument that
'im' should be 'imsc' since that's what the PL031 TRM calls
that register. By that logic 'is' would be 'ris'.

-- PMM