[PATCH V2 driver/input/synaptics] For serial devices try reconnecting if device is wedged

[Peter Hutterer]

On Tue, Jul 30, 2013 at 05:27:06AM +0200, Egbert Eich wrote:
> From: Takashi Iwai <tiwai at suse.de>
> 
> Under some circumstances the synaptics device may be wedged when
> coming back from a sleep state. Add some magic which tries to
> detect this and reconnect.
> 
> Signed-off-by: Takashi Iwai <tiwai at suse.de>
> Signed-off-by: Egbert Eich <eich at freedesktop.org>
> ---
> v2: Avoid '_' at beginning of function name.
> 
>  src/eventcomm.c    |  3 ++-
>  src/synaptics.c    | 56 +++++++++++++++++++++++++++++++++++++++++++++++-------
>  src/synapticsstr.h |  3 +++
>  3 files changed, 54 insertions(+), 8 deletions(-)
> 
> diff --git a/src/eventcomm.c b/src/eventcomm.c
> index 258a538..86b74cb 100644
> --- a/src/eventcomm.c
> +++ b/src/eventcomm.c
> @@ -638,7 +638,8 @@ EventReadHwState(InputInfoPtr pInfo,
>      }
>  
>      while (SynapticsReadEvent(pInfo, &ev)) {
> -        switch (ev.type) {
> +	priv->comm_read++;
> +	switch (ev.type) {

indentation, goes for the rest of the patch.

comm_read is increased on every event so there's the (unlikely) chance of an
overrun. why not just set this to 1?

>          case EV_SYN:
>              switch (ev.code) {
>              case SYN_REPORT:
> diff --git a/src/synaptics.c b/src/synaptics.c
> index f0a8269..007d0bb 100644
> --- a/src/synaptics.c
> +++ b/src/synaptics.c
> @@ -920,18 +920,30 @@ DeviceControl(DeviceIntPtr dev, int mode)
>  }
>  
>  static int
> -DeviceOn(DeviceIntPtr dev)
> +doDeviceOn(InputInfoPtr pInfo)
>  {
> -    InputInfoPtr pInfo = dev->public.devicePrivate;
>      SynapticsPrivate *priv = (SynapticsPrivate *) (pInfo->private);
> +    int n;
>  
>      DBG(3, "Synaptics DeviceOn called\n");
>  
> -    pInfo->fd = xf86OpenSerial(pInfo->options);
> +    for (n = priv->retries; n >= 0; n--) {

I really want a comment here - I read over the >= 0 and was wondering how
we'd even enter the loop.

> +	pInfo->fd = xf86OpenSerial(pInfo->options);
> +	if (pInfo->fd != -1)
> +	    break;
> +	if (n)
> +	    xf86Msg(X_WARNING, "%s: cannot open input device - "
> +		    "retrying %d more times\n", pInfo->name, n);
> +    }
>      if (pInfo->fd == -1) {
>          xf86IDrvMsg(pInfo, X_WARNING, "cannot open input device\n");
>          return !Success;
>      }
> +    /* This has succeeded once, so chances are the hardware *really* is present
> +     * - this is not a hotplug device after all.
> +     * Without trying really hard on some machines with some kernels the device
> +     * won't be found after S3/S4 again. */
> +    priv->retries = 4;
  
I'd love to have some details here above "some machines with some kernels".
I remember this being a problem around the Fedora 8 cycle (or so) but it's
hasn't been seen for ages here (evdev doesnt have the code anymore).

what's the errno when this happens?

>      if (priv->proto_ops->DeviceOnHook &&
>          !priv->proto_ops->DeviceOnHook(pInfo, &priv->synpara))
> @@ -956,11 +968,21 @@ DeviceOn(DeviceIntPtr dev)
>      }
>  
>      xf86AddEnabledDevice(pInfo);
> -    dev->public.on = TRUE;
>  
>      return Success;
>  }
>  
> +static int
> +DeviceOn(DeviceIntPtr dev)
> +{
> +    int ret = doDeviceOn(dev->public.devicePrivate);
> +
> +    if (ret == Success)
> +	dev->public.on = TRUE;
> +
> +    return ret;
> +}
> +
>  static void
>  SynapticsReset(SynapticsPrivate * priv)
>  {
> @@ -995,9 +1017,8 @@ SynapticsReset(SynapticsPrivate * priv)
>  }
>  
>  static int
> -DeviceOff(DeviceIntPtr dev)
> +_DeviceOff(InputInfoPtr pInfo)

this one would need a renaming too. For both DeviceOn and DeviceOff I'm
struggling to see a reason to even split this up. You're just moving
dev->public.on outside the function. DeviceOn won't set this except on
success. For DeviceOff it doesn't really matter what we do, iirc the server
doesn't handle not being able to turn a device off too well anyway.

>  {
> -    InputInfoPtr pInfo = dev->public.devicePrivate;
>      SynapticsPrivate *priv = (SynapticsPrivate *) (pInfo->private);
>      Bool rc = Success;
>  
> @@ -1018,11 +1039,18 @@ DeviceOff(DeviceIntPtr dev)
>          xf86CloseSerial(pInfo->fd);
>          pInfo->fd = -1;
>      }
> -    dev->public.on = FALSE;
>      return rc;
>  }
>  
>  static int
> +DeviceOff(DeviceIntPtr dev)
> +{
> +    int ret = _DeviceOff(dev->public.devicePrivate);
> +    dev->public.on = FALSE;
> +    return ret;
> +}
> +
> +static int
>  DeviceClose(DeviceIntPtr dev)
>  {
>      Bool RetValue;
> @@ -1469,6 +1497,7 @@ ReadInput(InputInfoPtr pInfo)
>  
>      SynapticsResetTouchHwState(hw, FALSE);
>  
> +    priv->comm_read = 0;
>      while (SynapticsGetHwState(pInfo, priv, hw)) {
>          /* Semi-mt device touch slots do not track touches. When there is a
>           * change in the number of touches, we must disregard the temporary
> @@ -1487,6 +1516,19 @@ ReadInput(InputInfoPtr pInfo)
>          newDelay = TRUE;
>      }
>  
> +    if (!priv->comm_read) {
> +        /* strange callback, check the device and reconnect if needed */

only the eventcomm backend sets priv->comm_read, so this breaks synaptics on
BSD/Solaris/...

> +	if (!priv->reconnecting) {
> +	    priv->reconnecting = 1;
> +	    xf86Msg(X_WARNING, "%s: reconnecting device...\n", pInfo->name);
> +	    _DeviceOff(pInfo);
> +	    usleep(100*1000);
> +	    doDeviceOn(pInfo);
> +	    xf86Msg(X_WARNING, "%s: reconnection done\n", pInfo->name);

shouldn't we check for the return value here and either disable the device
completely here or just continue as normal?

aside from that: you can't call DeviceOn/Off from ReadInput, neither is
signal-safe. So we really need a different approach here, sorry.

Cheers,
   Peter

> +	} else
> +	    priv->reconnecting = 0;
> +    }
> +


>      if (newDelay) {
>          priv->timer_time = GetTimeInMillis();
>          priv->timer = TimerSet(priv->timer, 0, delay, timerFunc, pInfo);
> diff --git a/src/synapticsstr.h b/src/synapticsstr.h
> index 428befa..98d6552 100644
> --- a/src/synapticsstr.h
> +++ b/src/synapticsstr.h
> @@ -204,6 +204,9 @@ struct _SynapticsPrivateRec {
>      OsTimerPtr timer;           /* for tap processing, etc */
>  
>      struct CommData comm;
> +    int comm_read;		/* for reconnection check */
> +    int reconnecting;		/* for reconnection check */
> +    int retries;
>  
>      struct SynapticsHwState *local_hw_state;    /* used in place of local hw state variables */
>  
> -- 
> 1.8.1.4
> 
> _______________________________________________
> xorg-devel at lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel
>