|Summary:||Switching from VT console disables keyboard|
|Product:||xorg||Reporter:||Mihail Konev <k.mvc>|
|Component:||Server/Input/Core||Assignee:||Xorg Project Team <xorg-team>|
|Status:||RESOLVED FIXED||QA Contact:||Xorg Project Team <xorg-team>|
|i915 platform:||i915 features:|
Description Mihail Konev 2016-09-20 21:00:17 UTC
Created attachment 126676 [details] [review] proposed patch Under Xserver 1.19 RC1, the keyboard does not respond to key presses after returning from virtual console. Bisected to 52d6a1e832a5e62289dd4f32824ae16a78dfd7e8. Not holding input lock on either Release or Enable suffices to work this around. Logging the input thread showed that one or more InputThreadReleaseDev-s aren't served before the main thread acquires the input lock for all the VT switch time. Waiting for input thread to process all the releases solves the problem.
Comment 1 Mihail Konev 2016-09-21 11:54:50 UTC
Created attachment 126702 [details] [review] proposed patch v2 Short-circuit the input loop conditional.
Comment 2 Mihail Konev 2016-09-23 15:53:05 UTC
Created attachment 126746 [details] [review] proposed patch v3 After running (in xorg/proto dir) evince kbproto/specs/*.svg Attempting to switch VT causes freeze. The only visible change is mouse pointer disappearing. This patch fixes the deadlock.
Comment 3 Mihail Konev 2016-09-24 10:18:20 UTC
Created attachment 126759 [details] [review] proposed patch v4 Fix stalls before VT switches.
Comment 4 Mihail Konev 2016-10-08 21:22:17 UTC
Created attachment 127144 [details] log 1 Log fragment for Xserver with the bug, with comments. X was run as: timeout 20 xinit ...
Comment 5 Mihail Konev 2016-10-08 21:28:01 UTC
Created attachment 127145 [details] [review] log 1 patch Patch used to produce the log 1. Needs patch  being applied first. Note about commented lines: uncommenting them results in bug disappearing (as (second) LogMessageVerb() takes time, the window for input thread increases). See the next comment for explanation.  https://patchwork.freedesktop.org/patch/113763/
Comment 6 Mihail Konev 2016-10-08 21:32:39 UTC
Looks like the lack of logs have misled upstream into patch , which does address the problem, but only partially. In short, the patch does not account for non-input-thread device state. Below is the detailed explanation. X server keeps a list of input devices (dev list). The sequence of events leading to the bug, as seen in the log 1, is as follows: - user switches from X server VT - main thread enters xf86DisableDevicesForVTSwitch - main thread sets all dev->state in dev list to device_state_removed. For each dev, it (in the InputThreadUnregisterDevice): * acquires input_lock * sets dev->state to device_state_removed * sets inputThreadInfo->changed to true * releases input_lock - as main thread traverses the list, input thread runs its main loop. The loop is as follows: * see if inputThreadInfo->changed is true. If it is: - acquire input_lock - process any dev->state-s which are not device_state_running - release input_lock * wait for input events * signal main thread about events (like with inputThreadInfo->changed) - typically, after each `dev->state = device_state_removed` by main thread the input thread acquires the lock, and processes the dev->state. (Somehow it does not gets stuck in waiting for input events; this must be due to removes being input events also). - but for the last device, the main thread releases the input_lock, then exits the loop in xf86Disable, and then does input_lock again, for all the time of VT switch. This leaves no chance for input thread to process the last device before the switch. - upon return from VT switch, main thread attaches devices (in the InputThreadRegisterDevice). For the unprocessed device, it leaves dev->state as is, i.e. device_state_removed. All the patch  does is to make main thread set dev->state to device_state_running. The patch, therefore, has no effect on input thread's behaviour upon returning from VT. The commit that caused the regression added input_lock to the main thread traversal of the dev list. Before, the main thread would, for each dev: * set dev->state to device_state_removed * set inputThreadInfo->chaged to true As soon as dev->state changed, the input thread would acquire the lock, and process the dev->state. For the last device, the input thread would process it, then release the lock. The main thread would then acquire the lock for all the VT switch. All releases are processed; all devices would be attached (by input thread) properly upon returning from VT switch. This is what happens when InputThreadUnregisterDev is un-patched from input_lock (comment 1). In case the input thread would not do that in time (which never happened), it still likely had a chance to process (any number of) `dev->state == device_state_removed` before the main thread would reach to them. How? Because of the order devices got attached/detached: if the 6th device was not processed, 5 preceding ones would first be added, and only then the presence of 6th examined (input thread would remove the dev by that time, forcing main into adding the dev to the list). The order of devices does not change, as it is determined by values of their file descriptors (corresponding to /dev/input/eventN), which do not get closed/re-opened on VT leave/enter, therefore staying the same. This is what happens when InputThreadRegisterDev is un-patched from input_lock (comment 1). The non-input-thread device state (be it OS, X server's but not a dev->state one, or their combination) change could only be initiated by input thread, through processing the dev->state-s. That is why thread synchronization is necessary - to trigger proper detach. Without a detach, there would be no attach (by the input thread processing dev->state-s). The device would not work therefore. For the  to work, it must be that "non-input-thread" device state is not changed during VT switch, so that there is no need to change it upon returning. This means that mouse would still be moving around and key events would still be sent to windows. This did not happen with any InputThread modifications.  https://patchwork.freedesktop.org/patch/113763/
Comment 7 Mihail Konev 2016-10-08 23:38:56 UTC
Typo in comment 6: > - upon return from VT switch, main thread attaches devices > (in the InputThreadRegisterDevice). > For the unprocessed device, it leaves dev->state as is, > i.e. device_state_removed. > All the patch  does is to make main thread set dev->state > to device_state_running. > The patch, therefore, has no effect on input thread's behaviour > upon returning from VT.  does affect the input thread, but only subtly: - previously the sixth device was fully removed, but not re-added at all * both input thread and OS (or X, or their combination) thought it was disabled) - now it just stays "hanged" in half-removed state * input thread thinks it should be turned on * OS (or ...) disabled it before VT switch, but not re-enabled afterwards; input thread would not receive any events
Comment 8 Mihail Konev 2016-10-09 03:11:45 UTC
> This means that mouse would still be moving around > and key events would still be sent to windows. Or rather, all the while-on-another-VT events would be delivered at burst, as soon as main thread releases the input_lock.
Comment 9 Mihail Konev 2016-10-11 11:58:51 UTC
Proper working fix is found at: https://lists.x.org/archives/xorg-devel/2016-October/051593.html