[PATCH xf86-input-libinput] Replace the "fd" option when enabling dependent devices

[Hans de Goede]

Hi,

On 20-11-15 03:29, Peter Hutterer wrote:
> The server uses pInfo->major/minor to detect if another device is using the
> same path for a logind-controlled fd. If so, it reuses that device's
> pInfo->fd and sets the "fd" option to that value.
>
> That pInfo->fd is the libinput epollfd though, not the actual device fd.
>
> This doesn't matter for us, since we manage the fds largely ourselves and the
> pInfo->fd we use is the epollfd anyway. On unplug however, the udev code
> triggers a device removal for all devices, including the duplicated ones. When
> we disable device, we restore the pInfo->fd from the "fd" option so that the
> server can request logind to close the fd.
>
> That only works if the "fd" option is correct, otherwise the server asks
> logind to close the epollfd and everyone is unhappy.
>
> Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> ---
> I had to revert the patch shortly after pushing because it caused hangs in a
> logind setup on device unplug. This is on top of the original bug since it's
> a bit convoluted and a lot easier to understand when it's not squashed in.
> For the actual final patch, I'll squash them together though.
>
> Not happy with this approach, but short of a big rewrite of the server's
> input APIs there isn't that much we can do. And no-one is keen to do that
> atm :)

FWIW I cannot think of a better fix either.

It took me a while to wrap my head around this patch, but after that I
cannot find anything wrong with it:

Reviewed-by: Hans de Goede <hdegoede at redhat.com>

Regards,

Hans



>   src/xf86libinput.c | 37 ++++++++++++++++++++++++++++++++++---
>   1 file changed, 34 insertions(+), 3 deletions(-)
>
> diff --git a/src/xf86libinput.c b/src/xf86libinput.c
> index 430fc9a..ee2165a 100644
> --- a/src/xf86libinput.c
> +++ b/src/xf86libinput.c
> @@ -83,6 +83,7 @@ struct xf86libinput_device {
>   	uint32_t id;
>   	struct libinput_device *device;
>   	struct xorg_list device_list;
> +	int server_fd;
>   };
>
>   struct xf86libinput {
> @@ -236,15 +237,40 @@ xf86libinput_shared_unref(struct xf86libinput_device *shared_device)
>   }
>
>   static inline struct libinput_device *
> -xf86libinput_shared_enable(struct xf86libinput_device *shared_device,
> +xf86libinput_shared_enable(InputInfoPtr pInfo,
> +			   struct xf86libinput_device *shared_device,
>   			   const char *path)
>   {
>   	struct libinput_device *device;
>   	struct libinput *libinput = driver_context.libinput;
>
> +	/* With systemd-logind the server requests the fd from logind, sets
> +	 * pInfo->fd and sets the "fd" option to the fd number.
> +	 *
> +	 * If we have a second device that uses the same path, the server
> +	 * checks all pInfo->major/minor for a match and returns the matched
> +	 * device's pInfo->fd. In this driver, this fd is the epollfd, not
> +	 * the actual device. This causes troubles when removing the
> +	 * device.
> +	 *
> +	 * What we need to do here is: after enabling the device the first
> +	 * time extract the real fd and store it in the shared device
> +	 * struct. The second device replaces the pInfo->options "fd" with
> +	 * the real fd we're using.
> +	 *
> +	 * When the device is unplugged, the server now correctly finds two
> +	 * devices on the real fd and releases them in order.
> +	 */
>   	shared_device->enabled_count++;
> -	if (shared_device->enabled_count > 1)
> +	if (shared_device->enabled_count > 1) {
> +		if (pInfo->flags & XI86_SERVER_FD) {
> +			pInfo->options = xf86ReplaceIntOption(pInfo->options,
> +							      "fd",
> +							      shared_device->server_fd);
> +		}
> +
>   		return shared_device->device;
> +	}
>
>   	device = libinput_path_add_device(libinput, path);
>   	if (!device)
> @@ -253,6 +279,10 @@ xf86libinput_shared_enable(struct xf86libinput_device *shared_device,
>   	libinput_device_set_user_data(device, shared_device);
>   	shared_device->device = libinput_device_ref(device);
>
> +	if (pInfo->flags & XI86_SERVER_FD)
> +		shared_device->server_fd = xf86CheckIntOption(pInfo->options,
> +							      "fd",
> +							      -1);
>   	return device;
>   }
>
> @@ -437,7 +467,8 @@ xf86libinput_on(DeviceIntPtr dev)
>   	struct libinput *libinput = driver_context.libinput;
>   	struct libinput_device *device;
>
> -	device = xf86libinput_shared_enable(shared_device,
> +	device = xf86libinput_shared_enable(pInfo,
> +					    shared_device,
>   					    driver_data->path);
>   	if (!device)
>   		return !Success;
>