From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:40913)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <digetx@gmail.com>) id 1ZBq63-0000sR-7g
	for qemu-devel@nongnu.org; Sun, 05 Jul 2015 16:01:04 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <digetx@gmail.com>) id 1ZBq5z-0008E6-V4
	for qemu-devel@nongnu.org; Sun, 05 Jul 2015 16:01:03 -0400
Received: from mail-la0-x229.google.com ([2a00:1450:4010:c03::229]:35298)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <digetx@gmail.com>) id 1ZBq5z-0008Ds-M3
	for qemu-devel@nongnu.org; Sun, 05 Jul 2015 16:00:59 -0400
Received: by lagh6 with SMTP id h6so132360182lag.2
	for <qemu-devel@nongnu.org>; Sun, 05 Jul 2015 13:00:57 -0700 (PDT)
Message-ID: <55998CF2.1000000@gmail.com>
Date: Sun, 05 Jul 2015 23:00:50 +0300
From: Dmitry Osipenko <digetx@gmail.com>
MIME-Version: 1.0
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>
	<CAEgOgz7k0nSWddk5bKD0ZB4T4U5aeFoe8oG9-YdVouUsXoDZ6Q@mail.gmail.com>
In-Reply-To: <CAEgOgz7k0nSWddk5bKD0ZB4T4U5aeFoe8oG9-YdVouUsXoDZ6Q@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Subject: Re: [Qemu-devel] [PATCH 1/2] arm_mptimer: Fix timer shutdown and
 mode change
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Cc: Peter Maydell <peter.maydell@linaro.org>, QEMU Developers <qemu-devel@nongnu.org>, Paolo Bonzini <pbonzini@redhat.com>

05.07.2015 22:07, Peter Crosthwaite пишет:
>> -        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 can be either 0 or tb->load, so it shouldn't hurt to re-load it here.

>>                   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.

Yeah, still there is some room for optimizations.

Well, I think it would be more reasonable to implement pausing with a conversion 
to ptimer use, since it should result in a cleaner and simpler code.

-- 
Dmitry