On Wed, Oct 12, 2016 at 04:55:08PM +0200, Hans de Goede wrote: > 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. > > BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=97880 > Reported-and-tested-by: Mihail Konev <[email protected]> > Signed-off-by: Hans de Goede <[email protected]> > --- > 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;
I'm wondering, especially with the other patch in mind: https://patchwork.freedesktop.org/patch/113763/ how about we just treat InputThreadDevice in state device_state_removed as "invisible" to InputThreadRegisterDev, i.e. we don't re-use the struct but allocate a new one. This way we have the old fd automatically removed and the new one added as expected, skipping the need for the device_state_re_added handling. This would be only a one-line change a bit above this hunk - if (old->fd == fd) { + if (old->fd == fd && dev->state != device_state_removed) { The only drawback is that we rely on xorg_list_append() and that the new entry is later than the previous one so we have the same remove/add order as in your device_state_re_added handling below. That needs a comment but other than that we should get the same result? Cheers, Peter > } 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 _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
