[Qemu-devel] [PATCH] ARM: hw/exynos4210_mct.c: Fix a bug which hangs Linux kernel.

[Qemu-devel] [PATCH] ARM: hw/exynos4210_mct.c: Fix a bug which hangs Linux kernel.

[Evgeny Voevodin]

From: Stanislav Vorobiov <s.vorobiov@samsung.com>

After some long period of time Linux kernel hanged due to
ptimer_get_count may return 0 before timer interrupt occurs,
thus, causing FRC to jump back in time

Signed-off-by: Evgeny Voevodin <e.voevodin@samsung.com>
---
 hw/exynos4210_mct.c |    4 ----
 1 file changed, 4 deletions(-)

diff --git a/hw/exynos4210_mct.c b/hw/exynos4210_mct.c
index 7474fcf..7a22b1f 100644
--- a/hw/exynos4210_mct.c
+++ b/hw/exynos4210_mct.c
@@ -376,10 +376,6 @@ static uint64_t exynos4210_gfrc_get_count(Exynos4210MCTGT *s)
 {
     uint64_t count = 0;
     count = ptimer_get_count(s->ptimer_frc);
-    if (!count) {
-        /* Timer event was generated and s->reg.cnt holds adequate value */
-        return s->reg.cnt;
-    }
     count = s->count - count;
     return s->reg.cnt + count;
 }
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH] ARM: hw/exynos4210_mct.c: Fix a bug which hangs Linux kernel.

[Peter Crosthwaite]

Hi Evgeny,

Im just speculating here, but I recently ran into Linux hangs on
Microblaze due to ptimer issues and I think you may be suffering the
same base issue.

The Microblaze timer (hw/xilinx_timer.c) has a similar implementation
to the exynos (chained one-shot ptimer). Recently Peter Chubb put
patch cf36b31db209a261ee3bc2737e788e1ced0a1bec through, which modified
ptimer to not set excessively short periods. Problem is, that only
works for periodic ptimers.

Anyways, you may find that changing your set_count() calls to
set_limit (i.e. the function designed for periodic timers) calls works
for you, without changing the logic of your device. Heres the change
pattern:

-    ptimer_set_count(ptimer, count);
+    ptimer_set_limit(ptimer, count, 1);

More permanently (and a question for the cheif maintainers) can we
look into ways of fixing ptimer properly?

Regards,
Peter

On Fri, Jun 22, 2012 at 5:22 PM, Evgeny Voevodin <e.voevodin@samsung.com> wrote:
> From: Stanislav Vorobiov <s.vorobiov@samsung.com>
>
> After some long period of time Linux kernel hanged due to
> ptimer_get_count may return 0 before timer interrupt occurs,
> thus, causing FRC to jump back in time
>
> Signed-off-by: Evgeny Voevodin <e.voevodin@samsung.com>

Reviewed-by Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>

> ---
>  hw/exynos4210_mct.c |    4 ----
>  1 file changed, 4 deletions(-)
>
> diff --git a/hw/exynos4210_mct.c b/hw/exynos4210_mct.c
> index 7474fcf..7a22b1f 100644
> --- a/hw/exynos4210_mct.c
> +++ b/hw/exynos4210_mct.c
> @@ -376,10 +376,6 @@ static uint64_t exynos4210_gfrc_get_count(Exynos4210MCTGT *s)
>  {
>     uint64_t count = 0;
>     count = ptimer_get_count(s->ptimer_frc);
> -    if (!count) {
> -        /* Timer event was generated and s->reg.cnt holds adequate value */
> -        return s->reg.cnt;
> -    }
>     count = s->count - count;
>     return s->reg.cnt + count;
>  }
> --
> 1.7.9.5
>
>

Re: [Qemu-devel] [PATCH] ARM: hw/exynos4210_mct.c: Fix a bug which hangs Linux kernel.

[Evgeny Voevodin]

On 22.06.2012 11:56, Peter Crosthwaite wrote:
> Hi Evgeny,
>
> Im just speculating here, but I recently ran into Linux hangs on
> Microblaze due to ptimer issues and I think you may be suffering the
> same base issue.
>
> The Microblaze timer (hw/xilinx_timer.c) has a similar implementation
> to the exynos (chained one-shot ptimer). Recently Peter Chubb put
> patch cf36b31db209a261ee3bc2737e788e1ced0a1bec through, which modified
> ptimer to not set excessively short periods. Problem is, that only
> works for periodic ptimers.

No, that's another problem. MCT was developed earlier then this commit 
landed
and it contains similar code to avoid such situation. But our patch 
fixes bug in logic.
The thing is that since ptimer uses BHs, it can be the situation when ptimer
is stopped but BH is not called yet. And exactly in this moment target reads
counter value that was incorrect calculated.

> Anyways, you may find that changing your set_count() calls to
> set_limit (i.e. the function designed for periodic timers) calls works
> for you, without changing the logic of your device. Heres the change
> pattern:
>
> -    ptimer_set_count(ptimer, count);
> +    ptimer_set_limit(ptimer, count, 1);
>
> More permanently (and a question for the cheif maintainers) can we
> look into ways of fixing ptimer properly?
>
> Regards,
> Peter
>
> On Fri, Jun 22, 2012 at 5:22 PM, Evgeny Voevodin<e.voevodin@samsung.com>  wrote:
>> From: Stanislav Vorobiov<s.vorobiov@samsung.com>
>>
>> After some long period of time Linux kernel hanged due to
>> ptimer_get_count may return 0 before timer interrupt occurs,
>> thus, causing FRC to jump back in time
>>
>> Signed-off-by: Evgeny Voevodin<e.voevodin@samsung.com>
> Reviewed-by Peter A. G. Crosthwaite<peter.crosthwaite@petalogix.com>

Thanks.

>> ---
>>   hw/exynos4210_mct.c |    4 ----
>>   1 file changed, 4 deletions(-)
>>
>> diff --git a/hw/exynos4210_mct.c b/hw/exynos4210_mct.c
>> index 7474fcf..7a22b1f 100644
>> --- a/hw/exynos4210_mct.c
>> +++ b/hw/exynos4210_mct.c
>> @@ -376,10 +376,6 @@ static uint64_t exynos4210_gfrc_get_count(Exynos4210MCTGT *s)
>>   {
>>      uint64_t count = 0;
>>      count = ptimer_get_count(s->ptimer_frc);
>> -    if (!count) {
>> -        /* Timer event was generated and s->reg.cnt holds adequate value */
>> -        return s->reg.cnt;
>> -    }
>>      count = s->count - count;
>>      return s->reg.cnt + count;
>>   }
>> --
>> 1.7.9.5
>>
>>


-- 
Kind regards,
Evgeny Voevodin,
Leading Software Engineer,
ASWG, Moscow R&D center, Samsung Electronics
e-mail: e.voevodin@samsung.com