Patchwork hw/pl031: Actually raise interrupt on timer expiry

login
register
mail settings
Submitter Andreas Färber
Date Feb. 15, 2012, 11:23 a.m.
Message ID <4F3B95AA.8090007@suse.de>
Download mbox | patch
Permalink /patch/141312/
State New
Headers show

Comments

Andreas Färber - Feb. 15, 2012, 11:23 a.m.
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:


Andreas
Peter Maydell - Feb. 15, 2012, 11:34 a.m.
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

Patch

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;