[PATCH V2 driver/input/synaptics] For serial devices try reconnecting if device is wedged
Takashi Iwai
tiwai at suse.de
Thu Aug 1 05:37:27 PDT 2013
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.
>
> >
> > > 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?
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.
Takashi
> 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.
>
More information about the xorg-devel
mailing list