[PATCH] Input: new EVIOCGKEYF to get keystate and flush queues

[PATCH] Input: new EVIOCGKEYF to get keystate and flush queues

[David Herrmann]

EVIOCGKEYF returns the current keystate similar to EVIOCGKEY but also
flushes the outgoing evdev event queue. This allows user-space devices to
reliably retrieve the current state without having old events still in the
read()-queue.

Userspace libraries like XKB/xkbcommon use stacking key trackers, so they
allow the kernel to map multiple keys to the same keycode. For instance if
left-shift and right-shift map to the same keycode, xkbcommon allows the
kernel to send key-down twice but also requires it to send key-up twice. A
single input device doesn't support that, but this is very useful if you want
multiple connected keyboards to have one shared modifier state: Pressing Shift
on the one keyboard affects keys on the other keyboard. This is not the usual
setup but it is supported and known to be used (for instance if you have
external num-pads etc.).

When a program opens input devices, it can read the keystate via EVIOCGKEY
to update its internal keyboard state. This allows to keep CTRL pressed
while switching VTs and having both applications remember that CTRL is
still pressed.
However, if there are still outgoing events in the evdev queue, the
application will update its internal keyboard state and then read the
event again, even though it already processed it via EVIOCGKEY.

EVIOCGKEYF solves this race condition by flushing the evdev queue while
returning the keystate. We cannot change EVIOCGKEY because we don't know
whether userspace depends on a clean read() history and flushing the queue
effectively drops events.

This race could be fixed in user-space by keeping an exact copy of the keystate
array for each input device that we opened and ignoring double-events for a
single keycode. However, if we, as mentioned, want a shared keyboard-state, it
seems to be a waste of computing power and memory to keep another copy for each
input device if we could just use something like EVIOCGKEYF.

Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
---
Hi

I am actually not sure whether I did the locking in the right order. However, I
need to take event_lock _and_ buffer_lock to be sure neither the kernel-internal
keystate nor the output queue is changed as this ioctl needs to be atomic.

Regards
David

 drivers/input/evdev.c      | 10 ++++++++++
 include/uapi/linux/input.h |  1 +
 2 files changed, 11 insertions(+)

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index f0f8928..f178ae0 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -782,6 +782,16 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
 	case EVIOCGSW(0):
 		return bits_to_user(dev->sw, SW_MAX, size, p, compat_mode);
 
+	case EVIOCGKEYF(0):
+		spin_lock_irq(&dev->event_lock);
+		spin_lock(&client->buffer_lock);
+		error = bits_to_user(dev->key, KEY_MAX, size, p, compat_mode);
+		if (error >= 0)
+			client->tail = client->packet_head = client->head;
+		spin_unlock(&client->buffer_lock);
+		spin_unlock_irq(&dev->event_lock);
+		return error;
+
 	case EVIOCGNAME(0):
 		return str_to_user(dev->name, size, p);
 
diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
index 5588285..ade47d1 100644
--- a/include/uapi/linux/input.h
+++ b/include/uapi/linux/input.h
@@ -142,6 +142,7 @@ struct input_keymap_entry {
 #define EVIOCGLED(len)		_IOC(_IOC_READ, 'E', 0x19, len)		/* get all LEDs */
 #define EVIOCGSND(len)		_IOC(_IOC_READ, 'E', 0x1a, len)		/* get all sounds status */
 #define EVIOCGSW(len)		_IOC(_IOC_READ, 'E', 0x1b, len)		/* get all switch states */
+#define EVIOCGKEYF(len)		_IOC(_IOC_READ, 'E', 0x1c, len)		/* get global key state and flush queue */
 
 #define EVIOCGBIT(ev,len)	_IOC(_IOC_READ, 'E', 0x20 + (ev), len)	/* get event bits */
 #define EVIOCGABS(abs)		_IOR('E', 0x40 + (abs), struct input_absinfo)	/* get abs value/limits */
-- 
1.8.1

Re: [PATCH] Input: new EVIOCGKEYF to get keystate and flush queues

[Dmitry Torokhov]

Hi David,

On Mon, Jan 07, 2013 at 12:58:23PM +0100, David Herrmann wrote:
> EVIOCGKEYF returns the current keystate similar to EVIOCGKEY but also
> flushes the outgoing evdev event queue. This allows user-space devices to
> reliably retrieve the current state without having old events still in the
> read()-queue.
> 
> Userspace libraries like XKB/xkbcommon use stacking key trackers, so they
> allow the kernel to map multiple keys to the same keycode. For instance if
> left-shift and right-shift map to the same keycode, xkbcommon allows the
> kernel to send key-down twice but also requires it to send key-up twice. A
> single input device doesn't support that, but this is very useful if you want
> multiple connected keyboards to have one shared modifier state: Pressing Shift
> on the one keyboard affects keys on the other keyboard. This is not the usual
> setup but it is supported and known to be used (for instance if you have
> external num-pads etc.).
> 
> When a program opens input devices, it can read the keystate via EVIOCGKEY
> to update its internal keyboard state. This allows to keep CTRL pressed
> while switching VTs and having both applications remember that CTRL is
> still pressed.
> However, if there are still outgoing events in the evdev queue, the
> application will update its internal keyboard state and then read the
> event again, even though it already processed it via EVIOCGKEY.
> 
> EVIOCGKEYF solves this race condition by flushing the evdev queue while
> returning the keystate. We cannot change EVIOCGKEY because we don't know
> whether userspace depends on a clean read() history and flushing the queue
> effectively drops events.
> 
> This race could be fixed in user-space by keeping an exact copy of the keystate
> array for each input device that we opened and ignoring double-events for a
> single keycode. However, if we, as mentioned, want a shared keyboard-state, it
> seems to be a waste of computing power and memory to keep another copy for each
> input device if we could just use something like EVIOCGKEYF.

I do not think we should introduce the new ioctl as I really doubt
anyone is using it and even if they do I really doubt they will have
problems if we flush the events.

But rather that flushing all events we should only flush the key events.
The relative, absolute, and other events should be kept in the queue.

Thanks.

-- 
Dmitry

Re: [PATCH] Input: new EVIOCGKEYF to get keystate and flush queues

[David Herrmann]

Hi Dmitry

On Sat, Feb 16, 2013 at 10:10 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> Hi David,
>
> On Mon, Jan 07, 2013 at 12:58:23PM +0100, David Herrmann wrote:
>> EVIOCGKEYF returns the current keystate similar to EVIOCGKEY but also
>> flushes the outgoing evdev event queue. This allows user-space devices to
>> reliably retrieve the current state without having old events still in the
>> read()-queue.
>>
>> Userspace libraries like XKB/xkbcommon use stacking key trackers, so they
>> allow the kernel to map multiple keys to the same keycode. For instance if
>> left-shift and right-shift map to the same keycode, xkbcommon allows the
>> kernel to send key-down twice but also requires it to send key-up twice. A
>> single input device doesn't support that, but this is very useful if you want
>> multiple connected keyboards to have one shared modifier state: Pressing Shift
>> on the one keyboard affects keys on the other keyboard. This is not the usual
>> setup but it is supported and known to be used (for instance if you have
>> external num-pads etc.).
>>
>> When a program opens input devices, it can read the keystate via EVIOCGKEY
>> to update its internal keyboard state. This allows to keep CTRL pressed
>> while switching VTs and having both applications remember that CTRL is
>> still pressed.
>> However, if there are still outgoing events in the evdev queue, the
>> application will update its internal keyboard state and then read the
>> event again, even though it already processed it via EVIOCGKEY.
>>
>> EVIOCGKEYF solves this race condition by flushing the evdev queue while
>> returning the keystate. We cannot change EVIOCGKEY because we don't know
>> whether userspace depends on a clean read() history and flushing the queue
>> effectively drops events.
>>
>> This race could be fixed in user-space by keeping an exact copy of the keystate
>> array for each input device that we opened and ignoring double-events for a
>> single keycode. However, if we, as mentioned, want a shared keyboard-state, it
>> seems to be a waste of computing power and memory to keep another copy for each
>> input device if we could just use something like EVIOCGKEYF.
>
> I do not think we should introduce the new ioctl as I really doubt
> anyone is using it and even if they do I really doubt they will have
> problems if we flush the events.
>
> But rather that flushing all events we should only flush the key events.
> The relative, absolute, and other events should be kept in the queue.

(seems we have nearly the same delay for responses, sorry..)

I'm not entirely sure whether I understand you correctly. Do you
propose to just fix EVIOCGKEY to flush the queues? It does break
semantics, but I agree, that this probably fixes existing userspace
and does not break it.

So if you agree, I will prepare a fix that flushes pending KEY-events
for EVIOCGKEY. And I will try to do the same for ABS/REL events. And
if you're too busy, I'll send it anyway ;)

I'm also talking to XKB developers and it really seems that userspace
already expects those semantics. I haven't found any application that
expects the queues to report the duplicate events.

But I'd really like to see this fixed. I hate knowing that there is a
theoretical race-condition and I keep reminding myself of it..

Thanks
David

Re: [PATCH] Input: new EVIOCGKEYF to get keystate and flush queues

[Dmitry Torokhov]

On Wednesday, March 27, 2013 10:02:31 PM David Herrmann wrote:
> Hi Dmitry
> 
> On Sat, Feb 16, 2013 at 10:10 PM, Dmitry Torokhov
> 
> <dmitry.torokhov@gmail.com> wrote:
> > Hi David,
> > 
> > On Mon, Jan 07, 2013 at 12:58:23PM +0100, David Herrmann wrote:
> >> EVIOCGKEYF returns the current keystate similar to EVIOCGKEY but also
> >> flushes the outgoing evdev event queue. This allows user-space devices to
> >> reliably retrieve the current state without having old events still in
> >> the
> >> read()-queue.
> >> 
> >> Userspace libraries like XKB/xkbcommon use stacking key trackers, so they
> >> allow the kernel to map multiple keys to the same keycode. For instance
> >> if
> >> left-shift and right-shift map to the same keycode, xkbcommon allows the
> >> kernel to send key-down twice but also requires it to send key-up twice.
> >> A
> >> single input device doesn't support that, but this is very useful if you
> >> want multiple connected keyboards to have one shared modifier state:
> >> Pressing Shift on the one keyboard affects keys on the other keyboard.
> >> This is not the usual setup but it is supported and known to be used
> >> (for instance if you have external num-pads etc.).
> >> 
> >> When a program opens input devices, it can read the keystate via
> >> EVIOCGKEY
> >> to update its internal keyboard state. This allows to keep CTRL pressed
> >> while switching VTs and having both applications remember that CTRL is
> >> still pressed.
> >> However, if there are still outgoing events in the evdev queue, the
> >> application will update its internal keyboard state and then read the
> >> event again, even though it already processed it via EVIOCGKEY.
> >> 
> >> EVIOCGKEYF solves this race condition by flushing the evdev queue while
> >> returning the keystate. We cannot change EVIOCGKEY because we don't know
> >> whether userspace depends on a clean read() history and flushing the
> >> queue
> >> effectively drops events.
> >> 
> >> This race could be fixed in user-space by keeping an exact copy of the
> >> keystate array for each input device that we opened and ignoring
> >> double-events for a single keycode. However, if we, as mentioned, want a
> >> shared keyboard-state, it seems to be a waste of computing power and
> >> memory to keep another copy for each input device if we could just use
> >> something like EVIOCGKEYF.
> > 
> > I do not think we should introduce the new ioctl as I really doubt
> > anyone is using it and even if they do I really doubt they will have
> > problems if we flush the events.
> > 
> > But rather that flushing all events we should only flush the key events.
> > The relative, absolute, and other events should be kept in the queue.
> 
> (seems we have nearly the same delay for responses, sorry..)
> 
> I'm not entirely sure whether I understand you correctly. Do you
> propose to just fix EVIOCGKEY to flush the queues?

Yes, that is exactly what I mean, with the exception that flush should
be selective. I.e. getting key state should not flush relative or absolute
events and getting absolute state shoudl not flush key/bunnon events and
so on.

Thanks.

-- 
Dmitry