On Thu, Mar 10, 2011 at 05:51:34PM -0800, Ping Cheng wrote:
> On Wed, Mar 2, 2011 at 10:06 PM, Peter Hutterer
> <peter.hutte...@who-t.net>wrote:
>
> > There is a small time window where a device may try to send an event even
> > though it is not fully setup to send events yet, causing a server crash.
> >
> > This window opens when the tool is added to the list of devices in
> > wcmParseOptions() and closes with the server calling xf86ActivateDevice().
> > If an event for a dependent device is processed during that time, the tool
> > will be available but the device pointer is still invalid.
> >
> > Crash can be reproduced by putting a breakpoint after wcmParseOptions() for
> > the eraser, then generating events with the eraser. These will cause the
> > tool to dereference tool->device->dev, which is uninitialized.
> >
> > Work around this with a simple "enabled" flag that is set whenever the tool
> > is actually enabled.
> >
>
> What's the plan for this patchset?
have you seen the crash with this patch applied?
I'm still waiting for the testing feedback. I've tested it with gdb, forcing
some race conditions and AFAICT this bug is fixed with this patch. The X
server patch that left a small window open that could cause weird events
(though unlikely to crash) is in the server.
> Is it possible to make a common->enabled
> so we can disable the Read in xf86Wacom.c or at least stop process the data
> as what you did in this patch for all tools on the same port while
> initializing the tools?
With this patch, we don't need the common->enabled hack. If twools are on
two different kernel devices, the bug can't hit us anyway, can it?
Cheers,
Peter
> I am willing to ack the patchset. But I hope we have considered all existing
> issues that we could cover. priv->enabled feels weak here.
>
> Another thing that I didn't want to bring up was: for those devices that
> support both touch and pen, we probably will have a bigger challenge. Pen
> and touch are on different logical port. But they are still the same
> physical device. To stop tools on one port accessing the tablet while the
> tools on the other port initializing is a job beyond common->disabled. We
> will need to link the two logical ports together by product ID plus
> something else, in case there are more than one identical tablets connected
> to the system.
>
> Hopefully we can come up with a solution when we need to worry about it
> later.
>
>
> > Signed-off-by: Peter Hutterer <peter.hutte...@who-t.net>
> > ---
> > src/wcmCommon.c | 9 ++++-----
> > src/xf86Wacom.c | 19 +++++++++++++++++++
> > src/xf86WacomDefs.h | 1 +
> > 3 files changed, 24 insertions(+), 5 deletions(-)
> >
> > diff --git a/src/wcmCommon.c b/src/wcmCommon.c
> > index 591a20e..2491e4b 100644
> > --- a/src/wcmCommon.c
> > +++ b/src/wcmCommon.c
> > @@ -1163,17 +1163,16 @@ static void commonDispatchDevice(WacomCommonPtr
> > common, unsigned int channel,
> > return;
> > }
> >
> > - pInfo = tool->device;
> > - DBG(11, common, "tool id=%d for %s\n", ds->device_type,
> > pInfo->name);
> > -
> > /* Tool on the tablet when driver starts. This sometime causes
> > * access errors to the device */
> > - if (!pInfo || !pInfo->dev || !pInfo->dev->enabled)
> > - {
> > + if (!tool->enabled) {
> > xf86Msg(X_ERROR, "wcmEvent: tool not initialized yet.
> > Skipping event. \n");
> > return;
> > }
> >
> > + pInfo = tool->device;
> > + DBG(11, common, "tool id=%d for %s\n", ds->device_type,
> > pInfo->name);
> > +
> > filtered = pChannel->valid.state;
> >
> > /* Device transformations come first */
> > diff --git a/src/xf86Wacom.c b/src/xf86Wacom.c
> > index 0962e1d..43d64a4 100644
> > --- a/src/xf86Wacom.c
> > +++ b/src/xf86Wacom.c
> > @@ -755,6 +755,23 @@ static void wcmDevClose(InputInfoPtr pInfo)
> > }
> > }
> >
> > +static void wcmEnableDisableTool(DeviceIntPtr dev, Bool enable)
> > +{
> > + InputInfoPtr pInfo = dev->public.devicePrivate;
> > + WacomDevicePtr priv = pInfo->private;
> > + WacomToolPtr tool = priv->tool;
> > +
> > + tool->enabled = enable;
> > +}
> > +
> > +static void wcmEnableTool(DeviceIntPtr dev)
> > +{
> > + wcmEnableDisableTool(dev, TRUE);
> > +}
> > +static void wcmDisableTool(DeviceIntPtr dev)
> > +{
> > + wcmEnableDisableTool(dev, FALSE);
> > +}
> >
> >
> > /*****************************************************************************
> > * wcmDevProc --
> > * Handle the initialization, etc. of a wacom tablet. Called by the
> > server
> > @@ -791,12 +808,14 @@ static int wcmDevProc(DeviceIntPtr pWcm, int what)
> > case DEVICE_ON:
> > if (!wcmDevOpen(pWcm))
> > goto out;
> > + wcmEnableTool(pWcm);
> > xf86AddEnabledDevice(pInfo);
> > pWcm->public.on = TRUE;
> > break;
> >
> > case DEVICE_OFF:
> > case DEVICE_CLOSE:
> > + wcmDisableTool(pWcm);
> > if (pInfo->fd >= 0)
> > {
> > xf86RemoveEnabledDevice(pInfo);
> > diff --git a/src/xf86WacomDefs.h b/src/xf86WacomDefs.h
> > index db83ddb..2a6e4ff 100644
> > --- a/src/xf86WacomDefs.h
> > +++ b/src/xf86WacomDefs.h
> > @@ -492,6 +492,7 @@ struct _WacomTool
> >
> > int typeid; /* Tool type */
> > int serial; /* Serial id, 0 == no serial id */
> > + Bool enabled;
> >
> > InputInfoPtr device; /* The InputDevice connected to this tool */
> > };
> > --
> > 1.7.4
------------------------------------------------------------------------------
Colocation vs. Managed Hosting
A question and answer guide to determining the best fit
for your organization - today and in the future.
http://p.sf.net/sfu/internap-sfd2d
_______________________________________________
Linuxwacom-devel mailing list
Linuxwacom-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
