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

[Egbert Eich]

On Thu, Aug 01, 2013 at 04:38:38PM +1000, Peter Hutterer wrote:
> 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?

Yeah, for the purpose we use it for it shouldn't make a difference.

> 
> >          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.

  I don't like comments for this purpose. So let's rewrite the code to
  make this more obvious.
  
> 
> > +	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?

I don't know the details here as I never looked into this bug.
Takashi - do you remember this? Do we still need to bother?

> 
> >      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

Right. Since the DeviceIntPtr is also in the InputInfoRec.

> 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.
> 

ReadInput() cannot return an error state, it's expected that it will
always succeed. So anything else but trying to kick the device inside 
the read routine seems to be a nightmare.
On the other hand I'm not sure what was going on here and why we get here 
in the first place. If the device is wedged that it cannot be read it 
should not be able to signal that there's something to read either ... 

Takashi, do you remember?

AFAIK SIGIOs are blocked in the read context anyway so couldn't we simply 
enclose the doDeviceOn/Off() functions in their wrappers (ie DeviceOn/Off()) 
with xf86BlockSIGIO() ... xf86UnblockSIGIO() to make those signal-safe?

Cheers,
	Egbert.