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

[Nick Withers]

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?

> > P.S.: For my education (because noone has anything better to do,
> > right?)... Is there a more general solution? Maybe servicing the tick
> > increment in the nanoseconds call if an interrupt's been generated (is
> > servicing an interrupt out-of-band "usually" legitimate across
> > architectures / PICs?) but deferring e.g. scheduling 'til interrupts're
> > re-enabled?
> >
> > If not, I guess it should be guaranteeable / assumable that interrupts
> > should never be blocked for longer than x cycles (when users can't
> > disable them themselves)?
> >
> > Would the FreeBSD clock code you mentioned earlier handle it any better?
> 
> Yes, there is a better solution: the FreeBSD timecounter.  We currently 
> try to get some budget to port this to RTEMS and update all BSPs 
> providing the nanoseconds extension.

Righto, thanks :-)

> > P.P.S.: Clock_driver_nanoseconds_since_last_tick() also seems broken in
> > the non-Book-E case if the decrementer value is (one's complement)
> > negative
> 
> The decrementer based clock tick is broken in general since it doesn't 
> respect the priorities of the interrupt controller.
-- 
Nick Withers

Embedded Systems Programmer
Department of Nuclear Physics, Research School of Physics and Engineering
The Australian National University (CRICOS: 00120C)