Re: [Qemu-devel] [PATCH v2] hw/ptimer: Don't wrap around counter for expired timer that uses tick handler

[Peter Maydell]

On 4 July 2016 at 10:55, Peter Maydell <address@hidden> wrote:
> On 1 July 2016 at 18:49, Dmitry Osipenko <address@hidden> wrote:
>> On 01.07.2016 19:36, Peter Maydell wrote:
>>> On 30 June 2016 at 20:01, Dmitry Osipenko <address@hidden> wrote:
>>>> On 30.06.2016 18:02, Peter Maydell wrote:
>>>>> What I meant was: ptimer_get_count() is typically called to generate
>>>>> a value to return from a register. That's a separate thing, conceptually,
>>>>> from whether the device happens to also trigger an interrupt on timer
>>>>> expiry by passing a bh to ptimer_init(). So it's very odd for a detail
>>>>> of interrupt-on-timer-expiry (that there is a bottom half) to affect
>>>>> the value returned when you read the timer count register.
>>>
>>>> In order to handle wraparound correctly, software needs to track the 
>>>> moment of
>>>> the wraparound - the interrupt. If software reads wrapped around counter 
>>>> value
>>>> before IRQ triggered (ptimer expired), then it would assume that no 
>>>> wraparound
>>>> happened and won't perform counter value correction, resulting in periodic
>>>> counter "jumping" backwards.
>>>
>>> That just says you need particular behaviour between counter reads
>>> and IRQ triggers; it doesn't say that you need the behaviour to be
>>> different if the ptimer code doesn't know about the IRQ trigger.
>>>
>>
>> Okay, I already explained the reason for having two different behaviours - to
>> make polled counter value more distributed when possible. If I understand you
>> correctly, you don't like it because it is "odd" and I agree that it's a bit 
>> clumsy.
>
>> So, what we are going to do now? Would you just revert the offending commit 
>> or
>> you have some other suggestions?
>
> Well, we need to fix the regression, but basically I'm kind of
> confused at the moment. I haven't invested a lot of time in
> trying to understand the timer code, so all I can really do
> is say "this does not look like the right thing" and ask you
> to come up with a different fix for it.

My current best guess is that this condition should simply be
"if (expired) {".

thanks
-- PMM