[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Bug#511992: xserver-xorg-input-evtouch: race conditions in timers drop events



Package: xserver-xorg-input-evtouch
Version: 0.8.7-3
Severity: normal
Tags: patch


The attached patch fixes race conditions in the timer code which can
result in some taps going missing.  Also included is a fix to have a
timer go off immediately (if set to 0).  And the ability to turn a
timer off (if set to -1).

Quortech's open source packages are available at:
  ftp://ftp.quortech.com/eclipse/deb-packages/

The packages associated with xf86-input-evtouch_0.8.7-3quortech3.dsc
implement this patch.

-- System Information:
Debian Release: 4.0
  APT prefers stable
  APT policy: (500, 'stable')
Architecture: i386 (i686)
Shell:  /bin/sh linked to /bin/bash
Kernel: Linux 2.6.22-14-generic
Locale: LANG=en_CA.UTF-8, LC_CTYPE=en_CA.UTF-8 (charmap=UTF-8)
# Description: This patch fixes a data-loss bug and enhances the timers
#   in evtouch.
#
# Changes
#   o disable_timers() : Move to state change routine, as old timers
#     must be cancelled with *every* state change.
#   o libtouchEnhancedTimerSet : Time values can also be off (-1) or
#     immediate (0).
#   o libtouchTriggerSM : Fix bug where not protected from reentrant
#     call when timer goes off.  Potential for state changes to be lost
#     and output events not reflecting input events.
#
# Feel free to send comments, critics, suggestions to wuth@acm.org. To
# apply simply change into the toplevel directory of the source to be
# modified and enter: patch -p1 < <PATH_TO_PATCH>
# 
# All patches are available under the GNU GPL, I hope they might be
# useful for you (-:
# - Brett Wuth
#
Index: xf86-input-evtouch-0.8.7/evtouch.c
===================================================================
--- xf86-input-evtouch-0.8.7.orig/evtouch.c	2009-01-15 14:53:27.000000000 -0700
+++ xf86-input-evtouch-0.8.7/evtouch.c	2009-01-15 14:53:27.000000000 -0700
@@ -440,11 +440,12 @@
 
         if (priv->emulate3) {
                 if ( (ev->value==1) && (priv->emulate3_timer==NULL) )
-                        priv->emulate3_timer = TimerSet(priv->emulate3_timer,
-                                                        0,
-                                                        priv->emulate3_timeout,
-                                                        emulate3Timer,
-                                                        local);
+                        priv->emulate3_timer =
+			  libtouchEnhancedTimerSet( priv->emulate3_timer,
+						    0,
+						    priv->emulate3_timeout,
+						    emulate3Timer,
+						    local );
 
                 if ( (ev->value == 1) && (ev->code == BTN_LEFT) ) {
                         priv->touch_flags |= LB_STAT;
Index: xf86-input-evtouch-0.8.7/libtouch.c
===================================================================
--- xf86-input-evtouch-0.8.7.orig/libtouch.c	2009-01-15 14:53:27.000000000 -0700
+++ xf86-input-evtouch-0.8.7/libtouch.c	2009-01-15 14:53:27.000000000 -0700
@@ -328,6 +328,50 @@
 }
 
 
+/* A special value which means a timer should never trigger.
+ */
+#define TimerNever_M ((CARD32)-1)
+
+
+/* Set the timer but handle special values for millis (the time).
+ * if flags set to relative time, millis is interpretted as:
+ * 0 : trigger immediately (TimerSet ignores)
+ * TimerNever_M : never trigger
+ */
+OsTimerPtr
+libtouchEnhancedTimerSet( OsTimerPtr timer,
+			  int flags,
+			  CARD32 millis,
+			  OsTimerCallback func,
+			  pointer arg )
+{
+  int/*bool*/ relative = !(flags & TimerAbsolute);
+  int/*bool*/ immediate = relative && (millis == 0);
+
+  if (relative && (millis == TimerNever_M))
+    {
+      /* Still want to process other flags, but not to set the timer.
+       * 0 is a value that means that to TimerSet() */
+      millis = 0;
+      DBG(4, ErrorF( "LibTouch: timer set to NEVER\n" ));
+    }
+
+  /* If millis is 0, will not process timer, but will process other
+   * flags (TimerForceOld).
+   * See TimerSet definition in os/WaitFor.c
+   */
+  OsTimerPtr retVal = TimerSet( timer, flags, millis, func, arg );
+
+  if (immediate)
+    {
+      /* Since wasn't called by TimerSet, we call it ourselves */
+      (void) (*func)( NULL, GetTimeInMillis(), arg );
+    }
+
+  return (retVal);
+}
+
+
 static CARD32
 tap_timer_func(OsTimerPtr timer, CARD32 now, pointer _priv)
 {
@@ -364,7 +408,6 @@
         int bit_size = sizeof(priv->pressed_btn_stat) * 8;
         
         priv->touch_flags = 0;
-        disable_timers(priv);
 
         /* do an untouch for all pressed buttons */
         for (i = 0; i < bit_size; i++)
@@ -424,10 +467,10 @@
 
 static void enter_touched(LibTouchRecPtr priv)
 {
-        disable_timers(priv);
-        priv->longtouch_timer = TimerSet(priv->longtouch_timer, 0,
-                                         priv->longtouch_timeo,
-                                         longtouch_timer_func, priv);
+        priv->longtouch_timer =
+	  libtouchEnhancedTimerSet( priv->longtouch_timer, 0,
+				    priv->longtouch_timeo,
+				    longtouch_timer_func, priv );
 }
 
 
@@ -480,7 +523,6 @@
 
 static void enter_moving(LibTouchRecPtr priv)
 {
-        disable_timers(priv);
 }
 
 
@@ -495,7 +537,6 @@
 
 static void enter_longtouched(LibTouchRecPtr priv)
 {
-        disable_timers(priv);
         DBG(4, ErrorF("LibTouch: Issuing Button-press 1\n"));
         issue_btn_event(priv, S_LONGTOUCHED, priv->cur_x, priv->cur_y);
 }
@@ -534,10 +575,10 @@
 
 static void enter_maybetap(LibTouchRecPtr priv)
 {
-        disable_timers(priv);
-        priv->tap_timer = TimerSet(priv->tap_timer, 0,
-                                   priv->tap_timeo,
-                                   tap_timer_func, priv);
+        priv->tap_timer =
+	  libtouchEnhancedTimerSet( priv->tap_timer, 0,
+				    priv->tap_timeo,
+				    tap_timer_func, priv );
 }
 
 
@@ -610,10 +651,10 @@
 
 static void enter_oneandahalftap(LibTouchRecPtr priv)
 {
-        disable_timers(priv);
-        priv->longtouch_timer = TimerSet( priv->longtouch_timer, 0,
-                                          priv->longtouch_timeo,
-                                          longtouch_timer_func, priv);
+        priv->longtouch_timer =
+	  libtouchEnhancedTimerSet( priv->longtouch_timer, 0,
+				    priv->longtouch_timeo,
+				    longtouch_timer_func, priv );
 }
 
 
@@ -670,27 +711,108 @@
 }
 
 
+/* Process a state machine event.  May cause a change in state. */
 void libtouchTriggerSM(LibTouchRecPtr libtouch, LibTouchState_t pen)
 {
-        static int state_idx = S_UNTOUCHED;
-        int next_state_idx;
-
-        if (pen != PEN_UNKNOWN)
-                libtouch->pen = pen;
-
-	DBG(4, ErrorF("LibTouch: Triggering SM pen = 0x%02x\n", pen));
-
-        next_state_idx = state_ar[state_idx].handle_state(libtouch);
-        if( (next_state_idx != state_idx) && 
-            (state_ar[next_state_idx].enter_state != NULL) ) {
-                state_ar[next_state_idx].enter_state(libtouch);
-        }
-
-	DBG(4, ErrorF("LibTouch: Next State %d = %s\n", next_state_idx,
-		      state_str[next_state_idx]));
-
-        state_idx = next_state_idx;
-        libtouch->past = libtouch->now;
-        libtouch->xpos_changed = 0;
-        libtouch->ypos_changed = 0;
+  /* This function must be reentrant, because it can be called twice
+   * simultaneously: once as a result of touch screen inputs (e.g. SYN
+   * received by evtouch.c) and, anywhere in the middle of our call
+   * for that, a second time for a timeout (e.g. tap_timer_func()).
+   */
+
+  static int state_idx = S_UNTOUCHED;
+
+
+  /* Disable timer interrupts to prevent reentrant execution of the
+   * critical region of this code.
+   */
+  int sigstate = xf86BlockSIGIO();
+
+
+  /* Changes to pen state are likely not to come from timeout events,
+   * so probably this doesn't need to be part of the critical region.
+   * But its low cost (fast) to keep in critical region so do so
+   * defensively in case a future writer does something fancy within a
+   * timer.
+   */
+  if (pen != PEN_UNKNOWN)
+    libtouch->pen = pen;
+  DBG(4, ErrorF("LibTouch: Triggering SM pen = 0x%02x\n", pen));
+
+
+  /* Let the state handler process the input events, possibly
+   * selecting a new state.
+   *
+   * This is in the critical region to avoid a race condition:
+   * handle_state() is handling input that will result in *no* state
+   * change.  Timeout occurs and we would be called reentrantly
+   * resulting in a state change.  handle_state() returns and state is
+   * overwritten with old state.  As a result the state change from
+   * the timeout is lost.
+   */
+  int next_state_idx = state_ar[state_idx].handle_state(libtouch);
+  DBG(4, ErrorF("LibTouch: Next State %d = %s\n", next_state_idx,
+		state_str[next_state_idx]));
+
+
+  /* Mark the input events as processed.
+   *
+   * This is part of the critical region, so a state handler doesn't
+   * process input event twice.
+   */
+  libtouch->past = libtouch->now;
+  libtouch->xpos_changed = 0;
+  libtouch->ypos_changed = 0;
+
+  int state_change = (next_state_idx != state_idx);
+
+  if (state_change)
+    {
+      state_idx = next_state_idx;
+
+      /* A state change always means the timers associated with the
+       * previous state are no longer valid.  (They would indicate a
+       * state transition path originating in a no-longer-current
+       * state.) This is therefore part of the critical region.
+       *
+       * Timeouts which are pending because of xf86BlockSIGIO() will
+       * not occur when unblocked, because disable_timers() calls
+       * TimerFree().
+       */
+      disable_timers( libtouch );
+    }
+
+
+  /* We're out of the critical region.  Return timer interrupts
+   * to normal. */
+  xf86UnblockSIGIO( sigstate );
+
+
+  if (state_change && state_ar[next_state_idx].enter_state != NULL)
+    {
+      /* Set up for the new state.
+       *
+       * This is not part of the critical region because it does not affect
+       * this function's state.
+       *
+       * Any code (e.g. an enter_ function) called by enter_state()
+       * which sets up a timer, must be prepared for that timer to go
+       * off before the code completes the rest of its initialization.
+       * That is not our responsibility.  We /could/ handle it here by
+       * moving this call into the critical region, but it would
+       * require libtouchEnhancedTimerSet() to be reimplemented to not
+       * call us recursively on a 0 timeout.
+       */
+      state_ar[next_state_idx].enter_state(libtouch);
+
+      /* enter_state() may cause a recursive call to this function (if
+       * a timeout is set to 0).  Since this is outside the critical
+       * region, the behaviour is identical to what would happen if
+       * the call came from an actual time out.
+       *
+       * If a cascade of state transitions happen, each with 0
+       * millisecond timeouts, we could eventually blow the stack, but
+       * that would be a configuration error.
+       */
+    }
 }
Index: xf86-input-evtouch-0.8.7/libtouch.h
===================================================================
--- xf86-input-evtouch-0.8.7.orig/libtouch.h	2009-01-15 14:53:27.000000000 -0700
+++ xf86-input-evtouch-0.8.7/libtouch.h	2009-01-15 14:53:27.000000000 -0700
@@ -164,4 +164,16 @@
 void libtouchSetXPos(LibTouchRecPtr libtouch, int x);
 void libtouchSetYPos(LibTouchRecPtr libtouch, int y);
 
+/* Set the timer but handle special values for millis (the time).
+ * if flags set to relative time, millis is interpretted as:
+ * 0 : trigger immediately (TimerSet ignores)
+ * TimerNever_M : never trigger
+ */
+OsTimerPtr
+libtouchEnhancedTimerSet( OsTimerPtr timer,
+			  int flags,
+			  CARD32 millis,
+			  OsTimerCallback func,
+			  pointer arg );
+
 #endif

Reply to: