[PATCH xserver 2/6] Remove SIGIO support.

[Peter Hutterer]

On Wed, Dec 09, 2015 at 11:13:14AM -0800, Keith Packard wrote:
> 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. 

yes, but you can do that with the suggestion above. do a straightforward
search/replace for OsBlockSignals -> input_lock, then when you drop 
OsBlockSignals from input_lock you can move things where needed. plus you
get to see the places where the reordering was needed stand out.
[ok, now that I read the rest of the email I see you're suggesting to do
this further below]

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

add a comment please, if for no other reason than to reduce confusion of
those who look at the patches separately.

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

yeah, that sounds fine to me, thanks.

Cheers,
   Peter