[Qemu-devel] [PATCH] arm_mptimer: Fix timer shutdown

[Qemu-devel] [PATCH] arm_mptimer: Fix timer shutdown

[Dmitry Osipenko]

Timer, running in periodic mode, can't be stopped or coming one-shot tick
won't be canceled because timer control code just doesn't handle timer
disabling. Fix it by checking enable bit and deleting timer if bit isn't set.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 hw/timer/arm_mptimer.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c
index 8b93b3c..917a10f 100644
--- a/hw/timer/arm_mptimer.c
+++ b/hw/timer/arm_mptimer.c
@@ -127,6 +127,9 @@ static void timerblock_write(void *opaque, hwaddr addr,
                 tb->count = tb->load;
             }
             timerblock_reload(tb, 1);
+        } else if (!(value & 1)) {
+            /* Shutdown timer.  */
+            timer_del(tb->timer);
         }
         break;
     case 12: /* Interrupt status.  */
-- 
2.4.4

Re: [Qemu-devel] [PATCH] arm_mptimer: Fix timer shutdown

[Peter Maydell]

On 1 July 2015 at 22:15, Dmitry Osipenko <digetx@gmail.com> wrote:
> Timer, running in periodic mode, can't be stopped or coming one-shot tick
> won't be canceled because timer control code just doesn't handle timer
> disabling. Fix it by checking enable bit and deleting timer if bit isn't set.
>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  hw/timer/arm_mptimer.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c
> index 8b93b3c..917a10f 100644
> --- a/hw/timer/arm_mptimer.c
> +++ b/hw/timer/arm_mptimer.c
> @@ -127,6 +127,9 @@ static void timerblock_write(void *opaque, hwaddr addr,
>                  tb->count = tb->load;
>              }
>              timerblock_reload(tb, 1);
> +        } else if (!(value & 1)) {
> +            /* Shutdown timer.  */
> +            timer_del(tb->timer);
>          }
>          break;
>      case 12: /* Interrupt status.  */

Thanks; this does look like a bug. This change will mean we
call timer_del() even if the timer was already disabled,
though, so I think it would be slightly better to rearrange
the existing logic something like this:

  if ((old & 1) != (value & 1)) {
     if (value & 1) {
         if (tb->count == 0 && (tb->control & 2)) {
             tb->count = tb->load;
         }
         timerblock_reload(tb, 1);
     } else {
         timer_del(tb->timer);
     }
  }

thanks
-- PMM

Re: [Qemu-devel] [PATCH] arm_mptimer: Fix timer shutdown

[Dmitry Osipenko]

Hello Peter, thanks a lot for comment.

02.07.2015 12:27, Peter Maydell пишет:
> Thanks; this does look like a bug. This change will mean we
> call timer_del() even if the timer was already disabled,
> though, so I think it would be slightly better to rearrange
> the existing logic something like this:
>
>    if ((old & 1) != (value & 1)) {
>       if (value & 1) {
>           if (tb->count == 0 && (tb->control & 2)) {
>               tb->count = tb->load;
>           }
>           timerblock_reload(tb, 1);
>       } else {
>           timer_del(tb->timer);
>       }
>    }
>
> thanks
> -- PMM
>

Calling timer_del() twice is safe, but I agree that it would be better to avoid 
it. I'll send V2.

-- 
Dmitry

[Qemu-devel] [PATCH v2] arm_mptimer: Fix timer shutdown

[Dmitry Osipenko]

Timer, running in periodic mode, can't be stopped or coming one-shot tick
won't be canceled because timer control code just doesn't handle timer
disabling. Fix it by deleting timer if enable bit isn't set.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---

v2: Avoid calling timer_del() if the timer was already disabled as per
    Peter Maydell suggestion.

 hw/timer/arm_mptimer.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c
index 8b93b3c..51c18de 100644
--- a/hw/timer/arm_mptimer.c
+++ b/hw/timer/arm_mptimer.c
@@ -122,11 +122,17 @@ static void timerblock_write(void *opaque, hwaddr addr,
     case 8: /* Control.  */
         old = tb->control;
         tb->control = value;
-        if (((old & 1) == 0) && (value & 1)) {
+        if (((old & 1) == 0) && ((value & 1) == 0)) {
+            break;
+        }
+        if (value & 1) {
             if (tb->count == 0 && (tb->control & 2)) {
                 tb->count = tb->load;
             }
             timerblock_reload(tb, 1);
+        } else {
+            /* Shutdown timer.  */
+            timer_del(tb->timer);
         }
         break;
     case 12: /* Interrupt status.  */
-- 
2.4.4

Re: [Qemu-devel] [PATCH v2] arm_mptimer: Fix timer shutdown

[Peter Maydell]

On 2 July 2015 at 18:20, Dmitry Osipenko <digetx@gmail.com> wrote:
> Timer, running in periodic mode, can't be stopped or coming one-shot tick
> won't be canceled because timer control code just doesn't handle timer
> disabling. Fix it by deleting timer if enable bit isn't set.
>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>
> v2: Avoid calling timer_del() if the timer was already disabled as per
>     Peter Maydell suggestion.
>
>  hw/timer/arm_mptimer.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c
> index 8b93b3c..51c18de 100644
> --- a/hw/timer/arm_mptimer.c
> +++ b/hw/timer/arm_mptimer.c
> @@ -122,11 +122,17 @@ static void timerblock_write(void *opaque, hwaddr addr,
>      case 8: /* Control.  */
>          old = tb->control;
>          tb->control = value;
> -        if (((old & 1) == 0) && (value & 1)) {
> +        if (((old & 1) == 0) && ((value & 1) == 0)) {
> +            break;
> +        }
> +        if (value & 1) {
>              if (tb->count == 0 && (tb->control & 2)) {
>                  tb->count = tb->load;
>              }
>              timerblock_reload(tb, 1);
> +        } else {
> +            /* Shutdown timer.  */
> +            timer_del(tb->timer);


This will now cause us to do the "reload the timer"
logic if you write a 1 to the control bit when it was
already 1, which we didn't do before.

The logic I suggested in my previous review
comment gets this right...

-- PMM

Re: [Qemu-devel] [PATCH v2] arm_mptimer: Fix timer shutdown

[Dmitry Osipenko]

02.07.2015 20:34, Peter Maydell пишет:
> On 2 July 2015 at 18:20, Dmitry Osipenko <digetx@gmail.com> wrote:
>> Timer, running in periodic mode, can't be stopped or coming one-shot tick
>> won't be canceled because timer control code just doesn't handle timer
>> disabling. Fix it by deleting timer if enable bit isn't set.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>
>> v2: Avoid calling timer_del() if the timer was already disabled as per
>>      Peter Maydell suggestion.
>>
>>   hw/timer/arm_mptimer.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c
>> index 8b93b3c..51c18de 100644
>> --- a/hw/timer/arm_mptimer.c
>> +++ b/hw/timer/arm_mptimer.c
>> @@ -122,11 +122,17 @@ static void timerblock_write(void *opaque, hwaddr addr,
>>       case 8: /* Control.  */
>>           old = tb->control;
>>           tb->control = value;
>> -        if (((old & 1) == 0) && (value & 1)) {
>> +        if (((old & 1) == 0) && ((value & 1) == 0)) {
>> +            break;
>> +        }
>> +        if (value & 1) {
>>               if (tb->count == 0 && (tb->control & 2)) {
>>                   tb->count = tb->load;
>>               }
>>               timerblock_reload(tb, 1);
>> +        } else {
>> +            /* Shutdown timer.  */
>> +            timer_del(tb->timer);
>
>
> This will now cause us to do the "reload the timer"
> logic if you write a 1 to the control bit when it was
> already 1, which we didn't do before.
>
> The logic I suggested in my previous review
> comment gets this right...
>
> -- PMM
>

Yep, you right. I overlooked it, let me try again.

-- 
Dmitry

Re: [Qemu-devel] [PATCH v2] arm_mptimer: Fix timer shutdown

[Dmitry Osipenko]

02.07.2015 20:34, Peter Maydell пишет:
>
> This will now cause us to do the "reload the timer"
> logic if you write a 1 to the control bit when it was
> already 1, which we didn't do before.
>
> The logic I suggested in my previous review
> comment gets this right...
>
> -- PMM
>

The problem with code you suggested is that won't start periodic count after 
one-shot tick was completed.

-- 
Dmitry

Re: [Qemu-devel] [PATCH v2] arm_mptimer: Fix timer shutdown

[Peter Maydell]

On 2 July 2015 at 18:52, Dmitry Osipenko <digetx@gmail.com> wrote:
> 02.07.2015 20:34, Peter Maydell пишет:
>>
>>
>> This will now cause us to do the "reload the timer"
>> logic if you write a 1 to the control bit when it was
>> already 1, which we didn't do before.
>>
>> The logic I suggested in my previous review
>> comment gets this right...
>>
>> -- PMM
>>
>
> The problem with code you suggested is that won't start periodic count after
> one-shot tick was completed.

Can you give more detail? This code is only for when
the guest writes to the control register, so it doesn't
get run when a one-shot tick completes.

In any case, the code currently in master does:
  old value   new value    action
      0           0          nothing
      0           1          reload timer
      1           0          nothing
      1           1          nothing

Your first patch did:

  old value   new value    action
      0           0          delete timer
      0           1          reload timer
      1           0          delete timer
      1           1          nothing

Your second patch does:

  old value   new value    action
      0           0          nothing
      0           1          reload timer
      1           0          delete timer
      1           1          reload timer

My suggestion was:

  old value   new value    action
      0           0          nothing
      0           1          reload timer
      1           0          delete timer
      1           1          nothing

If you think that's wrong, then surely your first
patch also has the same problem?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2] arm_mptimer: Fix timer shutdown

[Dmitry Osipenko]

02.07.2015 21:09, Peter Maydell пишет:
> On 2 July 2015 at 18:52, Dmitry Osipenko <digetx@gmail.com> wrote:
>> 02.07.2015 20:34, Peter Maydell пишет:
>>>
>>>
>>> This will now cause us to do the "reload the timer"
>>> logic if you write a 1 to the control bit when it was
>>> already 1, which we didn't do before.
>>>
>>> The logic I suggested in my previous review
>>> comment gets this right...
>>>
>>> -- PMM
>>>
>>
>> The problem with code you suggested is that won't start periodic count after
>> one-shot tick was completed.
>
> Can you give more detail? This code is only for when
> the guest writes to the control register, so it doesn't
> get run when a one-shot tick completes.
>
> In any case, the code currently in master does:
>    old value   new value    action
>        0           0          nothing
>        0           1          reload timer
>        1           0          nothing
>        1           1          nothing
>
> Your first patch did:
>
>    old value   new value    action
>        0           0          delete timer
>        0           1          reload timer
>        1           0          delete timer
>        1           1          nothing
>
> Your second patch does:
>
>    old value   new value    action
>        0           0          nothing
>        0           1          reload timer
>        1           0          delete timer
>        1           1          reload timer
>
> My suggestion was:
>
>    old value   new value    action
>        0           0          nothing
>        0           1          reload timer
>        1           0          delete timer
>        1           1          nothing
>
> If you think that's wrong, then surely your first
> patch also has the same problem?
>
> thanks
> -- PMM
>

Yes, my first patch has same problem. Noticed that issue couple hours ago, it's 
separate bug as I see now... Will make patch for it too.

To clarify new issue:
	1) load TIMER_LOAD with some value
	2) write (TIMER_CONTROL_ENABLE | TIMER_CONTROL_ONESHOT | 
TIMER_CONTROL_IT_ENABLE) to TIMER_CONTROL
	3) wait for one-shot complete
	4) re-load TIMER_LOAD
	5) write (TIMER_CONTROL_ENABLE | TIMER_CONTROL_PERIODIC | 
TIMER_CONTROL_IT_ENABLE) to TIMER_CONTROL <---- it won't start, bug


Oh, and just noticed that timer code doesn't handle IT(interrupt enable) bit. 
Will fix it too.

-- 
Dmitry

Re: [Qemu-devel] [PATCH v2] arm_mptimer: Fix timer shutdown

[Dmitry Osipenko]

02.07.2015 21:43, Dmitry Osipenko пишет:
> 02.07.2015 21:09, Peter Maydell пишет:
> TIMER_CONTROL_IT_ENABLE) to TIMER_CONTROL <---- it won't start, bug
>

s/it won't start/it won't start periodic/

-- 
Dmitry

Re: [Qemu-devel] [PATCH v2] arm_mptimer: Fix timer shutdown

[Dmitry Osipenko]

02.07.2015 21:43, Dmitry Osipenko пишет:
>      4) re-load TIMER_LOAD
>

And 4-th step can be omitted.

Anyway, I already sent patches fixing this and IT issues.

-- 
Dmitry