Peter Hutterer <peter.hutte...@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
signature.asc
Description: PGP signature
_______________________________________________ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel