[PATCH] Input - mt: Fix input_mt_get_slot_by_key

[PATCH] Input - mt: Fix input_mt_get_slot_by_key

[Benjamin Tissoires]

The case occured recently with a touchscreen using twice a slot during a
single EV_SYN event:

E: 0.288415 0000 0000 0000      # ------------ SYN_REPORT (0) ----------
E: 0.296207 0003 002f 0000      # EV_ABS / ABS_MT_SLOT          0
E: 0.296207 0003 0039 -001      # EV_ABS / ABS_MT_TRACKING_ID   -1
E: 0.296207 0003 002f 0001      # EV_ABS / ABS_MT_SLOT          1
E: 0.296207 0003 0035 0908      # EV_ABS / ABS_MT_POSITION_X    908
E: 0.296207 0003 0036 1062      # EV_ABS / ABS_MT_POSITION_Y    1062
E: 0.296207 0003 002f 0000      # EV_ABS / ABS_MT_SLOT          0
E: 0.296207 0003 0039 8787      # EV_ABS / ABS_MT_TRACKING_ID   8787
E: 0.296207 0003 0035 1566      # EV_ABS / ABS_MT_POSITION_X    1566
E: 0.296207 0003 0036 0861      # EV_ABS / ABS_MT_POSITION_Y    861
E: 0.296207 0003 0000 0908      # EV_ABS / ABS_X                908
E: 0.296207 0003 0001 1062      # EV_ABS / ABS_Y                1062
E: 0.296207 0000 0000 0000      # ------------ SYN_REPORT (0) ----------

This occured because while having already slots 0 and 1 assigned, the
touchscreen sent:

0.293377 Tip Switch: 0 | Contact Id: 0 | X:  539 | Y: 1960 | Contact Count: 3
0.294783 Tip Switch: 1 | Contact Id: 1 | X:  908 | Y: 1062 | Contact Count: 0
0.296187 Tip Switch: 1 | Contact Id: 2 | X: 1566 | Y:  861 | Contact Count: 0

Slot 0 is released correclty, but when we look for Contact ID 2, the slot
0 is then picked up again because it is marked as inactive (trackingID < 0).

This is wrong, and we should not reuse a slot in the same frame.
The test should also check for input_mt_is_used().

In addition, we need to initialize mt->frame to an other value than 0.
With mt->frame being 0, all slots are tagged as being used, and so
input_mt_get_slot_by_key() would return -1 for all requests.

Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=88903

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---

Hi Dmitry,

I wonder if this one should go to stable as well.

Cheers,
Benjamin

 drivers/input/input-mt.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/input/input-mt.c b/drivers/input/input-mt.c
index cb150a1..e02fd92 100644
--- a/drivers/input/input-mt.c
+++ b/drivers/input/input-mt.c
@@ -88,10 +88,13 @@ int input_mt_init_slots(struct input_dev *dev, unsigned int num_slots,
 			goto err_mem;
 	}
 
-	/* Mark slots as 'unused' */
+	/* Mark slots as 'inactive' */
 	for (i = 0; i < num_slots; i++)
 		input_mt_set_value(&mt->slots[i], ABS_MT_TRACKING_ID, -1);
 
+	/* Mark slots as 'unused' */
+	mt->frame = 1;
+
 	dev->mt = mt;
 	return 0;
 err_mem:
@@ -453,7 +456,7 @@ int input_mt_get_slot_by_key(struct input_dev *dev, int key)
 			return s - mt->slots;
 
 	for (s = mt->slots; s != mt->slots + mt->num_slots; s++)
-		if (!input_mt_is_active(s)) {
+		if (!input_mt_is_active(s) && !input_mt_is_used(mt, s)) {
 			s->key = key;
 			return s - mt->slots;
 		}
-- 
2.1.0

Re: [PATCH] Input - mt: Fix input_mt_get_slot_by_key

[Henrik Rydberg]

Hi Benjamin,

> Slot 0 is released correclty, but when we look for Contact ID 2, the slot
> 0 is then picked up again because it is marked as inactive (trackingID < 0).
> 
> This is wrong, and we should not reuse a slot in the same frame.
> The test should also check for input_mt_is_used().

Good catch! However...

> @@ -453,7 +456,7 @@ int input_mt_get_slot_by_key(struct input_dev *dev, int key)
>  			return s - mt->slots;
>  
>  	for (s = mt->slots; s != mt->slots + mt->num_slots; s++)
> -		if (!input_mt_is_active(s)) {
> +		if (!input_mt_is_active(s) && !input_mt_is_used(mt, s)) {
>  			s->key = key;
>  			return s - mt->slots;
>  		}
> 

Here, you are changing the preconditions of the function without explicit
reference to all its users. For one, it is now assumed that input_mt_is_used()
is up-to-date, which requires either input_mt_drop_unused() or
input_mt_sync_frame(), which does not seem to be true for all users of
input_mt_get_slot_by_key(). After a couple of iterations with
input_mt_report_slot_state() in those drivers, input_mt_is_used() will be true
for all slots, and the driver will stop working.

How about defering the deassignments until the end of the loop instead? That
would remove possible reuse.

Thanks,
Henrik

Re: [PATCH] Input - mt: Fix input_mt_get_slot_by_key

[Benjamin Tissoires]

On Feb 03 2015 or thereabouts, Henrik Rydberg wrote:
> Hi Benjamin,
> 
> > Slot 0 is released correclty, but when we look for Contact ID 2, the slot
> > 0 is then picked up again because it is marked as inactive (trackingID < 0).
> > 
> > This is wrong, and we should not reuse a slot in the same frame.
> > The test should also check for input_mt_is_used().
> 
> Good catch! However...
> 
> > @@ -453,7 +456,7 @@ int input_mt_get_slot_by_key(struct input_dev *dev, int key)
> >  			return s - mt->slots;
> >  
> >  	for (s = mt->slots; s != mt->slots + mt->num_slots; s++)
> > -		if (!input_mt_is_active(s)) {
> > +		if (!input_mt_is_active(s) && !input_mt_is_used(mt, s)) {
> >  			s->key = key;
> >  			return s - mt->slots;
> >  		}
> > 
> 
> Here, you are changing the preconditions of the function without explicit
> reference to all its users. For one, it is now assumed that input_mt_is_used()
> is up-to-date, which requires either input_mt_drop_unused() or
> input_mt_sync_frame(), which does not seem to be true for all users of
> input_mt_get_slot_by_key(). After a couple of iterations with
> input_mt_report_slot_state() in those drivers, input_mt_is_used() will be true
> for all slots, and the driver will stop working.

Hmm... You are right. Thanks for pointing this out. This is something I
should have definitively thought.

So, on the kernel tree, I have only 5 drivers matching
input_mt_get_slot_by_key:

$ grep -lIr input_mt_get_slot_by_key | grep -E "\.[ch]$"
drivers/hid/hid-logitech-hidpp.c
drivers/hid/hid-multitouch.c
drivers/hid/wacom_wac.c
drivers/input/input-mt.c
drivers/input/touchscreen/sur40.c
drivers/input/touchscreen/pixcir_i2c_ts.c
include/linux/input/mt.h

On these five drivers, 4 are using properly input_mt_sync_frame, and
only one is not. This one is the Wacom one :)

For most of the touch devices (except those that are auto-detected and
handled), the code just calls input_mt_report_pointer_emulation()
instead of input_mt_sync_frame().

Changing the 5 calls in this driver and adding a requirement to call
input_mt_sync_frame() when using input_mt_get_slot_by_key() does not
seem terrible.

> 
> How about defering the deassignments until the end of the loop instead? That
> would remove possible reuse.
> 

I don't think this is feasible. There is no loop in the slot assignment
case. The state is stored by the input subsystem, and when we process
a touch, all the previous touches have already been processed and forwarded
to the user space.

We *could* defer the slot release before sending the EV_SYN event, but
that would force users to call input_mt_sync_frame() anyway to release
the slots. So we would get an even greater requirement, because now, all
users of the slotted protocol would have to call input_mt_sync_frame().

Cheers,
Benjamin