[PATCH] Correct a race condition between the e500 core decrementer wrapping and the tick interrupt being handled

[Sebastian Huber]

On 15/12/14 00:45, Nick Withers wrote:
> On Fri, 2014-12-12 at 08:02 +0100, Sebastian Huber wrote:
>> >On 12/12/14 03:18, Nick Withers wrote:
>>> > >On Wed, 2014-12-10 at 08:42 +0100, Sebastian Huber wrote:
>>>> > >>On 10/12/14 02:53, Nick Withers wrote:
>>>>> > >>>On Wed, 2014-12-03 at 08:09 +0100, Sebastian Huber wrote:
>>>>>> > >>>>Hello Nick,
>>>>>> > >>>>
>>>>>> > >>>>to disable TCR[ARE] is not the right fix.  You have to check the
>>>>>> > >>>>TSR[DIS] register in the nanoseconds extension.  See for example
>>>>>> > >>>>lpc-clock-config.c for an example.
>>>>> > >>>Hi Sebastian,
>>>>> > >>>
>>>>> > >>>Thanks for your help, 'fraid I need to lean on you a little more...
>>>>> > >>>
>>>>> > >>>I don't understand how the proposed solution (or that in
>>>>> > >>>lpc-clock-config.c) can work properly in the case where the nanoseconds
>>>>> > >>>extension could be queried multiple times with an interrupt lock held
>>>>> > >>>(which I believe disables interrupts), preventing the tick from being
>>>>> > >>>incremented, as I believe happens in spnsext01.
>>>>> > >>>
>>>>> > >>>lpc-clock-config.c's lpc_clock_nanoseconds_since_last_tick(), if I'm
>>>>> > >>>understanding it correctly, says "Ah, there's an interrupt pending, so
>>>>> > >>>we need to add another tick period to our current timer value". Cool
>>>>> > >>>bananas.
>>>> > >>Yes, this is exactly how it works.
>>>> > >>
>>>>> > >>>But what if it's called again with the same interrupt still
>>>>> > >>>pending? The timer counter may have wrapped again and we could return an
>>>>> > >>>earlier time.
>>>>> > >>>
>>>>> > >>>...Couldn't we? What am I missing?
>>>>> > >>>
>>>>> > >>>Cheers:-)
>>>> > >>If you disable interrupts for more than one clock tick interval, then
>>>> > >>you have a bigger problem than a non-monotonic uptime.
>>> > >Yeah, that's probably fair...
>>> > >
>>>> > >>If you disable the auto-reload, then the decrementer stops, so you have
>>>> > >>a time freeze.
>>> > >I suppose that didn't worry me as much as non-monotonic time. Probably
>>> > >because I'm a selfish so-and-so and not personally writing a real-time
>>> > >application with strict periodic tasking requirements (and don't
>>> > >particularly care about the uptime drifting):-P
>>> > >
>>> > >A new patch is attached, thanks for your help and guidance!
>>> > >
>>> > >It's a little more convoluted than might be necessary because I wasn't
>>> > >confident that every relevant BSP's Clock_Decrementer_value (some of
>>> > >which are set at run-time) was small enough not to overflow the uint32_t
>>> > >tmp variable in intermediate calculations if I just straight-up doubled
>>> > >the potential range of values it might need to hold (with the extra
>>> > >clock tick period that might now be returned). It potentially adds
>>> > >another run-time division this way, though. I could just use a
>>> > >uint64_t...?
>> >
>> >Other nanoseconds extensions use a 64-bit multiplication to solve this
>> >issue.  It is quite fast in contrast to a 64-bit division which is a no-go.
> Without changing the maths and with the "simple" method, wouldn't it
> *have*  to be a 64-bit division, since the division itself is the only
> thing that reduces range?
>

OK, there is a 64-bit division, but it is a divide by 2**32, so it is a 
32 bit right shift actually.

-- 
Sebastian Huber, embedded brains GmbH

Address : Dornierstr. 4, D-82178 Puchheim, Germany
Phone   : +49 89 189 47 41-16
Fax     : +49 89 189 47 41-09
E-Mail  : sebastian.huber at embedded-brains.de
PGP     : Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.