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

[Peter Hutterer]

On Thu, Aug 01, 2013 at 02:37:27PM +0200, Takashi Iwai wrote:
> At Thu, 1 Aug 2013 14:12:58 +0200,
> Egbert Eich wrote:
> > 
> > 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?
> 
> It's the time of SLE11-GA, i.e. 2.6.27 kernel, found on a few HP
> laptops based on IronLake.  There is no problem since then.

[...]
> 
> IIRC, the device is still present in a stale state, so it's there
> uselessly until explicitly closed.  It was a kind of stale state while
> the kernel synaptics driver does reset after resume or so.

2.6.27 sounds about right for Fedora 8.fwiw, see evdev commit
9930477cbeb4acfd0, and its removal in 59056e656c6475816.
In that case the fd could be opened, but the first read would give ENODEV. I
suspect you're seeing the same problem here.

If you haven't seen this problem since we'll leave it out upstream though,
please ship this as distro patch if you still need it.

Cheers,
   Peter