Re: [Qemu-devel] [PATCH 1/2] arm_mptimer: Fix timer shutdown and mode change

[Peter Crosthwaite]

On Sun, Jul 5, 2015 at 8:39 AM, Dmitry Osipenko <address@hidden> 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 the timer if enable bit isn't set.
>
You don't need to itemize one-shot and periodic separately, disabling
the running timer just doesn't work universally.

> Timer won't start periodic ticking if ONE-SHOT -> PERIODIC mode change
> happened after one-shot tick was completed. Fix it by starting ticking
> only if the timer isn't ticking right now.
>

Needs some grammar work. Try:

The running timer can't be stopped because timer control code just
doesn't handle disabling the timer. Fix it by deleting the timer if
the enable bit is cleared.

The timer won't start periodic ticking if a ONE-SHOT -> PERIODIC mode
change happens after a one-shot tick was completed. Fix it by
re-starting ticking if the timer isn't ticking right now.

> To avoid code churning, these two fixes are squashed in one commit.
>
> Signed-off-by: Dmitry Osipenko <address@hidden>

Otherwise:

Reviewed-by: Peter Crosthwaite <address@hidden>

Regards,
Peter

> ---
>
> Commits are squashed as per Peter Crosthwaite suggestion.
>
>  hw/timer/arm_mptimer.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c
> index 8b93b3c..0e132b1 100644
> --- a/hw/timer/arm_mptimer.c
> +++ b/hw/timer/arm_mptimer.c
> @@ -122,11 +122,18 @@ static void timerblock_write(void *opaque, hwaddr addr,
>      case 8: /* Control.  */
>          old = tb->control;
>          tb->control = value;
> -        if (((old & 1) == 0) && (value & 1)) {
> -            if (tb->count == 0 && (tb->control & 2)) {
> +        if (value & 1) {
> +            if ((old & 1) && (tb->count != 0)) {
> +                /* Do nothing if timer is ticking right now.  */
> +                break;
> +            }
> +            if (tb->control & 2) {
>                  tb->count = tb->load;
>              }
>              timerblock_reload(tb, 1);
> +        } else if (old & 1) {
> +            /* Shutdown the timer.  */
> +            timer_del(tb->timer);
>          }
>          break;
>      case 12: /* Interrupt status.  */
> --
> 2.4.4
>
>