--- Begin Message ---
- To: Debian Bug Tracking System <submit@bugs.debian.org>
- Subject: xserver-xorg-input-evtouch: race conditions in timers drop events
- From: Brett Wuth <wuth@castrov.cuug.ab.ca>
- Date: Thu, 15 Jan 2009 19:25:51 -0700
- Message-id: <20090116022551.15254.28395.reportbug@macbook.castrov.cuug.ab.ca>
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
--- End Message ---