Tickless mode compare value check at counter overflow

[Pavel Pisa <pp...@pikron.com>]

Hello everybody,

I have noticed problematic pattern in the imxRT code
which is probably copied from STM32 code

  ./arch/arm/src/stm32f7/stm32_tickless.c:  while (tm <= stm32_get_counter())
  ./arch/arm/src/imxrt/imxrt_tickless.c:  while (tm <= imxrt_get_counter())

Criticality is minor, because overflow or 64-bit counter occurs
for microsecond resolution only once per  ~ 5e5 years and has
to coincide with miss of he event. But problem reported anyway,
because it similar problem appear in other places where cyclic
arithmetics is intended

The code

  https://github.com/apache/incubator-nuttx/blob/master/arch/arm/src/imxrt/imxrt_tickless.c#L539

  size_t offset = 1;
  uint64_t tm ....
  ....
  while (tm <= imxrt_get_counter())
    {
      tm = imxrt_get_counter() + offset++;
      putreg32(tm, g_tickless.base + IMXRT_GPT_OCR1_OFFSET + \
              (4 * (g_tickless.out_compare - 1)));
    }

When the cyclic nature of the tm requires cyclic (modulo arithmetics)
compare. But as is shown in the example below, the comparison
can lead incorrect decision that alarm has been set
successfully even when the time is and counter overflows.
The correct condition for cyclic compare is

  while ((int64_t)(tm - imxrt_get_counter()) <= 0)

Modulo, cyclic arithmetics and overflows are guaranteed only for unsigned
types by C language standard. That was correct. But for substraction
to find which way is the shorter path on the cycle between two points,
it is necessary to cast result to the signed type no larger than each
of the inputs. In this case uint64_t - uint64_t, so same or smaller
type with sign is int64_t .

The code has another problem, that alarm time can be easily far
after hardware counter overflow. But that should not be a problem
in such case, because compare to value truncated to 32-bits is triggered
earlier and if all other logic goes well, the next event would be shedulled
after complete cycle of the 32-bit part of the counter.

The analysis would get much more complex when calibration of the clock
and fine scaling is considered in the future. In such case, maximam
revet realizable time has to be considered and value has to be limited
to what is supported in the hardware. I can help to help read Linux kernel
part of this code... I have contributed to it years ago when I have extended
i.MX1 support to switch from tick based solution to high resolution
timing introduced by Thomas Gleixner at the time.

Ability to use clock/timer which period is not multiple of 1 usec
could be considered for NuttX in the long term perspective.

Best wishes,

Pavel

PS: I personally prefer to document cyclic nature of given comparison
    by macros ul_cyclic_gt(x,y) and #define ul_cyclic_ge(x,y)

      https://gitlab.com/pikron/sw-base/ulut/-/blob/master/ulut/ul_utdefs.h#L93

    but solution is a little fragile for case where some integer type can be
    defined on some target as using only 16 bits from 20 or 24-bit char
    on some DSP with memory organized by words. But even in DSP cases
    I have not found such problem yet. !6-bit type forced to align to
    4 bytes could be possible even on less obscure platform.

---------------------------------------------------------------------------------------------
#include <stdio.h>
#include <stdint.h>

int main(int argc, char *argv[])
{
  uint64_t counter;
  uint64_t tm;
  int cmp11, cmp12, cmp13, cmp21, cmp22, cmp23;

  counter = 1234;
  tm = counter + 10;
  counter = counter + 20;

  cmp11 = (tm <= counter);
  cmp12 = ((tm - counter) <= 0);
  cmp13 = ((int64_t)(tm - counter) <= 0);

  printf("compare 1 default %d, substract %d, cyclic %d\n", cmp11, cmp12, cmp13);

  counter = (uint64_t)-15;
  tm = counter + 10;
  counter = counter + 20;

  cmp21 = (tm <= counter);
  cmp22 = ((tm - counter) <= 0);
  cmp23 = ((int64_t)(tm - counter) <= 0);

  printf("compare 2 default %d, substract %d, cyclic %d\n", cmp21, cmp22, cmp23);

  return 0;
}
---------------------------------------------------------------------------------------------