[PATCH xserver 2/6] Remove SIGIO support.

[Keith Packard]

Peter Hutterer <peter.hutterer at who-t.net> writes:

>>      /* Call PIE here so we don't try to dereference a device that's
>>       * already been removed. */
>> -    OsBlockSignals();
>>      ProcessInputEvents();
>> +    input_lock();
>
> this is a behaviour change, was this intended? I'd rather not have this
> hidden in a giant patch that is otherwise mostly search and replace.

(I knew locking was the hard part...).

OsBlockSignals has a counter, so you can call it multiple
times. input_lock is (going to be) a regular mutex, so you can't (and
ProcessInputEvents definitely needs to hold the input lock). I moved
this to eliminate a deadlock, only to now discover that xfree86's
DeleteInputDeviceRequest grabs the input lock itself. And, kdrive's
evdev driver calls DeleteInputDeviceRequest from the event reading code (!).

We may uncover more deadlocks in the code; I think each one in isolation
should be pretty easy to fix. We could also implement a counter in the
mutex? That would require using thread local storage, which is kinda
ugly, but should at least allow the existing locking to not deadlock.

I'd rather work on annotating the code and data structures so that we
know when the lock should be held; right now it's somewhat haphazard,
with things like device property changes not taking the lock.

For instance, in this case we would mark the device as 'pending
deletion' and have the event queuing code discard incoming events:

        input_lock();
        dev->pending_deletion = TRUE;
        input_unlock();
        ProcessInputEvents();
        input_lock();
        DeleteInputDeviceRequest(dev);
        input_unlock();

>> @@ -1077,8 +1077,8 @@ TouchEndPhysicallyActiveTouches(DeviceIntPtr dev)
>>      InternalEvent *eventlist = InitEventList(GetMaximumEventsNum());
>>      int i;
>>  
>> -    OsBlockSignals();
>>      mieqProcessInputEvents();
>> +    input_lock();
>
> same here. How about making the first patch all the search/replace cases and
> let input_lock/unlock call OsBlockSignals/OsReleaseSignals. Then in the
> second patch you can change the places where it needs to be moved around,
> then in a third patch actually drop the signal handling.

For this case, mieqProcessDeviceEvent must not be called with the
input_lock held as it (eventually) may call SetCursorPosition, which
will grab input_lock to warp the cursor on the screen. Given what this
function does, it seems like it is equivalent to:

    input_lock();
    for (i = 0; i < dev->last.num_touches; i++) {
        DDXTouchPointInfoPtr ddxti = dev->last.touches + i;

        if (ddxti->active)
            QueueTouchEvents(dev, XI_TouchEnd, ddxti->ddx_id, 0, NULL);
    }
    input_unlock();
    ProcessInputEvents();

Similarly, ReleaseButtonsAndKeys should be changed to queue the relevant
events under the lock and then process them. That's a bit harder as that
function is looking at the logical state of the device, not the physical
state. It looks like that's just the tip of a large bug though -- a
frozen device that gets removed is going to have events in the
syncEvents list still pointing at it. That seems bad...

>> +xf86ReadInput(int fd, int ready, void *closure) {
>
> new line for {, no empty line before static void

Thanks.

>> diff --git a/hw/xfree86/common/xf86Helper.c b/hw/xfree86/common/xf86Helper.c
>> index c42e93e..b506338 100644
>> --- a/hw/xfree86/common/xf86Helper.c
>> +++ b/hw/xfree86/common/xf86Helper.c
>> @@ -1729,7 +1729,7 @@ xf86SetSilkenMouse(ScreenPtr pScreen)
>>       * yet.  Should handle this differently so that alternate async methods
>>       * work correctly with this too.
>>       */
>> -    pScrn->silkenMouse = useSM && xf86Info.useSIGIO && xf86SIGIOSupported();
>> +    pScrn->silkenMouse = useSM && FALSE;
>
> wait, what?

It's a temporary hack -- having removed SIGIO and not replaced it with
threading, all of the 'silken mouse' code can't actually work. Once we
add threads, then this looks sensible again.

>>  libbsd_la_SOURCES = \
>>  	$(srcdir)/../shared/posix_tty.c \
>> -	$(srcdir)/../shared/sigio.c \
>>  	$(srcdir)/../shared/vidmem.c \
>>  	bsd_VTsw.c \
>>  	bsd_init.c \
>
> [...]

I split the DRI1 breakage and SIGIO API removal out into separate
patches so we can discuss whether DRI1 needs to be supported independently
of whether input should be threaded.

>> diff --git a/xkb/xkbActions.c b/xkb/xkbActions.c
>> index aeb702c..effda7c 100644
>> --- a/xkb/xkbActions.c
>> +++ b/xkb/xkbActions.c
>> @@ -1526,7 +1526,7 @@ InjectPointerKeyEvents(DeviceIntPtr dev, int type, int button, int flags,
>>          return;
>>  
>>      events = InitEventList(GetMaximumEventsNum() + 1);
>> -    OsBlockSignals();
>> +    input_lock();
>>      pScreen = miPointerGetScreen(ptr);
>>      saveWait = miPointerSetWaitForUpdate(pScreen, FALSE);
>>      nevents = GetPointerEvents(events, ptr, type, button, flags, mask);
>> @@ -1534,10 +1534,10 @@ InjectPointerKeyEvents(DeviceIntPtr dev, int type, int button, int flags,
>>          UpdateFromMaster(&events[nevents], lastSlave, DEVCHANGE_POINTER_EVENT,
>>                           &nevents);
>>      miPointerSetWaitForUpdate(pScreen, saveWait);
>> -    OsReleaseSignals();
>>  
>>      for (i = 0; i < nevents; i++)
>>          mieqProcessDeviceEvent(ptr, &events[i], NULL);
>> +    input_unlock();
>
> again, a change in behaviour here

And this one is wrong even -- mieqProcessDeviceEvent needs to be called
with input_lock not held (as above).

> Cheers,
>    Peter

Thanks so much for reading through this. I think what I want to do is
provide a patch which continues to use OsBlockSIGIO/OsReleaseSIGIO but
fixes all of the recursive locking issues, then switches those (with a
sed script) to input_lock()/input_unlock(). Seem reasonable?

-- 
-keith
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 810 bytes
Desc: not available
URL: <http://lists.x.org/archives/xorg-devel/attachments/20151209/3c3de6a1/attachment.sig>