[PATCH v2] Allow driver to call DeleteInputDeviceRequest during UnInit

[Oldřich Jedlička]

Hi Peter, Simon,

On Friday 15 of January 2010 at 10:40:02, Simon Thum wrote:
> Oldřich Jedlička wrote:
> > When the input driver (like xf86-input-wacom) removes it's devices
> > during a call to UnInit, the CloseDownDevices() cannot handle it. The
> > "next" variable can become a pointer to freed memory.
> > 
> > The patch introduces order-independent device freeing mechanism by
> > remembering the already freed device ids. The devices can reorder any
> > time during freeing. No device will be double-freed - if the removing
> > failed for any reason; some implementations of DeleteInputDeviceRequest
> > don't free the devices already.
> > 
> > Signed-off-by: Oldřich Jedlička <oldium.pro at seznam.cz>
> > ---
> > 
> >  dix/devices.c |   42 +++++++++++++++++++++++++++++++-----------
> >  1 files changed, 31 insertions(+), 11 deletions(-)
> > 
> > diff --git dix/devices.c dix/devices.c
> > index 245a95b..dce0f12 100644
> > --- dix/devices.c
> > +++ dix/devices.c
> > @@ -878,13 +878,41 @@ CloseDevice(DeviceIntPtr dev)
> > 
> >  }
> >  
> >  /**
> > 
> > + * Shut down all devices of one list and free all resources.
> > + */
> > +static
> > +void
> > +CloseListDevices(DeviceIntPtr *listHead)
> > +{
> > +    /* Used to mark devices that we tried to free */
> > +    Bool freedIds[MAXDEVICES] = { 0 };
> 
> This would only work if UnInit()s removals are somehow forced onto
> freedIds. In any case the freed array is a sort of context that's not
> easy to get right. I'm CC'ing Peter, maybe he's got a better idea.

I was looking for an invariant and found a unique device ID. I don't know if 
there is anything better for this purpose.

The closing loop works as long as those conditions are met:

1. There is no device addition during freeing (the device ID could be re-used 
in such case - so that the device would not be freed). I don't know if that is 
even possible.

2. No device gets enabled during the inputInfo.off_devices list closing - the 
device would be moved to inputInfo.devices list which will not be iterated 
again. The current implementation suffers from the same problem.

Alternative solution: I was thinking also about a different solution. Make the 
DeviceInputDeviceRequest return a value (TRUE/FALSE) to indicate successful 
device removal (TRUE=device is not in any of devices/off_devices list). The 
lists (both?) would be iterated until there is no TRUE value returned or both 
lists are empty.

What do you think? Fix-up problems mentioned in Peter's last email, or go on 
with the alternative solution?

Cheers,
Oldřich.

> > +    DeviceIntPtr dev;
> > +
> > +    if (listHead == NULL)
> > +	return;
> > +
> > +    dev = *listHead;
> > +    while (dev != NULL)
> > +    {
> > +        freedIds[dev->id] = TRUE;
> > +        DeleteInputDeviceRequest(dev);
> > +
> > +        dev = *listHead;
> > +        while (dev != NULL && freedIds[dev->id])
> > +        {
> > +            dev = dev->next;
> > +        }
> > +    }
> > +}
> > +
> > +/**
> > 
> >   * Shut down all devices, free all resources, etc.
> >   * Only useful if you're shutting down the server!
> >   */
> >  
> >  void
> >  CloseDownDevices(void)
> >  {
> > 
> > -    DeviceIntPtr dev, next;
> > +    DeviceIntPtr dev;
> > 
> >      /* Float all SDs before closing them. Note that at this point
> >      resources
> >      
> >       * (e.g. cursors) have been freed already, so we can't just call
> > 
> > @@ -897,16 +925,8 @@ CloseDownDevices(void)
> > 
> >              dev->u.master = NULL;
> >      
> >      }
> > 
> > -    for (dev = inputInfo.devices; dev; dev = next)
> > -    {
> > -	next = dev->next;
> > -        DeleteInputDeviceRequest(dev);
> > -    }
> > -    for (dev = inputInfo.off_devices; dev; dev = next)
> > -    {
> > -	next = dev->next;
> > -        DeleteInputDeviceRequest(dev);
> > -    }
> > +    CloseListDevices(&inputInfo.devices);
> > +    CloseListDevices(&inputInfo.off_devices);
> > 
> >      CloseDevice(inputInfo.pointer);
> >      CloseDevice(inputInfo.keyboard);
> 
> _______________________________________________
> xorg-devel mailing list
> xorg-devel at lists.x.org
> http://lists.x.org/mailman/listinfo/xorg-devel