Comment 39 for bug 595555

Hannes,
Thanks for your independent verification that Fix #1 (Comment #21 item
#7) works in some cases (Comment #25), and your report of unsolved
cases (Comment #26). Yesterday I started to notice the same unsolved
cases too, and it took me a while to track down the issue.

Here is an example of Issue #2, which is not covered by Fix #1.
    19:44:33.343 #5 SyncChangeCounter newval=60000, oldval=21114,
                                          pIdle<=-1, pIdle>=60000
    19:44:33.344 #4 SyncAlarmTriggerFired alarm id 0x00c00005, counter=60000
    19:44:33.344 #9 SyncComputeBracketVal counte=60000, test = 60000, psci<=0
    19:44:37.441 #7 IdleTimeQueryValue idle = 1, oldval=60000,
                                          pIdle<=-1, pIdle>= -1

sync.c uses "pIdleTimeValueLess", "pIdleTimeValueGreater" (denoted as
"pidle<", "pIdle>" hereafter) to track when alarms should be
triggered. They (pIdle<,pidle>) could be null pointer (denoted as
"-1" in my debug output), or some value (eg. 60000 msec). For +trans
(-trans) alarms, sync.c checks current idle count against pIdle>
(pIdle<), *if* the pointer is not null.

At "19:44:37.441 #7", when idle count goes from 60000 to 1 (idle = 1,
oldval= 60000; caused by user key activity),
    *** the -trans alarm should be triggered, but it is not. ***
Why? Because pIdle< is null (denoted by "-1"), which is wrong; it
should be 60000ms for the -trans alarm from g-ss.

Who sets pIdle<? "#9 SyncComputeBracketValues" does. Look at this
code snippet:
    else if (pTrigger->test_type == XSyncNegativeTransition ...) {
        if (XSyncValueGreaterThan(pCounter->value, pTrigger->test_value) &&
            XSyncValueGreaterThan(pTrigger->test_value, psci->bracket_less))
It also has boundary condition issue. Why this first test?
      pCounter->value > pTrigger->test_value # Test#1
            eg. 60001 > 60000
Because we want to check for -trans, which means the idle counter
(pCounter->value) goes from above test_value to below test_value
(eg. from 60001ms to 1ms, which crosses the 60000ms boundary). We
need this 2nd test
      pTrigger->test_value > psci->bracket_less # Test#2
to handle multiple -trans alarms, in which case we want the trigger
with largest test_value.

What happens to Test#1 in the case of this?
    19:44:33.344 #9 SyncComputeBracketVal counte=60000, test = 60000, psci<=0
It does not pass Test#1 (it should).
      pCounter->value > pTrigger->test_value # Test#1
                60000 > 60000? False.
So issue #2 has the same boundary issue: 60000 should be considered as
"above the threshold", as in Fix #1.

Fix#2:
Change Test#1 to
      pCounter->value >= pTrigger->test_value # Test#1bis

After both Fix #1, Fix #2, I have not seen this issue again. But then
again, the issue always pops up when you think you have it nailed :-).

I searched sync.c for "Negative" and look for similar pattern as in
Fix #1,#2. There are only two occurences, fixed by #1,#2 separately.
So please let me know if you see further occurences.

Fix #1 (Comment #21 item #7)
    SyncCheckTriggerNegativeTransition(SyncTrigger *pTrigger, CARD64 oldval)
    {
        return (pTrigger->pCounter == NULL ||
                (XSyncValueGreaterOrEqual(oldval, pTrigger->test_value) &&
                 XSyncValueLessThan(pTrigger->pCounter->value,
                                       pTrigger->test_value)));
    }

Fix #2
SyncComputeBracketValues(SyncCounter *pCounter)
  else if (pTrigger->test_type == XSyncNegativeTransition &&
             ct != XSyncCounterNeverIncreases)
  {
    /*if (XSyncValueGreaterThan (pCounter->value, pTrigger->test_value) && */
      if (XSyncValueGreaterOrEqual(pCounter->value, pTrigger->test_value) &&
          XSyncValueGreaterThan(pTrigger->test_value, psci->bracket_less))