Re: [PATCH xserver] inputthread: Re-add fd to the inputThreadInfo->fds pollfd set when re-added

2016-10-14 Thread Keith Packard
Mihail Konev  writes:

> On Fri, Oct 14, 2016 at 02:38:00PM +1000, Peter Hutterer wrote:
>> 
>> 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
>> 
>
> This works:
>
> diff --git a/os/inputthread.c b/os/inputthread.c
> index 6aa0a9ce6fb5..ddafa7fe8343 100644
> --- a/os/inputthread.c
> +++ b/os/inputthread.c
> @@ -197,7 +197,7 @@ InputThreadRegisterDev(int fd,
>  
>  dev = NULL;
>  xorg_list_for_each_entry(old, >devs, node) {
> -if (old->fd == fd) {
> +if (old->fd == fd && old->state != device_state_removed) {
>  dev = old;
>  break;
>  }
> @@ -218,6 +218,9 @@ InputThreadRegisterDev(int fd,
>  dev->readInputProc = readInputProc;
>  dev->readInputArgs = readInputArgs;
>  dev->state = device_state_added;
> +
> +/* Do not prepend, so that any dev->state == device_state_removed
> + * with the same dev->fd get processed first. */
>  xorg_list_append(>node, >devs);
>  }

Looks good to me.

Reviewed-by: Keith Packard 

-- 
-keith


signature.asc
Description: PGP signature
___
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

Re: [PATCH xserver] inputthread: Re-add fd to the inputThreadInfo->fds pollfd set when re-added

2016-10-14 Thread Hans de Goede

Hi,

On 10/14/2016 06:38 AM, Peter Hutterer wrote:

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 
Signed-off-by: Hans de Goede 
---
 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?


I agree that as long as we can somehow guarantee that the ospoll_remove
happens before the ospoll_add that we can then simply fix this by not
re-using the struct.

Regards,

Hans






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

___
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


___
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

Re: [PATCH xserver] inputthread: Re-add fd to the inputThreadInfo->fds pollfd set when re-added

2016-10-13 Thread Peter Hutterer
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 
> Signed-off-by: Hans de Goede 
> ---
>  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
___
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