[PATCH v2] Allow driver to call DeleteInputDeviceRequest during UnInit

[Simon Thum]

Oldřich Jedlička wrote:
> 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.
So my workflow was: mutter around -> leave house -> get struck by how
the code is correct -> be offline.

I just failed to see the gist of the v2 patch. Sorry for the noise. Just
fix the naming as peter said, and of course it's:
Reviewed-by: Simon Thum <simon.thum at gmx.de>

As for the alternative solution you outlined below, I don't think it
makes sense to allow for device additions while closing down, especially
since config_fini() is called earlier in shutdown.

Cheers,

Simon

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