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