Hi,

On 10/09/2016 11:52 PM, Mihail Konev wrote:
Hello.

On Wed,  5 Oct 2016 14:45:20 +0200, Hans de Goede wrote:
When a fd is removed dev->state gets set to device_state_removed,
if the fd then gets re-added before InputThreadDoWork() deals with
the removal, the InputThreadDevice struct gets reused, but its
state would stay device_state_removed, so it would still get removed
on the first InputThreadDoWork() run, resulting in a non functioning
input device.

This commit fixes this by (re-)setting dev->state to device_state_running
when a InputThreadDevice struct gets reused.

This fixes input devices sometimes no longer working after a vt-switch.

Signed-off-by: Hans de Goede <hdegoede at redhat.com>
---
 os/inputthread.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/os/inputthread.c b/os/inputthread.c
index 6aa0a9c..ab1559f 100644
--- a/os/inputthread.c
+++ b/os/inputthread.c
@@ -206,6 +206,8 @@ InputThreadRegisterDev(int fd,
     if (dev) {
         dev->readInputProc = readInputProc;
         dev->readInputArgs = readInputArgs;
+        /* Override possible unhandled state == device_state_removed */
+        dev->state = device_state_running;
     } else {
         dev = calloc(1, sizeof(InputThreadDevice));
         if (dev == NULL) {
--
2.9.3


On 6 Oct 2016, Hans de Goede wrote:
On 24-09-16 19:55, Mihail Konev wrote:
<..>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97880

Thank you for the patch and you're right that there is an issue here.

I've posted a much simpler fix yesterday, and that has been
favorably reviewed, so I think we're going to go with that fix
instead, <..>


Applied on top of 9e00cf0f75f286ec0e8137d91ba80ef7ba72ab2a , the patch does not
solve the bug #97880 for me.

Note: ./configure --disable-glamor is broken somewhere after the ade315386
(Require xproto 7.0.31), so be sure to --enable-glamor.

On 9 Oct 2016, Hans de Goede wrote:
So you've tried my patch, with your patch reverted and it
does not work for you? Strange as I was hitting the exact
same problem and it did work for me.


The difference in problem is, however, that in my case result is
consistent - keyboard either works or not all the time, not "sometimes".
Mouse is moving alright.

First, patch was applied on top of fc1c358b9 (1.19 RC1).
It did not work, so log1.diff was made, applied,
and log1 produced (both attached).
Note: log1 is (fully) commented inline.

Rebasing on top of 97a8353ec (Fix id in error) did not help.

At this point (log1), nol_e.diff gives:
- nol_e.log1
- bug being present

Applying nol_d.diff to log1 gives:
- nol_d.log1
- bug being gone

Manually applied log1.diff to master 97a8353ec (gave log1m.diff).

At this point (log1m) nol_e.diff gives:
- nol_e.log1m
- bug being present
Mistake in bugzilla bug description:
this shows that not locking EnableDevice is not a sufficient workaround.

Applying nol_d.diff to log1m gives:
- nol_d.log1m
- bug being gone
This is what is right in bugzilla description:
not locking DisableDevice is a sufficient workaround.

As I see it,

The patch assumes no device state to be maintained except in the input thread 
dev
list.

No, the patch assumes that the only device state which gets delayed in 
processing
because InputThreadDoWork() not running in time is the input thread dev list and
this is correct, all InputThreadDoWork() does on device_state_removed is remove
the fd from the pollfd list, and del the node from the inputhread dev list.

My patch simply makes it correctly re-use the dev node on the list, instead
of removing it even though the device has been re-added.

Hmm, maybe the problem in your case is that the fd is the same before / after
device remove / (re-)add but has been closed, so it needs to be removed from
the poll list and re-added.

I've attached another patch (on top of my previous one) for you to try,
I've good hope that this will fix your issue.

Regards,

Hans

>From f9288ccbe8cf8184fcceec8e599213ff53db219c Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdego...@redhat.com>
Date: Tue, 11 Oct 2016 11:20:49 +0200
Subject: [PATCH xserver] inputthread: Re-add fd to the inputThreadInfo->fds
 pollfd set when re-added

If the following call sequence happens:
1) InputThreadUnregisterDev(fd)
2) close(fd)
3) fd = open(...) /* returns same fd as just closed */
4) InputThreadRegisterDev(fd, ...)

Without InputThreadDoWork(); running in the mean time, then we would
keep the closed fd in the inputThreadInfo->fds pollfd set, rather then
removing it and adding the new one, causing some devices to not work
after a vt-switch when using xf86-input-evdev.

Signed-off-by: Hans de Goede <hdego...@redhat.com>
---
 os/inputthread.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/os/inputthread.c b/os/inputthread.c
index ab1559f..c20c21c 100644
--- a/os/inputthread.c
+++ b/os/inputthread.c
@@ -49,6 +49,7 @@ Bool InputThreadEnable = TRUE;
 
 typedef enum _InputDeviceState {
     device_state_added,
+    device_state_re_added,
     device_state_running,
     device_state_removed
 } InputDeviceState;
@@ -206,8 +207,8 @@ InputThreadRegisterDev(int fd,
     if (dev) {
         dev->readInputProc = readInputProc;
         dev->readInputArgs = readInputArgs;
-        /* Override possible unhandled state == device_state_removed */
-        dev->state = device_state_running;
+        if (dev->state == device_state_removed)
+            dev->state = device_state_re_added;
     } else {
         dev = calloc(1, sizeof(InputThreadDevice));
         if (dev == NULL) {
@@ -344,6 +345,16 @@ InputThreadDoWork(void *arg)
                     ospoll_listen(inputThreadInfo->fds, dev->fd, X_NOTIFY_READ);
                     dev->state = device_state_running;
                     break;
+                case device_state_re_added:
+                    /* Device might use a new fd with the same number */
+                    ospoll_remove(inputThreadInfo->fds, dev->fd);
+                    ospoll_add(inputThreadInfo->fds, dev->fd,
+                               ospoll_trigger_level,
+                               InputReady,
+                               dev);
+                    ospoll_listen(inputThreadInfo->fds, dev->fd, X_NOTIFY_READ);
+                    dev->state = device_state_running;
+                    break;
                 case device_state_running:
                     break;
                 case device_state_removed:
-- 
2.9.3


_______________________________________________
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Reply via email to