From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55266) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZBpG9-0005q6-15 for qemu-devel@nongnu.org; Sun, 05 Jul 2015 15:07:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZBpG1-00025k-4b for qemu-devel@nongnu.org; Sun, 05 Jul 2015 15:07:24 -0400 Received: from mail-yk0-f178.google.com ([209.85.160.178]:33668) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZBpG1-00025W-0W for qemu-devel@nongnu.org; Sun, 05 Jul 2015 15:07:17 -0400 Received: by ykeo3 with SMTP id o3so11133165yke.0 for ; Sun, 05 Jul 2015 12:07:16 -0700 (PDT) MIME-Version: 1.0 Sender: peter.crosthwaite@petalogix.com In-Reply-To: <1436110742-6190-2-git-send-email-digetx@gmail.com> References: <1435877531-24983-1-git-send-email-digetx@gmail.com> <1436110742-6190-1-git-send-email-digetx@gmail.com> <1436110742-6190-2-git-send-email-digetx@gmail.com> Date: Sun, 5 Jul 2015 12:07:16 -0700 Message-ID: From: Peter Crosthwaite Content-Type: text/plain; charset=UTF-8 Subject: Re: [Qemu-devel] [PATCH 1/2] arm_mptimer: Fix timer shutdown and mode change List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Dmitry Osipenko Cc: Peter Maydell , QEMU Developers , Paolo Bonzini On Sun, Jul 5, 2015 at 8:39 AM, Dmitry Osipenko 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. > > 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. > > To avoid code churning, these two fixes are squashed in one commit. > > Signed-off-by: Dmitry Osipenko > --- > > 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) { So when the timer was previously disabled (!(old & 1)) and the count is non-zero this will cause a spurious auto-reload. I don't this causes a bug today because the code as-is doesn't support arbitrary count values, but it is a developer trap should the assumption that tb->count equals either 0 or the reload value not hold true. > tb->count = tb->load; > } > timerblock_reload(tb, 1); > + } else if (old & 1) { > + /* Shutdown the timer. */ > + timer_del(tb->timer); In general, this seems to now dup the code paths for control and load/counter writes. Both now have a del and reload call for various changes-of state. I had a go to see if I can consolidate. Turns out, doing so should implement timer pause and resumption while fixing both of your bugs, I'll send some patches. Regards, Peter > } > break; > case 12: /* Interrupt status. */ > -- > 2.4.4 > >